BUG: issues with encoding and null field updates #60
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThree repository update methods were changed to include None-valued fields in their serialized update payloads. InfinityEncoder gained an init, its default method parameter was renamed, and a Flight private post-process call was replaced with an attribute access (no invocation). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Repo as Repository
participant Model as PydanticModel
Client->>Repo: update_*_by_id(id, payload_model)
Repo->>Model: payload_model.model_dump(exclude_none=False)
note right of Repo #E8F0FF: now includes fields with None
Repo->>Repo: update_by_id(id, serialized_payload)
Repo-->>Client: update result
sequenceDiagram
autonumber
participant Caller
participant Enc as InfinityEncoder
participant Flight
participant Parent as JSONEncoder
Caller->>Enc: encode(obj: Flight)
Enc->>Flight: access obj._Flight__evaluate_post_process
note right of Enc #E8F0FF: attribute read only — no call
alt handles Flight
Enc->>Flight: read fields (including None)
Enc-->>Caller: serialized JSON
else fallback
Enc->>Parent: parent.default(obj)
Parent-->>Enc: fallback result
Enc-->>Caller: serialized JSON
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/repositories/motor.py (1)
29-32: Same concern as in flight.py: verify None-value handling.This change mirrors the one in
src/repositories/flight.py. Ensure that including None-valued fields in the update payload aligns with the intended update semantics and doesn't cause unintended data overwrites.src/repositories/rocket.py (1)
29-32: Same concern as in flight.py: verify None-value handling.This change mirrors the one in
src/repositories/flight.py. Ensure that including None-valued fields in the update payload aligns with the intended update semantics and doesn't cause unintended data overwrites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/repositories/flight.py(1 hunks)src/repositories/motor.py(1 hunks)src/repositories/rocket.py(1 hunks)src/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/repositories/rocket.py (6)
src/models/rocket.py (3)
UPDATED(41-42)RocketModel(13-63)DELETED(45-46)src/routes/rocket.py (1)
update_rocket(56-68)tests/unit/test_routes/test_rockets_route.py (1)
test_update_rocket(357-365)src/controllers/flight.py (1)
update_rocket_by_flight_id(47-66)src/routes/flight.py (1)
update_flight_rocket(140-155)tests/unit/test_routes/test_flights_route.py (1)
test_update_rocket_by_flight_id(190-199)
src/repositories/flight.py (3)
src/routes/flight.py (1)
update_flight(58-70)tests/unit/test_routes/test_flights_route.py (5)
test_update_flight_invalid_input(212-216)test_update_environment_by_flight_id(176-187)test_update_flight_by_id(166-173)test_update_rocket_by_flight_id(190-199)test_update_flight_not_found(219-225)src/controllers/flight.py (2)
update_rocket_by_flight_id(47-66)update_environment_by_flight_id(25-44)
src/repositories/motor.py (3)
src/routes/motor.py (1)
update_motor(56-68)src/views/motor.py (2)
MotorView(76-77)MotorSimulation(7-73)tests/unit/test_routes/test_motors_route.py (3)
test_update_motor(291-298)test_update_motor_not_found(308-316)test_update_motor_invalid_input(301-305)
src/utils.py (1)
src/views/flight.py (1)
FlightSimulation(9-140)
🪛 GitHub Actions: Pylint
src/utils.py
[error] 78-78: pylint: C0303 trailing whitespace detected.
[error] 83-83: pylint: C0303 trailing whitespace detected.
[error] 89-89: pylint: C0303 trailing whitespace detected.
[error] 92-92: pylint: C0303 trailing whitespace detected.
[error] 95-95: pylint: C0303 trailing whitespace detected.
[error] 98-98: pylint: W0612 Unused variable 'e' (unused-variable).
🪛 Ruff (0.13.3)
src/utils.py
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
94-94: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
98-98: Do not catch blind exception: Exception
(BLE001)
98-98: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
| async def update_flight_by_id(self, flight_id: str, flight: FlightModel): | ||
| await self.update_by_id( | ||
| flight.model_dump(exclude_none=True), data_id=flight_id | ||
| flight.model_dump(exclude_none=False), data_id=flight_id | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that including None-valued fields doesn't cause unintended data overwrites.
Changing exclude_none=False means None-valued fields are now included in update payloads. If the intent is partial updates (only updating provided fields), this could overwrite existing non-null database values with None.
For example, if a client sends FlightModel(environment=some_env, rocket=None), the rocket field would be set to None in the database even if a rocket was previously associated.
Confirm that one of the following is true:
- The update operation is meant to be a full replacement (all fields should be updated, including None values)
- The underlying
update_by_idimplementation filters out None values before persisting - The API contract requires clients to always send complete objects
Run the following script to examine the update_by_id implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check how update_by_id handles None values in the payload
# Find the interface implementation
ast-grep --pattern $'class RepositoryInterface:
$$$
async def update_by_id($$$):
$$$'
# Also check for any None-value filtering logic
rg -nP --type=py -A10 'def update_by_id' | rg -A10 'None|null'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for definitions of update_by_id
rg -n "async def update_by_id" -C5 --type=py
rg -n "def update_by_id" -C5 --type=py
# Search all usages to find its implementation context
rg -n "update_by_id" -C5 --type=pyLength of output: 7605
Filter out None-valued fields in the update payload
update_by_id applies {"$set": data} directly, so any keys with None from model_dump(exclude_none=False) will overwrite existing fields with null. Use model_dump(exclude_none=True) or manually remove None entries for true partial updates.
🤖 Prompt for AI Agents
In src/repositories/flight.py around lines 29 to 32, the update payload uses
flight.model_dump(exclude_none=False) which includes None values and will
overwrite existing fields with null; change to
flight.model_dump(exclude_none=True) or filter the dict to remove keys with None
before calling update_by_id so only present fields are $set.
There was a problem hiding this comment.
@aasitvora99 why is this necessary? I would prefer we drop the null fields unless their null state is actually used by the lib
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Pull Request Overview
This PR addresses encoding issues and null field update problems in the flight simulation system. The main issue was that the _Flight__evaluate_post_process() method was corrupted as a numpy array instead of a callable method, causing encoding failures.
- Added robust error handling and corruption detection for the
_Flight__evaluate_post_processmethod in the JSON encoder - Changed repository update methods to include null fields instead of excluding them
- Implemented fallback mechanisms for encoding errors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/utils.py | Added comprehensive error handling and corruption detection for Flight object encoding |
| src/repositories/rocket.py | Changed model dump to include null fields during updates |
| src/repositories/motor.py | Changed model dump to include null fields during updates |
| src/repositories/flight.py | Changed model dump to include null fields during updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/utils.py
Outdated
| try: | ||
| delattr(obj, '_Flight__evaluate_post_process') | ||
|
|
||
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) |
There was a problem hiding this comment.
After deleting the corrupted attribute, immediately accessing it again may not restore the original method. The cached_property mechanism might not automatically recreate the method. Consider using the class definition to access the original property descriptor.
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) | |
| # Access the property via the class descriptor to restore it correctly | |
| restored_method = type(obj)._Flight__evaluate_post_process.__get__(obj, type(obj)) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Wdyt @aasitvora99 ?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/utils.py
Outdated
| delattr(obj, '_Flight__evaluate_post_process') | ||
|
|
||
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) | ||
| if restored_method and callable(restored_method): |
There was a problem hiding this comment.
Arguably this is a tautology.
If it is callable it will always pass the first check.
Maybe just reduce it to if callable(obj)
src/utils.py
Outdated
| except Exception as fix_error: | ||
| logger.error(f"Error fixing _Flight__evaluate_post_process: {fix_error}") | ||
|
|
||
| elif evaluate_post_process is not None and callable(evaluate_post_process): |
There was a problem hiding this comment.
Same here.
Only callable check is enough
src/utils.py
Outdated
| try: | ||
| delattr(obj, '_Flight__evaluate_post_process') | ||
|
|
||
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) |
There was a problem hiding this comment.
Wdyt @aasitvora99 ?
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils.py (2)
src/views/flight.py (1)
FlightSimulation(9-140)src/controllers/flight.py (1)
FlightController(12-108)
🪛 GitHub Actions: Pylint
src/utils.py
[error] 78-78: E0001: Parsing failed: 'expected an indented block after 'try' statement on line 77 (src.utils, line 78)'
🪛 Ruff (0.13.3)
src/utils.py
77-78: Expected an indented block after try statement
(invalid-syntax)
78-78: Expected except or finally after try block
(invalid-syntax)
79-79: Unexpected indentation
(invalid-syntax)
82-82: Unexpected indentation
(invalid-syntax)
89-89: Expected except or finally after try block
(invalid-syntax)
89-89: Expected an identifier, but found a keyword 'except' that cannot be used here
(invalid-syntax)
89-89: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
89-89: Expected a statement
(invalid-syntax)
89-90: Expected an expression
(invalid-syntax)
91-91: Unexpected indentation
(invalid-syntax)
91-91: Expected a statement
(invalid-syntax)
91-91: Expected a statement
(invalid-syntax)
91-92: Expected an expression
(invalid-syntax)
92-92: Unexpected indentation
(invalid-syntax)
94-94: unindent does not match any outer indentation level
(invalid-syntax)
94-94: Invalid annotated assignment target
(invalid-syntax)
94-95: Expected an expression
(invalid-syntax)
95-95: Unexpected indentation
(invalid-syntax)
97-97: unindent does not match any outer indentation level
(invalid-syntax)
97-97: Expected a statement
(invalid-syntax)
97-97: Expected a statement
(invalid-syntax)
97-98: Expected an expression
(invalid-syntax)
99-99: Unexpected indentation
(invalid-syntax)
100-100: unindent does not match any outer indentation level
(invalid-syntax)
102-102: unindent does not match any outer indentation level
(invalid-syntax)
102-102: Expected a statement
(invalid-syntax)
102-102: Only single target (not tuple) can be annotated
(invalid-syntax)
102-103: Expected an expression
(invalid-syntax)
103-103: Unexpected indentation
(invalid-syntax)
🔇 Additional comments (1)
src/utils.py (1)
100-104: Verify the indentation and control flow for solution handling.After fixing the indentation issues, ensure that lines 100-103 (the try-except block for solution retrieval) are properly indented within the
if isinstance(obj, Flight):block. The early return on line 103 should prevent reaching line 104 when the exception occurs, but the current broken indentation makes this unclear.As noted in previous comments, if the fallback on line 103 executes,
solutionwill be undefined. However, the earlyreturnshould prevent reaching line 104. Confirm the intended control flow once indentation is corrected.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 44a2ebc.
…/Infinity-API into bug/simulate-forward-fixes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils.py (1)
57-58: Remove unnecessary__init__override.The
__init__method only delegates to the parent class without adding any custom initialization. Python automatically calls the parent's__init__when not overridden, making this override redundant.Apply this diff to remove the unnecessary override:
- def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - def default(self, obj):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pylint
src/utils.py
[error] 60-60: Pylint: W0237 - Parameter 'o' has been renamed to 'obj' in overriding InfinityEncoder.default method (arguments-renamed).
[error] 78-78: Pylint: W0104 - Statement seems to have no effect (pointless-statement).
🪛 Ruff (0.13.3)
src/utils.py
78-78: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (2)
src/utils.py (2)
60-60: Parameter rename improves clarity.Renaming the parameter from
otoobjis more descriptive and consistent with the method body. The Pylint warning (W0237) about parameter renaming in an overriding method is expected and acceptable here.
78-78: Clarify intent of private attribute access on Flight
The expressionobj._Flight__evaluate_post_processmerely reads a name-mangled member without calling or using it. Confirm whether this is meant to trigger a cached evaluation (and exists on the externalFlightclass), document that side effect, handle corruption, or remove it:
- Document as
# Access cached_property to trigger evaluation before serialization- Or detect/log corruption: check if it’s an
np.ndarrayand warn- Or remove the line if it serves no purpose
2 participants
Some fields from encoding was causing issues in the
_Flight__evaluate_post_process()method. Made a small hotfix for now to remove the bad actors that could not be converted.Summary by CodeRabbit
New Features
Bug Fixes
Chores