Skip to content

fix: Validate the order by names of the entity queries - #48

Merged
joamag merged 2 commits into
masterfrom
fix/order-by-names
Oct 1, 2026
Merged

joamag merged 2 commits into
masterfrom
fix/order-by-names

Conversation

@joamag

@joamag joamag commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #47, the client side being hivesolutions/uxf#40.

Cause

The sort value of a request (sort or order) is turned by create_filter into the order by of the query, and _resolve_name wrote its names into the order by clause without checking them, unlike the names of the filters, which are validated with _validate_name. An unknown name, such as the translated markup sent by the UXF filter of a page translated by the browser (OMNI-LDJ-15), reached the database as it was, failing the query with a syntax error and the request with an internal error. Unknown relation names of a dotted path were written as well, as get_relation returns an empty descriptor for them.

Changes

  • _resolve_name (data/src/entity_manager/system.py, only called by _order_query_f): validates each relation name of the path against the class it belongs to and the final name against the target class of the relation, raising ValidationError for unknown names. Data references in the path are resolved into their real classes through get_entity(), as the joins do, raising ValidationError when one is not resolved (the relation is not joined in that case).
  • _class_create_filter (mvc/src/mvc_utils/entity_model.py): the sort value of the request is used only when it is __default__, __identifier__, a reserved name (_class and _mtime, as in the entity manager) or a name of the entity (or of an eager loaded relation, for dotted names), otherwise the default order is kept, as already done for the filters of relations that are not eager loaded. The order by is now built after the eager relations of the request are merged, so that a relation requested through eager can still be sorted by.
  • resolve() (inner function of _class_create_filter): resolves the data references of the path into their real classes at every level, keeping the reference when it is not found. Besides the sorting, this makes the filters through data references cast their values with the real class, which previously discarded them (None) for the attributes not declared in the reference.
  • New MockEntityManager, MockEntity, MockAddress, MockAddressReference, MockPerson, MockPersonReference and MockCompany in mvc/src/mvc_utils/mocks.py.
  • CHANGELOG entries under Fixed.

Verification

  • 13 new tests: test_order_by_identifier, test_order_by_invalid, test_order_by_reserved and test_order_by_reference in the entity manager, and the new CreateFilterTestCase (9 tests) in the MVC utils. test_order_by_invalid fails against the previous code (no such column: _person.unknown, the name reaching SQLite), test_order_by_reference fails without the resolution of the data references (attribute does not exist in 'Address', validated against the reference), as do 6 of the CreateFilterTestCase tests, and all of them pass with the change.
  • Coverage of the changed statements: 26 of 26 (100%), measured with coverage.py.
  • Complete suite passing locally (24 bundles, 1281 tests) and black --check clean.
  • The Omni suite (1670 tests) passes against this branch. The sort options of the Omni lists that sort through relations (11 entities, eg: primary_contact_information.email) all have the relation eager loaded when the filter is created, so they keep being accepted.

- The entity manager validates every name of the order by, as for filters
- Create filter ignores sort values that are not attributes of the entity
- Unknown sort values (eg: from translated pages) keep the default order
- Tests cover the identifier values, unknown names and relation paths
Copilot AI balanced review requested due to automatic review settings September 30, 2026 19:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T20:04:29.699419Z 3cde856 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f50a5107-de8e-4380-ad4a-06943f46f525

📥 Commits

Reviewing files that changed from the base of the PR and between 3cde856 and e3f5e76.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • data/src/entity_manager/system.py
  • data/src/entity_manager/test.py
  • mvc/src/mvc_utils/entity_model.py
  • mvc/src/mvc_utils/mocks.py
  • mvc/src/mvc_utils/test.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The MVC filter validates requested sort fields against entity attributes and eager-loaded relations. Invalid or unavailable fields retain the configured order. The entity manager validates relation paths and attribute names. Tests cover sorting, reference resolution, and nested relation filters.

Changes

Entity query sort and relation validation

Layer / File(s) Summary
MVC filter sort and relation resolution
mvc/src/mvc_utils/entity_model.py, mvc/src/mvc_utils/mocks.py, mvc/src/mvc_utils/test.py, CHANGELOG.md
The MVC filter applies a requested sort only when its field is supported, including fields on eager-loaded relations. Registered reference targets are resolved for relation sorting and filtering. Tests cover accepted and rejected sorts, nested relation filters, and the changelog records the behavior.
Entity manager relation-name validation
data/src/entity_manager/system.py, data/src/entity_manager/test.py
The entity manager validates relation-path components and final attribute names, resolving registered reference classes where available. Tests cover identifier and _mtime ordering, invalid names and paths, and reference relations.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to e3f5e

The change validates query names while preserving default sorting for invalid requests. No actionable merge-blocking issue remains; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e3f5e

The change strengthens sort-field validation and preserves configured ordering for unsupported input. No introduced authorization bypass was established. Remaining uncertainty concerns whether applications share mutable query defaults across requests and how relation registrations are governed.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly evidenced exposure is request-influenced entity queries using configured eager relations and registered target metadata. The inspected changes do not establish a new service or credential boundary. The maximum tenant, application, and database exposure remains dependent on uninspected callers and authorization policy.

Trust Boundaries and Controls

  • observed — Request eager relations are intersected with the configured allowed set. Dotted MVC sorts must resolve through the resulting eager structure, and final names are constrained by reserved names or entity metadata. These are query-structure controls, not evidence of tenant or row-level authorization.

Resilience and Maintainability Implications

  • inferred — Normalization and nested eager-filter construction mutate supplied maps. Reusing nonempty defaults could preserve request-derived relations or filters across repetition, exceptions, or concurrent calls. This mutation predates the PR; production sharing and any materially worsened exposure from concrete reference casting were not established. The new sort assignment itself remains local.

Hardening Proposals

  • proposed — If applications reuse query defaults, make nested eager state request-owned and verify isolation across repeated, interrupted, and concurrent calls, including registered-reference filters. This is conditional hardening, not an observed PR-introduced vulnerability.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 5 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR #48 meets the coding requirements in directly linked issue #47. _class_create_filter validates sort and order values, accepts __default__, __identifier__, reserved names, entity attribute…
Out of Scope Changes check ✅ Passed The changes stay within issue #47. Reference-class resolution supports validation of sort paths through data references. The related filter-value tests verify the shared resolution behavior and do not…
Title check ✅ Passed The title clearly and concisely describes the main change: validating order-by names in entity queries.
Description check ✅ Passed The description directly explains the cause, implementation, tests, and verification for the order-by validation changes.
Full details: Docstring Coverage

Explanation

Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 5 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3cde856cb5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread mvc/src/mvc_utils/entity_model.py
Comment thread data/src/entity_manager/system.py
- Order by names through data references validate against the real class
- Reserved names, such as the modification time, are valid sort values
- Relation paths of filters resolve data references, keeping their values
- Tests cover resolved, nested and unresolved data references
@joamag
joamag merged commit 40a4b9b into master Oct 1, 2026
39 checks passed
@joamag
joamag deleted the fix/order-by-names branch October 1, 2026 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate the names of the order by of the entity queries

2 participants