Skip to content

BUG: issues with encoding and null field updates #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

BUG: issues with encoding and null field updates #60

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
82ea8bb
MNT: updating objects with null initialization on edit
aasitvora99 Oct 11, 2025
44a2ebc
BUG: fix handling of corrupted Flight object in InfinityEncoder
aasitvora99 Oct 11, 2025
8cbd939
Update src/utils.py
GabrielBarberini Oct 11, 2025
9114d81
Update src/utils.py
GabrielBarberini Oct 11, 2025
cbefff8
Update src/utils.py
GabrielBarberini Oct 11, 2025
b721e28
Update src/utils.py
GabrielBarberini Oct 11, 2025
2f36252
Revert "BUG: fix handling of corrupted Flight object in InfinityEncoder"
aasitvora99 Oct 11, 2025
597aad0
BUG: fix method call for post-processing in InfinityEncoder
aasitvora99 Oct 11, 2025
ec9a552
Merge branch 'bug/simulate-forward-fixes' of github.com:RocketPy-Team…
aasitvora99 Oct 11, 2025
0286681
REV: undoing changes
aasitvora99 Oct 11, 2025
476f88b
chore: type correction
aasitvora99 Oct 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/repositories/flight.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async def read_flight_by_id(self, flight_id: str) -> Optional[FlightModel]:
@repository_exception_handler
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
)
Comment on lines 29 to 32
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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:

  1. The update operation is meant to be a full replacement (all fields should be updated, including None values)
  2. The underlying update_by_id implementation filters out None values before persisting
  3. 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=py

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aasitvora99 why is this necessary? I would prefer we drop the null fields unless their null state is actually used by the lib

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


@repository_exception_handler
Expand Down
2 changes: 1 addition & 1 deletion src/repositories/motor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async def read_motor_by_id(self, motor_id: str) -> Optional[MotorModel]:
@repository_exception_handler
async def update_motor_by_id(self, motor_id: str, motor: MotorModel):
await self.update_by_id(
motor.model_dump(exclude_none=True), data_id=motor_id
motor.model_dump(exclude_none=False), data_id=motor_id
)

@repository_exception_handler
Expand Down
2 changes: 1 addition & 1 deletion src/repositories/rocket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async def read_rocket_by_id(self, rocket_id: str) -> Optional[RocketModel]:
@repository_exception_handler
async def update_rocket_by_id(self, rocket_id: str, rocket: RocketModel):
await self.update_by_id(
rocket.model_dump(exclude_none=True), data_id=rocket_id
rocket.model_dump(exclude_none=False), data_id=rocket_id
)

@repository_exception_handler
Expand Down
26 changes: 24 additions & 2 deletions src/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,30 @@ def default(self, o):
mutate_self=False,
)
if isinstance(obj, Flight):
obj._Flight__evaluate_post_process()
solution = np.array(obj.solution)
try:
evaluate_post_process = getattr(obj, '_Flight__evaluate_post_process', None)

# Check if it's corrupted (numpy array instead of cached_property)
if isinstance(evaluate_post_process, np.ndarray):
try:
delattr(obj, '_Flight__evaluate_post_process')

restored_method = getattr(obj, '_Flight__evaluate_post_process', None)
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt @aasitvora99 ?

if restored_method and callable(restored_method):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably this is a tautology.

If it is callable it will always pass the first check.

Maybe just reduce it to if callable(obj)

restored_method()
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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Only callable check is enough

evaluate_post_process()

except (AttributeError, TypeError, ValueError) as e:
logger.error(f"Error handling Flight object corruption: {e}")

try:
solution = np.array(obj.solution)
except Exception as e:
return super().default(obj) # Fall back to parent encoder
size = len(solution)
if size > 25:
reduction_factor = size // 25
Expand Down
Loading