Skip to content

Comments

[MPT-18174] Fix any and all with multiple nested conditions for a collection#214

Merged
robcsegal merged 1 commit intomainfrom
MPT-18174-fix-any-and-all-with-multiple-nested-conditions-for-a-collection
Feb 18, 2026
Merged

[MPT-18174] Fix any and all with multiple nested conditions for a collection#214
robcsegal merged 1 commit intomainfrom
MPT-18174-fix-any-and-all-with-multiple-nested-conditions-for-a-collection

Conversation

@robcsegal
Copy link
Contributor

@robcsegal robcsegal commented Feb 18, 2026

Any and all are treating nested conditions incorrectly. It wraps entire set of conditions, instead of properly using collection name:

Collection Name is used because inferring possible collections in query values is complex and prone to inference errors. Simplest and safest way I can think of is using collection name.

https://softwareone.atlassian.net/browse/MPT-18174

Closes MPT-18174

  • Fix nested condition handling for any() and all(): these methods now require an explicit collection_name and build nested expressions using that collection instead of wrapping the entire condition set.
  • Public API changed: RQLQuery.any(collection_name) and RQLQuery.all(collection_name) now require a collection/path argument (no-argument forms removed).
  • Added internal _nest helper to compose collection-scoped nested expressions (e.g., any(saleDetails,gt(orderQty,1))).
  • Multi-condition queries now format correctly for collections (e.g., all(saleDetails,and(gt(orderQty,1),lt(price,100)))).
  • Tests updated to validate explicit collection-name usage and multiple nested condition scenarios.

@robcsegal robcsegal requested a review from a team as a code owner February 18, 2026 03:43
@coderabbitai
Copy link

coderabbitai bot commented Feb 18, 2026

📝 Walkthrough

Walkthrough

The RQL query builder now requires a collection_name parameter for any() and all() and uses a new internal _nest(op, collection_name) helper to construct nested expressions. Unit tests were updated to pass explicit relation paths and to assert the updated string representations.

Changes

Cohort / File(s) Summary
Core implementation
mpt_api_client/rql/query_builder.py
any(collection_name) and all(collection_name) signatures changed to require a collection name; added _nest(self, op: str, collection_name: str) to assemble nested expressions and replaced prior direct construction with _nest. Docstrings and examples updated to collection,condition syntax.
Unit tests
tests/unit/rql/query_builder/test_rql_all_any.py
Tests updated to call any("saleDetails") / all("saleDetails"), and assertions updated to expect any(saleDetails,...) / all(saleDetails,...) string forms, including combined and(...) conditions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Jira Issue Key In Title ✅ Passed PR title contains exactly one Jira issue key in the required MPT-XXXX format: MPT-18174.
Test Coverage Required ✅ Passed PR modifies 1 code file and includes corresponding test file changes with 28 insertions and 3 deletions. Test coverage requirement is met.
Single Commit Required ✅ Passed The PR contains exactly one commit addressing the stated issue, meeting the single commit requirement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Copy link

@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: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 531-538: The _nest method contains excess blank lines violating
WPS473; remove the extra blank lines so the body is compact (declare name =
collection_name.replace("__", "."), then collection = self._to_string(self) if
self.children else self.expr or "", then expr = f"{op}({name},{collection})",
then return self.new(expr=expr)) without blank lines between these statements;
update the function _nest in class/method where Self and new(...) are used to
keep the same logic but with no extra empty lines.
- Around line 260-286: The docstring examples for the any and all methods are
incorrect; update the examples in the any() and all() docstrings to reflect the
actual RQL output format produced by _nest using OP_ANY/OP_ALL (e.g., show
gt(orderQty,11) instead of orderQty=11, and any(saleDetails,and(...)) or
all(saleDetails,and(...)) for combined queries) so they match the real output
syntax produced by RQLQuery._nest and the OP_ANY/OP_ALL operators.

In `@tests/unit/rql/query_builder/test_rql_all_any.py`:
- Around line 20-24: The test function test_all_multiple_conditions contains
blank lines inside the test body which violate AAA05; remove the empty lines so
the test has a single contiguous block (e.g., no blank lines between the
RQLQuery assignments and the final query expression) and apply the same removal
to the other affected test functions referenced (around lines 31-35) to satisfy
the linter rules.

@robcsegal robcsegal force-pushed the MPT-18174-fix-any-and-all-with-multiple-nested-conditions-for-a-collection branch from dbb3569 to dde678d Compare February 18, 2026 03:48
Copy link

@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.

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 267-270: Update the docstring examples for the any() method in
rql/query_builder.py to match the actual output of _nest(): for a single
predicate use any(saleDetails,gt(orderQty,11)) and for combined predicates use
any(saleDetails,and(gt(orderQty,11),lt(price,100))). Ensure the examples
reference the any() method and reflect that the collection name is the first
argument to any() and the combined predicates are passed as the second argument
(produced by _nest()).
- Around line 283-284: The docstring example for combining multiple conditions
is wrong: update the example under RQLQuery (the usage of .all in the docstring)
so the collection name is the second argument to all() instead of inside the
and(), i.e. change the expected output from
all(and(saleDetails,gt(orderQty,11),lt(price,100))) to
all(saleDetails,and(gt(orderQty,11),lt(price,100))) so the RQLQuery/.all
documentation matches the actual output format.

@robcsegal robcsegal marked this pull request as draft February 18, 2026 04:31
@robcsegal robcsegal marked this pull request as ready for review February 18, 2026 05:24
@robcsegal robcsegal force-pushed the MPT-18174-fix-any-and-all-with-multiple-nested-conditions-for-a-collection branch from dde678d to eb284ec Compare February 18, 2026 13:16
@sonarqubecloud
Copy link

Copy link

@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.

🧹 Nitpick comments (1)
tests/unit/rql/query_builder/test_rql_all_any.py (1)

30-37: Consider adding tests for OR operator.

The current tests cover combining conditions with & (AND). For completeness, consider adding tests for | (OR) operator to verify the output format like any(saleDetails,or(gt(orderQty,1),lt(price,100))).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/rql/query_builder/test_rql_all_any.py` around lines 30 - 37, Add a
new unit test mirroring test_any_multiple_conditions that constructs two
RQLQuery instances (e.g., order_qty_query = RQLQuery(orderQty__gt=1) and
price_query = RQLQuery(price__lt=100)), combine them with the OR operator using
the pipe symbol (order_qty_query | price_query), call .any("saleDetails") on the
combined query, convert to string and assert the result equals
"any(saleDetails,or(gt(orderQty,1),lt(price,100)))"; this verifies the OR path
in the RQLQuery and any() formatting for the or(...) form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/rql/query_builder/test_rql_all_any.py`:
- Around line 30-37: Add a new unit test mirroring test_any_multiple_conditions
that constructs two RQLQuery instances (e.g., order_qty_query =
RQLQuery(orderQty__gt=1) and price_query = RQLQuery(price__lt=100)), combine
them with the OR operator using the pipe symbol (order_qty_query | price_query),
call .any("saleDetails") on the combined query, convert to string and assert the
result equals "any(saleDetails,or(gt(orderQty,1),lt(price,100)))"; this verifies
the OR path in the RQLQuery and any() formatting for the or(...) form.

@robcsegal robcsegal merged commit 46b7f7b into main Feb 18, 2026
4 checks passed
@robcsegal robcsegal deleted the MPT-18174-fix-any-and-all-with-multiple-nested-conditions-for-a-collection branch February 18, 2026 13:23
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