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

Support non Foundation JSON coders #125

Conversation

jordanebelanger
Copy link
Contributor

@jordanebelanger jordanebelanger commented Aug 29, 2020

Add support for custom, non-Foundation, JSON encoder and decoder through global variables PostgresNIO._defaultJSONEncoder and PostgresNIO._defaultJSONDecoder (#125, fixes #126).

* add a `PostgresJSONEncoder` protocol with an API that mirrors the Foundation `JSONEncoder`'s decoding function

* add global `_defaultJSONEncoder` variable used in the JSON and JSONB type and that is defaulted to a Foundation `JSONEncoder`

* add a `PostgresJSONDecoder` protocol with an API that mirrors the Foundation `JSONDecoder`'s decoding function

* add global `_defaultJSONDecoder` variable used in the JSON and JSONB type and that is defaulted to a Foundation `JSONDecoder`
@tanner0101 tanner0101 added enhancement New feature or request semver-minor labels Sep 2, 2020
@tanner0101 tanner0101 added this to Awaiting Review in Vapor 4 via automation Sep 2, 2020
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. This is a good compromise for the time being. A few things:

1: Add a test so we can make sure this works and doesn't get accidentally removed or broken in the future. This should set the static var back to normal after to not affect other tests.
2: Update PR body with a summary for release notes: https://github.com/vapor/vapor/blob/master/.github/contributing.md#releases
3: A docblock where _defaultJSON{Decoder,Encoder} is defined explaining what it does and to care for thread safety would be nice.

@jordanebelanger
Copy link
Contributor Author

@tanner0101 Closing this and reopened on #127, for some reason this PR's branch name "no-currentIndex-increment..." got messed up no idea why

Vapor 4 automation moved this from Awaiting Review to Done Sep 4, 2020
@jordanebelanger jordanebelanger deleted the no-currentIndex-increment-on-decode-fail branch September 4, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

Support custom json decoder and encoder when dealing with JSON/JSONB columns
2 participants