Skip to content

Conversation

@abnegate
Copy link
Member

@abnegate abnegate commented Oct 6, 2025

Summary by CodeRabbit

  • New Features

    • Clear validation error when attempting to order by nested attributes (e.g., relationship fields): “Cannot order by nested attribute: {attribute}”.
  • Bug Fixes

    • Prevents ordering and cursor usage with nested relationship attributes to avoid inconsistent or unsupported query behavior.
  • Tests

    • Added end-to-end tests verifying rejection of ordering and cursor operations on nested relationship attributes and the validation message.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 6, 2025

Walkthrough

Adds validation that rejects ordering by nested attributes when the top-level segment exists in the schema, causing an immediate validation failure and message "Cannot order by nested attribute: {attribute}". Adds an e2e test asserting this behavior for both order-by and cursor usage with a relationship attribute.

Changes

Cohort / File(s) Summary
Order validation logic
src/Database/Validator/Query/Order.php
When an order attribute contains a dot and its top-level segment exists in the schema, return false immediately and set error "Cannot order by nested attribute: {attribute}".
E2E tests for relationship ordering
tests/e2e/Adapter/Scopes/RelationshipTests.php
Adds testOrderAndCursorWithRelationshipQueries to create collections, seed data, assert validation errors for ordering/cursor by author.name, and clean up collections.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant O as Order::isValid
  participant S as Schema

  C->>O: Validate order attribute (e.g., "author.name")
  O->>O: Detect dot in attribute
  O->>S: Check top-level segment exists
  alt Top-level exists and attribute is nested
    O-->>C: Error "Cannot order by nested attribute: {attribute}"\nreturn false
  else No dot or top-level not found
    O->>O: Proceed with standard validation
    O-->>C: Validation result (success/failure)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • 1.x #717 — Modifies the same src/Database/Validator/Query/Order.php validator logic; likely related to overlapping changes in order validation.

Poem

A rabbit taps keys with a mindful hop,
"No nested orders!" it chirps on the top.
Tests hop in circles to catch every case,
Authors and posts keep their proper place.
Validation stands firm — neat, quick, and smart. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title “Validate order” is too generic and does not clearly convey that the main change is adding validation to reject nested attribute ordering in database queries, leaving readers without context for the specific behavior introduced. Consider renaming the pull request to something more descriptive, for example “Disallow ordering by nested attributes in Query Order validator” to clearly summarize the primary change.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-relationship-updates

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f1307e0 and 1b22598.

📒 Files selected for processing (1)
  • tests/e2e/Adapter/Scopes/RelationshipTests.php (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/e2e/Adapter/Scopes/RelationshipTests.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Setup & Build Docker Image

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 and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/Database/Validator/Query/Order.php (1)

40-44: Consider including the full original attribute path in the error message.

The error message currently uses the truncated top-level segment (e.g., "author") rather than the full dot-path the user provided (e.g., "author.name"). While the validation logic is correct, including the full original attribute would make debugging easier for users.

You could store the original attribute before exploding and use it in the error message:

+            $originalAttribute = $attribute;
             $attribute = \explode('.', $attribute)[0];

             if (isset($this->schema[$attribute])) {
-                $this->message = 'Cannot order by nested attribute: ' . $attribute;
+                $this->message = 'Cannot order by nested attribute: ' . $originalAttribute;
                 return false;
             }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8475957 and f1307e0.

📒 Files selected for processing (2)
  • src/Database/Validator/Query/Order.php (1 hunks)
  • tests/e2e/Adapter/Scopes/RelationshipTests.php (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.

Applied to files:

  • tests/e2e/Adapter/Scopes/RelationshipTests.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php (3)
src/Database/Database.php (2)
  • Database (37-8134)
  • find (6695-6826)
src/Database/Adapter/SQL.php (1)
  • find (2334-2513)
src/Database/Query.php (4)
  • Query (8-1116)
  • equal (435-438)
  • orderAsc (602-605)
  • cursorAfter (645-648)
🔇 Additional comments (4)
tests/e2e/Adapter/Scopes/RelationshipTests.php (4)

4416-4432: LGTM!

The test setup correctly creates the necessary collections, attributes, and relationships to validate ordering behavior with nested relationship attributes.


4434-4471: LGTM!

Test data is properly structured with appropriate authors and posts to exercise the validation logic.


4473-4483: LGTM!

The test correctly verifies that ordering by a nested relationship attribute throws a validation error with the expected message. The use of $caught flag ensures the exception is actually thrown.


4485-4500: LGTM!

The cursor test correctly verifies that combining a cursor with an invalid nested order attribute triggers the same validation error. The test properly distinguishes between creating a valid cursor and using it with an invalid order attribute.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@abnegate abnegate merged commit 33ca426 into main Oct 6, 2025
17 checks passed
@abnegate abnegate deleted the feat-relationship-updates branch October 6, 2025 04:04
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.

2 participants