Skip to content

fix: handle TypeError exposed by simple history logic #5476

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

Merged
matthewelwell merged 2 commits into from
May 29, 2025
Merged

fix: handle TypeError exposed by simple history logic #5476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5be328
Handle typeerror
matthewelwell May 21, 2025
ba588c1
Handle keyerror when sending invalid payload to update / create multi…
matthewelwell May 22, 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
16 changes: 9 additions & 7 deletions api/core/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,11 @@ class _BaseHistoricalModel(models.Model):
class Meta:
abstract = True

def get_change_details(self) -> typing.Optional[typing.List[ModelChange]]: # type: ignore[return]
if self.history_type == "~": # type: ignore[attr-defined]
def get_change_details(self) -> typing.List[ModelChange]:
Copy link
Copy Markdown
Contributor

@Zaimwa9 Zaimwa9 May 27, 2025

Choose a reason for hiding this comment

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

Just in case we could add a test to ensure it's covered

def test_get_change_details_handles_exception_prev_record_is_null():
    instance = HistoricalFeatureState(history_type="~")
    instance.prev_record = None 

    assert instance.get_change_details() == []

Ideally we could log when it happens but I let you decide if we will really track it or covering the case is enough

Copy link
Copy Markdown
Contributor Author

@matthewelwell matthewelwell May 29, 2025

Choose a reason for hiding this comment

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

Unfortunately it's not that easy. prev_record is a property, not an attribute, so I think any test that we add would involve more mocking than is really worthwhile.

Regarding the logging, since we've only seen this on a handful of cases, and the use case for this data is pretty slim, I don't think it's worth investigating further, hence negating the need for any additional logging.

I'm going to go ahead and merge this, but let me know if you disagree with either of my points above.

# Note that self.prev_record should never be None when self.history_type == "~" but we have
# seen rare cases in Sentry which cause an unhandled exception, so we instead handle the case
# and fall back to just return an empty list.
if self.history_type == "~" and self.prev_record is not None: # type: ignore[attr-defined]
return [
change
for change in self.diff_against(self.prev_record).changes # type: ignore[attr-defined]
Expand All @@ -96,11 +99,10 @@ def get_change_details(self) -> typing.Optional[typing.List[ModelChange]]: # ty
for key, value in self.instance.to_dict().items() # type: ignore[attr-defined]
if key not in self._change_details_excluded_fields # type: ignore[attr-defined]
]
elif self.history_type == "-": # type: ignore[attr-defined]
# Ignore deletes because they get painful due to cascade deletes
# Maybe we can resolve this in the future but for now it's not
# critical.
return []

# Note that we ignore deletes because they get painful due to cascade deletes. Maybe we can
# resolve this in the future but for now it's not critical.
return []


def base_historical_model_factory(
Expand Down
2 changes: 1 addition & 1 deletion api/features/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ def validate(self, attrs): # type: ignore[no-untyped-def]
)

mv_values = attrs.get("multivariate_feature_state_values", [])
if sum([v["percentage_allocation"] for v in mv_values]) > 100:
if sum([v.get("percentage_allocation", 0) for v in mv_values]) > 100:
raise serializers.ValidationError(
"Multivariate percentage values exceed 100%."
)
Expand Down
Loading