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

[albertsola]

Closes MPT-18065

• Add Property and Value wrapper classes to RQL construction:
  • Property marks model property references to be rendered unquoted in RQL.
  • Value marks literal values and formats them for RQL (strings and dates/datetimes quoted; booleans as true/false; numerics as-is).
• Update encoding and conversion logic:
  • query_value_str now accepts Any and rql_encode handles QueryValue | Value | Property, auto-wrapping plain QueryValue into Value where appropriate.
  • Existing list/tuple/set handling in rql_encode is preserved.
• Public API update: export Property and Value from mpt_api_client.rql.
• Tests updated and added:
  • New unit tests for property-based comparisons (property vs. property and property vs. literal).
  • Multiple existing tests updated to expect quoted string/date literals in generated RQL (e.g., eq(field,'value')).
• Compatibility note: RQL string serialization now quotes string/date literals; clients or integrations that assumed unquoted literals must update expectations.

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

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

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