Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Box-based wrapper with a model-driven system: adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
albertsola
force-pushed
the
MPT-18701/Resource-incorrect-case-handling
branch
from
44608ee to
ac99f10
Compare
robcsegal
approved these changes
Mar 6, 2026
albertsola
force-pushed
the
MPT-18701/Resource-incorrect-case-handling
branch
2 times, most recently
from
745bf3d to
ed627c4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/models/resource/test_resource.py (1)
143-156:test_overwritting_as_objectdoes not currently exercise an object-assignment path.The action in Line 155 still assigns a dict, so this test overlaps heavily with
test_overwritting. Consider renaming or changing the assignment to an object instance to match intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/resource/test_resource.py` around lines 143 - 156, The test test_overwritting_as_object is claiming to test object-assignment but still assigns a dict to agreement.parameters.ordering[1]; change the assignment to instantiate and assign an actual parameter-item object instead of a dict (e.g., create an instance of the ordering item class used by AgreementDummy or the model class that represents ordering entries) so the code path for object replacement is exercised; update the assignment target agreement.parameters.ordering[1] to use that instantiated object and adjust any assertions if needed to compare object attributes rather than raw dicts.
🤖 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/models/model.py`:
- Around line 177-183: The Model.__init__ currently mutates the caller's
resource_data dict and allows payload keys like "meta" or "resource_data" to
collide with constructor params; to fix, make a new dict copy for merging (e.g.,
data = dict(resource_data) if resource_data else {}) so the caller's object is
not modified, then check kwargs for reserved names ("meta", "resource_data") and
either remove them or raise a clear TypeError to prevent silent argument binding
collisions, finally update the new data dict with kwargs and call
super().__init__(**data); reference the __init__ method, the "meta" and
"resource_data" keys, and the Model constructor when applying this change.
---
Nitpick comments:
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-156: The test test_overwritting_as_object is claiming to test
object-assignment but still assigns a dict to agreement.parameters.ordering[1];
change the assignment to instantiate and assign an actual parameter-item object
instead of a dict (e.g., create an instance of the ordering item class used by
AgreementDummy or the model class that represents ordering entries) so the code
path for object replacement is exercised; update the assignment target
agreement.parameters.ordering[1] to use that instantiated object and adjust any
assertions if needed to compare object attributes rather than raw dicts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1c512842-de4b-4639-b374-fb275dede14a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
albertsola
force-pushed
the
MPT-18701/Resource-incorrect-case-handling
branch
from
ed627c4 to
bdb6d5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mpt_api_client/models/model.py (1)
177-183:⚠️ Potential issue | 🟠 Major
Model.new/Model.__init__still allow key collisions and caller-data mutation.This is still unresolved: payload splatting in
newcan collide with reserved constructor params (meta,resource_data), and__init__can mutate caller-ownedresource_datawhen kwargs are merged.💡 Proposed fix
class Model(BaseModel): @@ def __init__( self, resource_data: ResourceData | None = None, meta: Meta | None = None, **kwargs: Any ) -> None: object.__setattr__(self, "meta", meta) - data = resource_data or {} + data = dict(resource_data or {}) data.update(kwargs) super().__init__(**data) @@ def new(cls, resource_data: ResourceData | None = None, meta: Meta | None = None) -> Self: """Creates a new resource from ResourceData and Meta.""" - return cls(meta=meta, **(resource_data or {})) + return cls(resource_data=resource_data, meta=meta)As per coding guidelines, “For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved.”
Also applies to: 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 177 - 183, Model.new and Model.__init__ currently allow payload splatting to collide with reserved params (meta, resource_data) and mutate caller-owned resource_data when merging kwargs; fix by defensively copying and validating inputs: in Model.__init__ (and in Model.new) make a shallow copy of the incoming resource_data (use a new dict) before merging, explicitly disallow or raise if kwargs contain reserved keys ("meta" or "resource_data") to prevent collisions, and perform the merge into the new copy rather than mutating the caller's dict; update Model.new to filter/validate its payload before forwarding to Model.__init__ (or call __init__ with explicit args) so reserved params cannot be overwritten.
🤖 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/models/model.py`:
- Around line 140-164: The _process_value method in BaseModel is too complex;
extract the dict and list branches into small helpers: implement a
_process_dict_value(self, value: dict, target_class: Any) -> Any that contains
the dict-to-BaseModel logic (use target_class if issubclass(BaseModel) else
BaseModel), a _resolve_list_model_class(self, target_class: Any) ->
Type[BaseModel] that encapsulates get_origin/get_args logic to pick the element
model, and a _process_list_value(self, value: Iterable, target_class: Any) ->
ModelList that calls _resolve_list_model_class and returns ModelList(value,
model_class=...), then replace the dict/list blocks in _process_value with calls
to these helpers so functionality (including checks for BaseModel, ModelList,
UserList) remains identical but cognitive complexity is reduced.
---
Duplicate comments:
In `@mpt_api_client/models/model.py`:
- Around line 177-183: Model.new and Model.__init__ currently allow payload
splatting to collide with reserved params (meta, resource_data) and mutate
caller-owned resource_data when merging kwargs; fix by defensively copying and
validating inputs: in Model.__init__ (and in Model.new) make a shallow copy of
the incoming resource_data (use a new dict) before merging, explicitly disallow
or raise if kwargs contain reserved keys ("meta" or "resource_data") to prevent
collisions, and perform the merge into the new copy rather than mutating the
caller's dict; update Model.new to filter/validate its payload before forwarding
to Model.__init__ (or call __init__ with explicit args) so reserved params
cannot be overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9a6ca653-f8b8-44db-997d-e8434c3c2d81
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mpt_api_client/models/model.py (1)
140-164:⚠️ Potential issue | 🟠 Major
_process_valuestill exceeds the cognitive complexity gate.Line 140 keeps the same high-branching structure; Sonar still reports this as above threshold. Please split dict/list/type-resolution into helpers and keep
_process_valueas a dispatcher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 140 - 164, Refactor _process_value by moving the dict and list handling into small helpers: create _process_dict_value(value, target_class) to encapsulate the dict->BaseModel logic (including the issubclass check and returning target_class(**value) or BaseModel(**value), and create _resolve_list_model_class(target_class) to return the element model_class (default BaseModel) by inspecting get_origin/get_args, and _process_list_value(value, target_class) to call that resolver and return ModelList(value, model_class=model_class); then make _process_value simply dispatch to these helpers (dict -> _process_dict_value, list/UserList -> _process_list_value, else return value) so branching is isolated into the new functions while preserving existing behavior and using the same symbols: _process_value, _process_dict_value, _process_list_value, _resolve_list_model_class, BaseModel, ModelList.
🧹 Nitpick comments (1)
tests/unit/models/resource/test_resource.py (1)
143-160:test_overwritting_as_objectdoes not currently test object-style mutation.Line 155 still performs dict assignment (same path as the prior test), so the object-mutation path remains untested. Consider mutating
agreement.parameters.ordering[1].value(or similar) and renaming the typo’doverwrittingtests.💡 Suggested adjustment
-def test_overwritting_as_object(): +def test_overwriting_as_object(): @@ - agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act + agreement.parameters.ordering[1].value = "Evergreen Terrace" # act + agreement_data["parameters"]["ordering"][1]["value"] = "Evergreen Terrace" @@ - assert agreement.parameters.ordering[1].external_id == "address" + assert agreement.parameters.ordering[1].external_id == "address" + assert agreement.parameters.ordering[1].value == "Evergreen Terrace"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/resource/test_resource.py` around lines 143 - 160, Rename the test and change the mutation to exercise the object-style path: in test_overwritting_as_object (or better test_overwriting_as_object) instead of assigning a dict to agreement.parameters.ordering[1], mutate the attribute on the item object (e.g., agreement.parameters.ordering[1].value = "Springfield" or change .external_id/.value as appropriate) so you exercise the object-mutation code path on AgreementDummy and its nested list; keep assertions for agreement.id and the ordering items' attributes (use .external_id/.value accessors) and ensure agreement.to_dict() still equals the original agreement_data after the object mutation.
🤖 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/models/model.py`:
- Around line 14-24: The acronym regex _SNAKE_CASE_ACRONYM should not use an
atomic group because it blocks backtracking and breaks conversions like
"IPAddress" and "HTTPServerError"; replace its definition with a plain capturing
pattern such as re.compile(r"([A-Z]+)([A-Z][a-z0-9])") so to_snake_case's
subsequent substitution _SNAKE_CASE_ACRONYM.sub(r"\1_\2", snake) correctly
inserts underscores between acronym runs and the following capitalized word (no
other changes to to_snake_case or the substitution logic required).
---
Duplicate comments:
In `@mpt_api_client/models/model.py`:
- Around line 140-164: Refactor _process_value by moving the dict and list
handling into small helpers: create _process_dict_value(value, target_class) to
encapsulate the dict->BaseModel logic (including the issubclass check and
returning target_class(**value) or BaseModel(**value), and create
_resolve_list_model_class(target_class) to return the element model_class
(default BaseModel) by inspecting get_origin/get_args, and
_process_list_value(value, target_class) to call that resolver and return
ModelList(value, model_class=model_class); then make _process_value simply
dispatch to these helpers (dict -> _process_dict_value, list/UserList ->
_process_list_value, else return value) so branching is isolated into the new
functions while preserving existing behavior and using the same symbols:
_process_value, _process_dict_value, _process_list_value,
_resolve_list_model_class, BaseModel, ModelList.
---
Nitpick comments:
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-160: Rename the test and change the mutation to exercise the
object-style path: in test_overwritting_as_object (or better
test_overwriting_as_object) instead of assigning a dict to
agreement.parameters.ordering[1], mutate the attribute on the item object (e.g.,
agreement.parameters.ordering[1].value = "Springfield" or change
.external_id/.value as appropriate) so you exercise the object-mutation code
path on AgreementDummy and its nested list; keep assertions for agreement.id and
the ordering items' attributes (use .external_id/.value accessors) and ensure
agreement.to_dict() still equals the original agreement_data after the object
mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1a05f83d-5ca8-44af-a145-16e814bd0efe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
albertsola
force-pushed
the
MPT-18701/Resource-incorrect-case-handling
branch
from
bdb6d5e to
841cf34
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_api_client/models/model.py (1)
205-207:⚠️ Potential issue | 🟠 MajorGuard
_data_keyextraction before calling.get.Line 206 assumes a dict when
_data_keyis set. If the payload is non-dict, this raisesAttributeErrorinstead of the intendedTypeError.Proposed fix
if isinstance(response_data, dict): response_data.pop("$meta", None) if cls._data_key: - response_data = response_data.get(cls._data_key) + if not isinstance(response_data, dict): + raise TypeError("Response data must be a dict.") + response_data = response_data.get(cls._data_key) if not isinstance(response_data, dict): raise TypeError("Response data must be a dict.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 205 - 207, The code calls response_data.get(cls._data_key) when cls._data_key is set, which will raise AttributeError if response_data is not a dict; update the block in the class method that handles response_data so you first check isinstance(response_data, dict) before attempting response_data.get(...), and if it's not a dict raise the intended TypeError (or handle None appropriately) with a clear message; change the logic around the symbols cls._data_key and response_data to perform the type guard prior to extraction and preserve existing behavior when the key is missing.
🤖 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/models/model.py`:
- Line 5: The import of typing.override in mpt_api_client/models/model.py will
fail on Python 3.12; replace it with a conditional/fallback import so older
Pythons use typing_extensions.override (or import override from typing when
available) — update the import line that currently references "override"
alongside "Any, ClassVar, Self, get_args, get_origin" to try importing override
from typing_extensions first (or fall back to typing for 3.13+), and ensure
packaging includes typing-extensions if you choose the stable dependency route.
- Around line 69-70: The BaseModel class is annotated with `@dataclass` which
auto-generates __eq__ for zero fields causing all BaseModel instances to compare
equal; update the class declaration to disable dataclass equality (e.g., change
`@dataclass` to `@dataclass`(eq=False)) or remove the `@dataclass` decorator entirely,
ensuring existing dynamic attribute handling via object.__setattr__ remains
unchanged; adjust any imports/usages if you remove the decorator and run tests
focusing on BaseModel, _process_value, and places that instantiate BaseModel to
confirm equality semantics are fixed.
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-160: The test test_overwritting_as_object is supposed to
exercise replacing an ordering element with an object but currently assigns a
dict (duplicating test_overwritting); change the assignment to instantiate the
same element class as the existing item instead of a plain dict. For example,
replace the line agreement.parameters.ordering[1] = {"externalId": "address",
"value": "Springfield"} with something that constructs the same type as
agreement.parameters.ordering[1] (e.g. agreement.parameters.ordering[1] =
type(agreement.parameters.ordering[1])("address", "Springfield") or use the
appropriate constructor/class for parameter items) so the test uses object
reassignment while keeping the rest of assertions and AgreementDummy usage
intact.
---
Outside diff comments:
In `@mpt_api_client/models/model.py`:
- Around line 205-207: The code calls response_data.get(cls._data_key) when
cls._data_key is set, which will raise AttributeError if response_data is not a
dict; update the block in the class method that handles response_data so you
first check isinstance(response_data, dict) before attempting
response_data.get(...), and if it's not a dict raise the intended TypeError (or
handle None appropriately) with a clear message; change the logic around the
symbols cls._data_key and response_data to perform the type guard prior to
extraction and preserve existing behavior when the key is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 95a8615a-c598-4098-81be-bfb97fb07713
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
| from collections import UserList | ||
| from collections.abc import Iterable | ||
| from dataclasses import dataclass | ||
| from typing import Any, ClassVar, Self, get_args, get_origin, override |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get Python version requirements from pyproject.toml
echo "=== Python version requirements ==="
fd pyproject.toml --exec sed -n '1,240p' {}
echo -e "\n=== Override imports in codebase ==="
rg -n 'from typing import.*override|from typing_extensions import override' --type=py -A 2
echo -e "\n=== Python version and typing-extensions references ==="
rg -n 'requires-python|python_version|classifiers|typing-extensions|typing_extensions' pyproject.toml -C2Repository: softwareone-platform/mpt-api-python-client
Length of output: 8483
Import of typing.override fails on Python 3.12, which is within declared support range.
The project declares requires-python = ">=3.12,<4", but typing.override was only added in Python 3.13. This import will fail at module load time on Python 3.12.
Fix by either:
- Using a conditional import:
from typing_extensions import override(or fallback from typing for Python 3.13+), or - Adding
typing-extensionsto production dependencies (currently only in dev), or - Updating
requires-pythonto>=3.13,<4
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpt_api_client/models/model.py` at line 5, The import of typing.override in
mpt_api_client/models/model.py will fail on Python 3.12; replace it with a
conditional/fallback import so older Pythons use typing_extensions.override (or
import override from typing when available) — update the import line that
currently references "override" alongside "Any, ClassVar, Self, get_args,
get_origin" to try importing override from typing_extensions first (or fall back
to typing for 3.13+), and ensure packaging includes typing-extensions if you
choose the stable dependency route.
Comment on lines
+69
to
+70
| @dataclass | ||
| class BaseModel: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect BaseModel declaration and whether __eq__ is overridden.
rg -n '^\s*@dataclass|^\s*class BaseModel|^\s*def __eq__' mpt_api_client/models/model.py -C2
# Demonstrate Python dataclass behavior for a no-field base class.
python - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Base:
pass
class Child(Base):
def __init__(self, value):
self.value = value
print("Child('x') == Child('y'):", Child("x") == Child("y"))
PYRepository: softwareone-platform/mpt-api-python-client
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the full BaseModel class definition
sed -n '69,150p' mpt_api_client/models/model.py
# Check if there are any subclasses and how they use BaseModel
rg 'class.*\(BaseModel\)' mpt_api_client/models/model.py -A10 | head -40Repository: softwareone-platform/mpt-api-python-client
Length of output: 3686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for direct instantiation of BaseModel
rg 'BaseModel\(' mpt_api_client/models/model.py -B2 -A2
# Check all files that import or use BaseModel
rg 'BaseModel' mpt_api_client/ -l | head -10Repository: softwareone-platform/mpt-api-python-client
Length of output: 538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the class declaration area including any potential fields
sed -n '69,85p' mpt_api_client/models/model.py
# Check what __annotations__ are on BaseModel
rg '__annotations__' mpt_api_client/models/model.py -B5 -A2Repository: softwareone-platform/mpt-api-python-client
Length of output: 1417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for equality comparisons with BaseModel or Model instances
rg '==|!=' mpt_api_client/ -B2 -A2 | grep -E '(BaseModel|Model|self.*==|instance.*==)' -i | head -20
# Also check tests
find . -name "*test*.py" -type f | head -5Repository: softwareone-platform/mpt-api-python-client
Length of output: 1706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get Model class definition to see if it has fields
sed -n '140,180p' mpt_api_client/models/model.py
# Check if there are any test files that test equality
find . -name "test_*.py" -o -name "*_test.py" | xargs grep -l "==" | head -3Repository: softwareone-platform/mpt-api-python-client
Length of output: 2049
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Search for tests that might compare Model or BaseModel instances
find . -path ./node_modules -prune -o -name "test_*.py" -print | xargs grep -l "BaseModel\|Model" 2>/dev/null | head -3 | xargs grep -E "==.*Model|Model.*==" 2>/dev/null | head -20
# Also check if there's documentation about expected behavior
find . -name "*.md" -o -name "*.rst" | xargs grep -i "equality\|equal" 2>/dev/null | head -10Repository: softwareone-platform/mpt-api-python-client
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for tests that might compare Model or BaseModel instances
find . -path ./node_modules -prune -o -name "test_*.py" -print | head -5 | xargs grep -E "==.*Model|Model.*==" 2>/dev/null | head -20
# Check if there's equality testing in the actual test files
rg 'assert.*==|self\.assertEqual' tests/ --type py | head -20Repository: softwareone-platform/mpt-api-python-client
Length of output: 70
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for direct instantiation of BaseModel and usage patterns
rg 'BaseModel\(' mpt_api_client/ -B3 -A3
# Check if __eq__ is explicitly implemented somewhere in BaseModel
rg '__eq__' mpt_api_client/models/model.py -B2 -A5Repository: softwareone-platform/mpt-api-python-client
Length of output: 627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's test the actual behavior with the code structure we found
python3 << 'PYTEST'
from dataclasses import dataclass
from typing import Any
# Simulate BaseModel structure
`@dataclass`
class BaseModelTest:
"""Simulate the actual BaseModel"""
def __init__(self, **kwargs: Any) -> None:
for key, value in kwargs.items():
object.__setattr__(self, key, value)
# Create instances with different data
b1 = BaseModelTest(a=1, b=2)
b2 = BaseModelTest(x=10, y=20)
b3 = BaseModelTest(a=1, b=2)
print("b1.__dict__:", b1.__dict__)
print("b2.__dict__:", b2.__dict__)
print("b3.__dict__:", b3.__dict__)
print("\nb1 == b2 (different data):", b1 == b2)
print("b1 == b3 (same data):", b1 == b3)
print("\nThis shows both compare True because `@dataclass` with no fields ignores __dict__")
PYTESTRepository: softwareone-platform/mpt-api-python-client
Length of output: 318
Remove automatic equality generation from @dataclass on BaseModel.
BaseModel has no dataclass fields and uses dynamic attribute management via object.__setattr__. The generated __eq__ compares zero fields, causing all instances to be equal regardless of their data. Since BaseModel is instantiated directly in _process_value(), this breaks equality semantics throughout the hierarchy.
Proposed fix
-@dataclass
+@dataclass(eq=False)
class BaseModel:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @dataclass | |
| class BaseModel: | |
| `@dataclass`(eq=False) | |
| class BaseModel: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpt_api_client/models/model.py` around lines 69 - 70, The BaseModel class is
annotated with `@dataclass` which auto-generates __eq__ for zero fields causing
all BaseModel instances to compare equal; update the class declaration to
disable dataclass equality (e.g., change `@dataclass` to `@dataclass`(eq=False)) or
remove the `@dataclass` decorator entirely, ensuring existing dynamic attribute
handling via object.__setattr__ remains unchanged; adjust any imports/usages if
you remove the decorator and run tests focusing on BaseModel, _process_value,
and places that instantiate BaseModel to confirm equality semantics are fixed.
Comment on lines
+143
to
+160
| def test_overwritting_as_object(): | ||
| agreement_data = { | ||
| "id": "AGR-123", | ||
| "parameters": { | ||
| "ordering": [ | ||
| {"externalId": "contact", "value": "Hommer Simpson"}, | ||
| {"externalId": "address", "value": "Springfield"}, | ||
| ] | ||
| }, | ||
| } | ||
| agreement = AgreementDummy(agreement_data) | ||
|
|
||
| agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act | ||
|
|
||
| assert agreement.id == "AGR-123" | ||
| assert agreement.parameters.ordering[0].external_id == "contact" | ||
| assert agreement.parameters.ordering[1].external_id == "address" | ||
| assert agreement.to_dict() == agreement_data |
There was a problem hiding this comment.
test_overwritting_as_object currently duplicates dict-path coverage.
This test name suggests object reassignment, but Line 155 still assigns a dict, so it overlaps with test_overwritting and misses the intended branch.
Proposed test adjustment
def test_overwritting_as_object():
@@
- agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act
+ replacement = agreement.parameters.ordering[1]
+ agreement.parameters.ordering[1] = replacement # act: object-path reassignment
@@
assert agreement.parameters.ordering[1].external_id == "address"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/models/resource/test_resource.py` around lines 143 - 160, The test
test_overwritting_as_object is supposed to exercise replacing an ordering
element with an object but currently assigns a dict (duplicating
test_overwritting); change the assignment to instantiate the same element class
as the existing item instead of a plain dict. For example, replace the line
agreement.parameters.ordering[1] = {"externalId": "address", "value":
"Springfield"} with something that constructs the same type as
agreement.parameters.ordering[1] (e.g. agreement.parameters.ordering[1] =
type(agreement.parameters.ordering[1])("address", "Springfield") or use the
appropriate constructor/class for parameter items) so the test uses object
reassignment while keeping the rest of assertions and AgreementDummy usage
intact.
Replaced box for dataclasses
albertsola
force-pushed
the
MPT-18701/Resource-incorrect-case-handling
branch
from
841cf34 to
c53c5b4
Compare
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/unit/models/resource/test_resource.py (1)
143-160:⚠️ Potential issue | 🟡 Minor
test_overwritting_as_objectstill does not test object reassignment.Line 155 assigns a dict again, so this test duplicates
test_overwrittinginstead of exercising the object-path mutation it is named for.💡 Proposed fix
- agreement.parameters.ordering[1] = {"externalId": "address", "value": "Springfield"} # act + replacement = agreement.parameters.ordering[1] + agreement.parameters.ordering[1] = replacement # actAs per coding guidelines:
For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/resource/test_resource.py` around lines 143 - 160, The test test_overwritting_as_object currently reassigns agreement.parameters.ordering[1] to a dict (duplicating test_overwritting); update it to reassign with an actual object instance that mimics the resource wrapper (e.g., construct the same type used for items in ordering or a simple object with attributes external_id and value) so the object-path mutation is exercised; keep assertions (agreement.id, agreement.parameters.ordering[0].external_id, agreement.parameters.ordering[1].external_id and agreement.to_dict()) the same to verify the object assignment round-trips, and use AgreementDummy / agreement.parameters.ordering to locate the change.mpt_api_client/models/model.py (1)
69-70:⚠️ Potential issue | 🟠 MajorDisable dataclass-generated equality on
BaseModel.At Line 69,
@dataclassauto-generates__eq__; with dynamic attributes and no dataclass fields, equality can become incorrect for model instances. Use@dataclass(eq=False)(or remove@dataclass) to avoid false-equal comparisons.💡 Proposed fix
-@dataclass +@dataclass(eq=False) class BaseModel:#!/bin/bash set -euo pipefail # Verify current declaration in repository rg -n '^\s*@dataclass|^\s*class\s+BaseModel\b' mpt_api_client/models/model.py -C1 # Verify Python dataclass equality behavior (standalone, no repo code execution) python - <<'PY' from dataclasses import dataclass `@dataclass` class Base: pass class Child(Base): def __init__(self, **kwargs): self.__dict__.update(kwargs) print("Child(a=1) == Child(b=2):", Child(a=1) == Child(b=2)) PYAs per coding guidelines:
For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/models/model.py` around lines 69 - 70, The BaseModel is decorated with `@dataclass` which generates an automatic __eq__ that can produce incorrect comparisons for instances with dynamic attributes; change the decorator to `@dataclass`(eq=False) (or remove the dataclass decorator) on the BaseModel class declaration so dataclass-generated equality is disabled for BaseModel (locate the class by the symbol BaseModel and the decorator `@dataclass` and update it to `@dataclass`(eq=False) to prevent false-equal comparisons).
🤖 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/models/model.py`:
- Around line 69-70: The BaseModel is decorated with `@dataclass` which generates
an automatic __eq__ that can produce incorrect comparisons for instances with
dynamic attributes; change the decorator to `@dataclass`(eq=False) (or remove the
dataclass decorator) on the BaseModel class declaration so dataclass-generated
equality is disabled for BaseModel (locate the class by the symbol BaseModel and
the decorator `@dataclass` and update it to `@dataclass`(eq=False) to prevent
false-equal comparisons).
In `@tests/unit/models/resource/test_resource.py`:
- Around line 143-160: The test test_overwritting_as_object currently reassigns
agreement.parameters.ordering[1] to a dict (duplicating test_overwritting);
update it to reassign with an actual object instance that mimics the resource
wrapper (e.g., construct the same type used for items in ordering or a simple
object with attributes external_id and value) so the object-path mutation is
exercised; keep assertions (agreement.id,
agreement.parameters.ordering[0].external_id,
agreement.parameters.ordering[1].external_id and agreement.to_dict()) the same
to verify the object assignment round-trips, and use AgreementDummy /
agreement.parameters.ordering to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0275b50b-591e-4b6b-a96c-a70e300c0b0e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_api_client/models/model.pypyproject.tomltests/unit/models/resource/test_resource.pytests/unit/models/resource/test_resource_custom_key.py
💤 Files with no reviewable changes (1)
- pyproject.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MPT-18701