fix: Fix rendering of nan and inf in JSON responses #20302
Conversation
webjunkie
changed the title
| @@ -187,7 +187,7 @@ def property_definitions(self, request: request.Request, **kw): | |||
|
|
|||
| group_type_index_to_properties = defaultdict(list) | |||
| for group_type_index, key, count in rows: | |||
| group_type_index_to_properties[group_type_index].append({"name": key, "count": count}) | |||
| group_type_index_to_properties[str(group_type_index)].append({"name": key, "count": count}) | |||
There was a problem hiding this comment.
Not sure I understand this change 🤔
There was a problem hiding this comment.
They became string keys anyway with the previous JSON serializer. This one threw an error, unless we flip an option. Since we had it as string, I explicitly made it string so the serializer does not complain.
Sorry, could have mentioned in changes.
| @@ -565,7 +565,7 @@ def local_evaluation(self, request: request.Request, **kwargs): | |||
| seen_cohorts_cache[id] = cohort or "" | |||
|
|
|||
| if cohort and not cohort.is_static: | |||
| cohorts[cohort.pk] = cohort.properties.to_dict() | |||
| cohorts[str(cohort.pk)] = cohort.properties.to_dict() | |||
There was a problem hiding this comment.
... or this. Why convert them to strings?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
|
@webjunkie friendly reminder to tag me/Juraj whenever anything changes to feature flags / local_evaluation / decide api endpoints, no matter how small. No expectation to block on us, but would just like to be aware. Glad the tests were sufficient here to get the exact format we needed 👍 |
|
Got it! Was there some issue with it? |
|
Nope, and I don't expect there to be any in the future either 😅 #WeTrustOurTests - but just for awareness |
Problem
We still have a problem with inf and nan values in JSON, if they appear in tuples.
Changes
nullby default, no more traversing the whole response with our own loops 🚀How did you test this code?