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

bplan/serializer: add binary=True to point JSONField #5917

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented Jan 7, 2025

bplan/serializer: add binary=True to point JSONField as we receive a string containing (geo)json, not actual json. This came up during testing.

Tasks

  • PR name contains story or task reference
  • Steps to recreate and test the changes
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@goapunk goapunk requested a review from m4ra January 7, 2025 13:59
string containing (geo)json, not actual json
@goapunk goapunk force-pushed the jd-2025-01-bplan-point-json-fix branch from af83df9 to 6e2c998 Compare January 7, 2025 14:12
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

so point can be sent as bytestring? see my comment.

@@ -101,7 +101,7 @@ The following fields need to be provided:
- End date of the participation in
- [ISO 8601 format](https://en.wikipedia.org/wiki/ISO_8601)
(if no time zone is defined, german time zones UTC+01 and UTC+02 are used)
- *point*: geojson
- *point*: string containing valid geojson
Copy link
Contributor

Choose a reason for hiding this comment

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

with the binary=True now, I assume, this can be a bytestring? Can you add this info here, in a changelog, and a test case?

Copy link
Contributor Author

@goapunk goapunk Jan 7, 2025

Choose a reason for hiding this comment

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

@m4ra I'm not entirely sure what you mean by bytestring in this context? The documentation for binary=true is:

binary - If set to True then the field will output and validate a JSON encoded string, rather than a primitive data structure. Defaults to False.

which doesn't mention bytestrings. Maybe you have an example?

Regarding changelog: the point field is new and unreleased, so not sure what to put in the changelog for it other than what's already there in the original PR

Copy link
Contributor

Choose a reason for hiding this comment

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

From docs again, the whole JSONFiled says:

JSONField

A field class that validates that the incoming data structure consists of valid JSON primitives. In its alternate binary mode, it will represent and validate JSON-encoded binary strings.

Signature: JSONField(binary, encoder)

binary - If set to True then the field will output and validate a JSON encoded string, rather than a primitive data structure. Defaults to False.

I understand when binary is True it can accept binary strings, misspelled it before with bytestrings.

Copy link
Contributor Author

@goapunk goapunk Jan 8, 2025

Choose a reason for hiding this comment

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

Ok, I did some testing: with binary=False it accepts actual json values (python dicts) but rejects a string (2nd example)

input = {"type":"Feature", "geometry":{"type":"Point","coordinates":[13.397788148643649,52.52958586909979]}}

whereas with binary=True it accepts a string containing json but rejects the example above:

input = '{"type":"Feature", "geometry":{"type":"Point","coordinates":[13.397788148643649,52.52958586909979]}}'

So unless I'm missing something it behaves as written in our bplan api docs, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

great, thanks for clarifying it!

@m4ra m4ra merged commit 669cac7 into main Jan 8, 2025
2 of 3 checks passed
@m4ra m4ra deleted the jd-2025-01-bplan-point-json-fix branch January 8, 2025 11:35
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