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: JSONEncoded should be encoded as json not jsonb #347

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

TeofilC
Copy link
Contributor

@TeofilC TeofilC commented Oct 18, 2024

Resolves #344

@TeofilC TeofilC requested a review from ocharles October 18, 2024 10:33
@TeofilC
Copy link
Contributor Author

TeofilC commented Oct 18, 2024

This is a breaking change

@ocharles
Copy link
Contributor

I have a feeling we are relying on Value being jsonb at the moment in CircuitHub code. Could you check that? Hopefully grepping for Column f Value and then checking the current schema will point you in the right direction. Generally speaking I think the current behavior is correct - the PostgreSQL json type preserves whitespace (iirc), but Value doesn't - so it's not quite the same type.

@TeofilC
Copy link
Contributor Author

TeofilC commented Oct 18, 2024

Maybe the right thing to do in that case is have JSONEncoded not go directly via the Value instance

@TeofilC TeofilC changed the title fix: Value should be encoded as json not jsonb fix: JSONEncoded should be encoded as json not jsonb Oct 18, 2024
@TeofilC
Copy link
Contributor Author

TeofilC commented Oct 18, 2024

How about this instead @ocharles ?

@ocharles
Copy link
Contributor

Yea this looks good - I'll have to remind myself how TypeInformation works! Could we write any tests for this?

@TeofilC
Copy link
Contributor Author

TeofilC commented Oct 18, 2024

Actually I'm going to inline some of this to make it a bit clearer and mirror JSONBEncoded

@ocharles
Copy link
Contributor

SGTM!

@TeofilC
Copy link
Contributor Author

TeofilC commented Oct 18, 2024

I've added a test case as well

Copy link
Contributor

@ocharles ocharles left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@TeofilC TeofilC enabled auto-merge (squash) October 22, 2024 11:10
@TeofilC TeofilC merged commit 06af0ea into master Oct 22, 2024
2 checks passed
@TeofilC TeofilC deleted the teo/jsonb branch October 22, 2024 11:13
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.

JSONEncoded produces jsonb rather than the documented json
2 participants