Skip to content
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.

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

fix(python): make sure json.dumps return JSON compliant JSON (python) #2683

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

saulane
Copy link
Contributor

@saulane saulane commented Aug 1, 2024

The Python version can return a non-JSON-compliant value to Redis, which breaks when trying to retrieve the value using the Node version of the library.

By adding "allow_nan=True", we raise an error before sending the value to Redis, which prevents having bad JSON in the job's return value.

@roggervalf
Copy link
Collaborator

hi @saulane, thanks for this fix. Could you also add a test case where it would fail without this change?

@saulane saulane force-pushed the json-compliant-python branch from ed428de to 29f9a08 Compare August 1, 2024 14:41
@saulane saulane force-pushed the json-compliant-python branch from 29f9a08 to a58e6f0 Compare August 1, 2024 18:58
@saulane
Copy link
Contributor Author

saulane commented Aug 1, 2024

hi @saulane, thanks for this fix. Could you also add a test case where it would fail without this change?

@roggervalf done

Copy link
Collaborator

@roggervalf roggervalf left a comment

Choose a reason for hiding this comment

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

Lgtm

@roggervalf roggervalf merged commit 4441711 into taskforcesh:master Aug 2, 2024
7 of 10 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 7, 2024
## [5.12.1](v5.12.0...v5.12.1) (2024-08-07)

### Bug Fixes

* **job:** consider passing stackTraceLimit as 0 ([#2692](#2692)) ref [#2487](#2487) ([509a36b](509a36b))
* **job:** make sure json.dumps return JSON compliant JSON [python] ([#2683](#2683)) ([4441711](4441711))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants