MPT-18065 - Fix backward incompatible issues related to RQL: Support of property comparison

[albertsola]

Closes MPT-18065

• Add RQLProperty and RQLValue wrapper classes in mpt_api_client.rql.query_builder and export them from mpt_api_client.rql
• Support property-to-property comparisons and null() via RQLProperty.null()
• Quote string and date/datetime literals in RQL serialization (e.g., eq(status,active) → eq(status,'active'))
• Broaden query_value_str signature to accept Any and delegate to RQLValue/RQLProperty when appropriate
• Update rql_encode to handle RQLValue/RQLProperty and list operands correctly
• Update and add unit tests to reflect quoting and Property/Value behavior

[coderabbitai]

Actionable comments posted: 4

🤖 Fix all issues with AI agents

In `@mpt_api_client/rql/__init__.py`:
- Line 3: The __all__ export list in mpt_api_client/rql/__init__.py is unsorted
and contains an invalid Ruff noqa directive; update the __all__ declaration to
be alphabetically sorted (i.e., list "Literal" before "RQLQuery") and remove the
invalid "# noqa: WPS410" comment so the file exports the symbols Literal and
RQLQuery correctly and no longer references a non-Ruff rule.

In `@mpt_api_client/rql/query_builder.py`:
- Around line 79-83: Remove the stray blank line in the value-serialization
block so the if isinstance(value, dt.date | dt.datetime): branch and the
subsequent return str(value) are formatted per project style; locate the block
in query_builder.py that checks isinstance(value, dt.date | dt.datetime) (the
value variable) and collapse the extra blank line, then run `ruff format .` to
apply the formatter.
- Around line 9-14: The Literal class lacks type annotations; update it to be
mypy-strict compliant by annotating the attribute and methods: add a class
attribute annotation for value (e.g., value: str), change __init__ signature to
def __init__(self, value: str) -> None and set self.value = value, and annotate
__str__ to def __str__(self) -> str; ensure all uses of Literal.value and its
methods match these types.

In `@tests/unit/rql/query_builder/test_properties_values.py`:
- Around line 22-25: The file has an extra blank line between the tests
test_ne_quoted and test_ne_property causing Ruff to reformat; open the test file
and remove one of the blank lines so there are only two consecutive newlines
between the end of test_ne_quoted and the def line for test_ne_property, or
simply run the formatter (e.g., ruff format .) to apply the standard Ruff
formatting rules and commit the resulting change.

[coderabbitai]

🧹 Nitpick comments (1)

mpt_api_client/rql/query_builder.py (1)

21-21: Remove the invalid # noqa: WPS221.

Ruff flags this as an unknown/unused noqa; the line looks short enough to drop the directive.

🧹 Proposed fix

-QueryValue = str | bool | dt.date | dt.datetime | Numeric | Literal  # noqa: WPS221
+QueryValue = str | bool | dt.date | dt.datetime | Numeric | Literal

As per coding guidelines, "Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections."

[coderabbitai]

Actionable comments posted: 2

🤖 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 137-142: The code path that handles list operators (when op in
constants.LIST) currently builds the list literal using str(el), so string
elements are not quoted consistently; change the branch that returns
",".join(str(el) for el in value) to join the per-element output of
query_value_str (e.g., ",".join(query_value_str(el) for el in value)) so each
element is encoded with the same literal-quoting logic; update the branch
referencing op, constants.LIST, and query_value_str accordingly and ensure it
accepts list|tuple|set values.
- Line 9: The type alias QueryValue in query_builder.py currently has an invalid
inline ignore ("# noqa: WPS221"); remove that trailing comment so the line reads
without any noqa directive. Locate the definition "QueryValue = str | bool |
dt.date | dt.datetime | Numeric" and delete the "# noqa: WPS221" token so Ruff
no longer reports the unknown rule code.

---

Duplicate comments:
In `@mpt_api_client/rql/__init__.py`:
- Around line 1-3: Remove the invalid wemake `# noqa: WPS410` comment in the
module where you export Property, RQLQuery, and Value (the __all__ declaration
in mpt_api_client.rql.__init__.py); either delete the `# noqa: WPS410` suffix
entirely or replace it with a proper Ruff per-file ignore directive for the
corresponding Ruff codes (e.g., add a `# ruff: noqa: RUF100,RUF102` at the top
of the file) so Ruff no longer flags an invalid rule code while keeping the
__all__ export intact.

[jentyk]

I think Value is premature optimization at this stage, it does nothing. The default behavior should assume the value is not a property and just quote it. In case the value is a property, which was uncommon so far, we were not using it, user would use Property.

[albertsola]

Value converts different types of values: datetime, date, str, boolean, to the right string format.
Some values needs to add the quotes other not so we encapsulate that logic in the str function of Value.

Moved this out of query_value_str so we simplify and encapsulate that method.

It helps keeping things clean.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

mpt_api_client/rql/query_builder.py (1)

137-142: Redundant condition check and potential list element quoting inconsistency.

Line 139 is unreachable for its intended purpose: if op in constants.COMP, then op not in constants.LIST is already true, so line 137 would have already returned. The condition at line 139 will only trigger if op is neither in LIST nor a valid operator (which would then fall through to the TypeError).

Additionally, a past review flagged that list elements at line 142 use str(el) instead of query_value_str(el), which means string elements in lists won't be quoted. However, test expectations (e.g., in(asset.id,(first,second))) suggest this is intentional. Please confirm whether list element quoting is deliberately excluded for backward compatibility.

♻️ Proposed simplification (remove redundant condition)

 def rql_encode(op: str, value: Any) -> str:
     ...
     if op not in constants.LIST and isinstance(value, QueryValue | Value | Property):
         return query_value_str(value)
-    if op in constants.COMP and isinstance(value, QueryValue | Value | Property):
-        return query_value_str(value)
     if op in constants.LIST and isinstance(value, list | tuple | set):
         return ",".join(str(el) for el in value)
 
     raise TypeError(f"the `{op}` operator doesn't support the {type(value)} type.")

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/rql/query_builder.py` around lines 137 - 142, Remove the
redundant condition checking "if op in constants.COMP" because the preceding "if
op not in constants.LIST and isinstance(...)" already handles COMP; delete the
second branch that duplicates that logic (referencing
constants.LIST/constants.COMP and the QueryValue/Value/Property checks and
query_value_str). For the list branch that currently uses ",".join(str(el) for
el in value), decide whether list elements should be serialized with
query_value_str(el) (to ensure strings are quoted) or left as str(el) for
backward compatibility; implement the chosen behavior and add a short inline
comment explaining the decision near the list serialization in query_builder
(and adjust tests if switching to query_value_str).

🤖 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 26-27: The __init__ method of the Value class is missing an
explicit return type annotation; update the signature of Value.__init__ to
include "-> None" (i.e., change def __init__(self, value: QueryValue) to def
__init__(self, value: QueryValue) -> None) so it matches mypy strict
requirements and is consistent with the Property class.
- Around line 15-16: The __init__ method of the Property class is missing an
explicit return type for mypy strict mode; update the signature of
Property.__init__ to include the return annotation "-> None" so it reads like a
standard initializer with an explicit None return type (locate the __init__
method inside the Property class in query_builder.py and add the annotation).

---

Nitpick comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 137-142: Remove the redundant condition checking "if op in
constants.COMP" because the preceding "if op not in constants.LIST and
isinstance(...)" already handles COMP; delete the second branch that duplicates
that logic (referencing constants.LIST/constants.COMP and the
QueryValue/Value/Property checks and query_value_str). For the list branch that
currently uses ",".join(str(el) for el in value), decide whether list elements
should be serialized with query_value_str(el) (to ensure strings are quoted) or
left as str(el) for backward compatibility; implement the chosen behavior and
add a short inline comment explaining the decision near the list serialization
in query_builder (and adjust tests if switching to query_value_str).

[coderabbitai]

Actionable comments posted: 1

🤖 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 137-140: The second conditional block checking "if op in
constants.COMP and isinstance(value, QueryValue | Value | Property): return
query_value_str(value)" is dead code because COMP and LIST are disjoint and the
prior "if op not in constants.LIST and isinstance(...)" already returns for all
COMP ops; remove the redundant if block (delete the lines referencing
constants.COMP and the duplicate isinstance check) so the code relies on the
initial check and the call to query_value_str(value) only once, preserving
existing behavior with constants.LIST, constants.COMP, QueryValue, Value,
Property, and query_value_str.

---

Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 15-16: The Property.__init__ constructor is missing an explicit
return type annotation required by strict mypy; update the method signature of
Property.__init__ to include "-> None" (i.e., def __init__(self, value: str) ->
None:) so the constructor is properly typed.
- Around line 26-27: The __init__ method of the Value class is missing an
explicit return type annotation; update the signature of Value.__init__ (the
constructor that takes value: QueryValue) to include the -> None return
annotation to satisfy mypy strict rules, e.g., change def __init__(self, value:
QueryValue): to def __init__(self, value: QueryValue) -> None:, leaving the body
(self.value = value) unchanged.
- Around line 141-142: The list-serialization branch in query_builder (the if
checking "op in constants.LIST" and "isinstance(value, list | tuple | set)")
uses str(el) which leaves string elements unquoted and inconsistent with scalar
handling; change the element formatting to call query_value_str(el) for each
element (i.e., replace str(el) with query_value_str(el) in the join) so list
members are quoted the same way as single values, preserving current isinstance
checks and the op in constants.LIST condition.

[albertsola]

@coderabbitai which issues do I missed to resolve?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@albertsola, let me check which review comments are still unresolved on this PR.

---

🏁 Script executed:

#!/bin/bash

# Get unresolved review comments on PR `#213`
gh pr view 213 --json reviewThreads --jq '.reviewThreads[] | select(.isResolved == false) | {
  path: .path,
  line: .line,
  isOutdated: .isOutdated,
  comments: [.comments[] | {
    author: .author.login,
    body: .body,
    createdAt: .createdAt
  }]
}'

Length of output: 674

---

🏁 Script executed:

#!/bin/bash

# Use GitHub REST API to get review comments with resolved status
gh api graphql -f query='
  query {
    repository(owner: "softwareone-platform", name: "mpt-api-python-client") {
      pullRequest(number: 213) {
        reviewThreads(first: 100) {
          nodes {
            isResolved
            isOutdated
            path
            line
            comments(first: 10) {
              nodes {
                author {
                  login
                }
                body
                createdAt
              }
            }
          }
        }
      }
    }
  }
' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {
  path: .path,
  line: .line,
  isOutdated: .isOutdated,
  comments: [.comments.nodes[] | {
    author: .author.login,
    body: .body
  }]
}'

Length of output: 1082

---

🏁 Script executed:

#!/bin/bash

# Get all review threads and filter for coderabbitai comments
gh api graphql -f query='
  query {
    repository(owner: "softwareone-platform", name: "mpt-api-python-client") {
      pullRequest(number: 213) {
        reviewThreads(first: 100) {
          nodes {
            isResolved
            isOutdated
            path
            line
            comments(first: 10) {
              nodes {
                author {
                  login
                }
                body
                createdAt
              }
            }
          }
        }
      }
    }
  }
' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | select(.comments.nodes[] | .author.login == "coderabbitai[bot]") | {
  path: .path,
  line: .line,
  isOutdated: .isOutdated,
  firstComment: .comments.nodes[0].body
}'

Length of output: 0

---

@albertsola, great news! All of my review comments have been resolved. ✅

However, there are 2 unresolved review threads from other reviewers:

1. Active discussion in mpt_api_client/rql/query_builder.py (line 23):

   • @jentyk raised a question about whether the Value class is premature optimization
   • You provided an explanation defending the design choice
   • This thread is still not resolved and may need @jentyk to confirm they're satisfied with the explanation

2. Outdated thread in mpt_api_client/rql/query_builder.py:

   • @jentyk questioned a use case about operator comparison
   • You acknowledged and confirmed "Fixed now"
   • This thread is marked as outdated (the code has changed since), so it can probably be resolved

Summary: You haven't missed any of my review comments—they're all addressed! The remaining items are from @jentyk's review.

[albertsola]

@coderabbitai if I have not missed anything why are you still requesting changes? should you update to approve the changes?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Check the current review status for PR `#213`
gh pr view 213 --json reviews --jq '.reviews[] | select(.author.login == "coderabbitai[bot]") | {state: .state, submittedAt: .submittedAt}' | tail -5

Length of output: 0

---

🏁 Script executed:

#!/bin/bash

# Check all reviews on PR `#213`
gh pr view 213 --json reviews --jq '.reviews[] | {author: .author.login, state: .state, submittedAt: .submittedAt}'

echo "---"

# Also check the overall PR status
gh pr view 213 --json reviewDecision,reviews --jq '{reviewDecision: .reviewDecision, totalReviews: (.reviews | length)}'

Length of output: 1976

---

@albertsola, you're absolutely right! I confirmed earlier that all my review comments have been resolved, but my review status is still showing "CHANGES_REQUESTED" from my last full review. Since all the issues I raised have been addressed, I'll approve the changes now. ✅

[approve]

[albertsola]

@coderabbitai resolve

[coderabbitai]

✅ Actions performed

Comments resolved and changes approved.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ccdb3f and 92cd854.

📒 Files selected for processing (12)

• mpt_api_client/rql/__init__.py
• mpt_api_client/rql/query_builder.py
• tests/unit/http/mixins/test_collection_mixin.py
• tests/unit/http/test_base_service.py
• tests/unit/http/test_query_state.py
• tests/unit/rql/query_builder/test_create_rql.py
• tests/unit/rql/query_builder/test_multiple_ops.py
• tests/unit/rql/query_builder/test_properties_values.py
• tests/unit/rql/query_builder/test_rql.py
• tests/unit/rql/query_builder/test_rql_dot_path.py
• tests/unit/rql/query_builder/test_rql_in.py
• tests/unit/rql/query_builder/test_rql_parse_kwargs.py

🚧 Files skipped from review as they are similar to previous changes (5)

• tests/unit/rql/query_builder/test_rql_in.py
• tests/unit/http/mixins/test_collection_mixin.py
• tests/unit/http/test_query_state.py
• tests/unit/rql/query_builder/test_properties_values.py
• tests/unit/rql/query_builder/test_create_rql.py

---

📝 Walkthrough

Walkthrough

Exports RQLProperty and RQLValue, adds those wrapper classes to the RQL query builder, widens query_value_str to accept Any, updates rql_encode to handle Property/Value and list elements, and adjusts many tests to expect single-quoted string literals in RQL output and URL queries.

Changes

Cohort / File(s)	Summary
Core RQL library mpt_api_client/rql/__init__.py, mpt_api_client/rql/query_builder.py	Imported and exported RQLProperty and RQLValue in __init__; added RQLProperty and RQLValue classes; changed query_value_str(value: Any) signature and widened encoding logic (rql_encode) to handle RQLProperty/RQLValue and list element encoding.
RQL unit tests — new/expanded tests/unit/rql/query_builder/test_properties_values.py, tests/unit/rql/query_builder/test_rql.py	Added tests for Property/Value usage, null handling, and mixed operand in-clauses; updated imports to include RQLProperty/RQLValue.
RQL unit tests — string/format expectation updates tests/unit/rql/query_builder/test_create_rql.py, tests/unit/rql/query_builder/test_multiple_ops.py, tests/unit/rql/query_builder/test_rql_dot_path.py, tests/unit/rql/query_builder/test_rql_in.py, tests/unit/rql/query_builder/test_rql_parse_kwargs.py	Updated expected RQL string outputs to emit single-quoted string/date/datetime literals and reflect new quoting/representation behavior (including lists/tuples and mixed elements).
HTTP tests — URL query expectations tests/unit/http/mixins/test_collection_mixin.py, tests/unit/http/test_base_service.py, tests/unit/http/test_query_state.py	Updated URL/query-string assertions to expect quoted string values in predicates (e.g., eq(status,'active')).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Jira Issue Key In Title	✅ Passed	The PR title contains exactly one Jira issue key in the required MPT-XXXX format at the beginning.
Test Coverage Required	✅ Passed	The PR modifies 2 code files in mpt_api_client/rql/ with comprehensive test coverage across 10 test files including a new dedicated test file for RQLProperty and RQLValue classes.
Single Commit Required	✅ Passed	The PR contains exactly one commit (92cd854: 'MPT-18065 WIP - Fix backward incompatible issues related to RQL: Support of property comparison').

_{✏️ 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.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/unit/rql/query_builder/test_rql.py (1)

4-4: Remove invalid noqa directive.

Static analysis indicates AAA01 is an unknown rule code. The # noqa: AAA01 comment on this line (and similar ones throughout the file) can be removed.

🔧 Proposed fix

-def test_repr():  # noqa: AAA01
+def test_repr():

🤖 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.py` at line 4, Remove the invalid `#
noqa: AAA01` directive from the test function declaration(s) (e.g., `def
test_repr():`) in tests/unit/rql/query_builder/test_rql.py and any other lines
in the same file that use the unknown `AAA01` rule; simply delete the `# noqa:
AAA01` suffix so the function signatures/readability remain unchanged and static
analysis no longer flags an unknown rule code.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/unit/rql/query_builder/test_properties_values.py`:
- Around line 25-27: Add the missing blank line between the top-level functions
test_compare_property and test_compare_null so there are two blank lines
separating them (PEP8/Ruff requirement); update the spacing in the test file
(around the functions named test_compare_property and test_compare_null) and
re-run ruff format to verify the file is correctly formatted.

---

Nitpick comments:
In `@tests/unit/rql/query_builder/test_rql.py`:
- Line 4: Remove the invalid `# noqa: AAA01` directive from the test function
declaration(s) (e.g., `def test_repr():`) in
tests/unit/rql/query_builder/test_rql.py and any other lines in the same file
that use the unknown `AAA01` rule; simply delete the `# noqa: AAA01` suffix so
the function signatures/readability remain unchanged and static analysis no
longer flags an unknown rule code.

---

ℹ️ Review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a94e68d and 7af0117.

📒 Files selected for processing (12)

• mpt_api_client/rql/__init__.py
• mpt_api_client/rql/query_builder.py
• tests/unit/http/mixins/test_collection_mixin.py
• tests/unit/http/test_base_service.py
• tests/unit/http/test_query_state.py
• tests/unit/rql/query_builder/test_create_rql.py
• tests/unit/rql/query_builder/test_multiple_ops.py
• tests/unit/rql/query_builder/test_properties_values.py
• tests/unit/rql/query_builder/test_rql.py
• tests/unit/rql/query_builder/test_rql_dot_path.py
• tests/unit/rql/query_builder/test_rql_in.py
• tests/unit/rql/query_builder/test_rql_parse_kwargs.py

[coderabbitai]

🧹 Nitpick comments (1)

mpt_api_client/rql/query_builder.py (1)

102-110: Consider simplifying the sequential isinstance checks.

The logic is correct, but the structure with multiple sequential if isinstance checks that all return str(value) could be slightly simplified. However, this is a minor readability preference and the current implementation works correctly.

♻️ Optional simplification

 def query_value_str(value: Any) -> str:
     """Converts a value to string for use in RQL queries."""
     if isinstance(value, QueryValue):
         value = Value(value)
-    if isinstance(value, Value):
-        return str(value)
-    if isinstance(value, Property):
+    if isinstance(value, Value | Property):
         return str(value)
     return str(value)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/rql/query_builder.py` around lines 102 - 110, The function
query_value_str currently uses multiple sequential isinstance checks returning
str(value); simplify by first unwrapping QueryValue (if isinstance(value,
QueryValue) then set value = Value(value)) and then use a single return
str(value) for all other cases (including Value and Property), so update
query_value_str to remove redundant isinstance checks and have one unified
return after optional unwrapping; reference symbols: function query_value_str,
types QueryValue, Value, Property.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 102-110: The function query_value_str currently uses multiple
sequential isinstance checks returning str(value); simplify by first unwrapping
QueryValue (if isinstance(value, QueryValue) then set value = Value(value)) and
then use a single return str(value) for all other cases (including Value and
Property), so update query_value_str to remove redundant isinstance checks and
have one unified return after optional unwrapping; reference symbols: function
query_value_str, types QueryValue, Value, Property.

---

ℹ️ Review info

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7af0117 and 9387846.

📒 Files selected for processing (12)

• mpt_api_client/rql/__init__.py
• mpt_api_client/rql/query_builder.py
• tests/unit/http/mixins/test_collection_mixin.py
• tests/unit/http/test_base_service.py
• tests/unit/http/test_query_state.py
• tests/unit/rql/query_builder/test_create_rql.py
• tests/unit/rql/query_builder/test_multiple_ops.py
• tests/unit/rql/query_builder/test_properties_values.py
• tests/unit/rql/query_builder/test_rql.py
• tests/unit/rql/query_builder/test_rql_dot_path.py
• tests/unit/rql/query_builder/test_rql_in.py
• tests/unit/rql/query_builder/test_rql_parse_kwargs.py

🚧 Files skipped from review as they are similar to previous changes (7)

• tests/unit/rql/query_builder/test_rql_dot_path.py
• tests/unit/http/mixins/test_collection_mixin.py
• tests/unit/rql/query_builder/test_properties_values.py
• tests/unit/rql/query_builder/test_rql_parse_kwargs.py
• tests/unit/http/test_base_service.py
• tests/unit/http/test_query_state.py
• tests/unit/rql/query_builder/test_create_rql.py

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud