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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement JSON and JSONB codecs #161

Merged
merged 1 commit into from Sep 24, 2019
Merged

Implement JSON and JSONB codecs #161

merged 1 commit into from Sep 24, 2019

Conversation

JoseLion
Copy link
Contributor

Hi @mp911de! I'm opening this PR as an alternative to #39 but using Jackson dependency instead of GSON. I also took into account your suggestions to provide a certain level of flexibility and added return types for ByteBuf, byte[], String and InputStream.

I only added the codec to JSONB since we'll need another codec for JSON. This is because the doEncode(Object value) method does not have any way to choose which one it should encode: JSON or JSONB.
I can open another PR after this one with the JSON codec or add it to this PR if you think it would be ok 馃檪

Cheers!

@mp911de
Copy link
Collaborator

mp911de commented Sep 17, 2019

Thanks for your PR. As stated in #39, supporting JSON(Oid) is something that we want. What we do not want is including a dependency to any JSON library in the core driver. We should expose JSON payload as binary (byte array, ByteBuffer, ByteBuf?, InputStream) data type, similar for writing.

Any JSON processors should live on the client library or application level or can be contributed by driver extensions (driver extensibility is on our roadmap).

We鈥檙e happy to merge basic support for now, without Jackson.

@JoseLion
Copy link
Contributor Author

JoseLion commented Sep 20, 2019

@mp911de that makes sense! I updated the branch with the suggested approach, please let me know what you think 馃檪
I cannot change the branch name so if you prefer I could close this PR and open a new one with a proper branch name

PS: I included both JSON and JSONB codecs

@mp911de
Copy link
Collaborator

mp911de commented Sep 20, 2019

No worries, the branch name isn鈥檛 an issue at all. Codecs already look well. Recently we kept binary and text formats within a single codec (see https://github.com/r2dbc/r2dbc-postgresql/blob/master/src/main/java/io/r2dbc/postgresql/codec/AbstractNumericCodec.java). I鈥檇 suggest adding an AbstractJsonCodec with concrete subtypes for ByteBuffer, ByteBuf, byte[] and InputStream.

The need for multiple subtypes is current a flaw of our approach that we will need to cleanup.

@JoseLion JoseLion changed the title Implement JSONB codec based on "com.fasterxml.jackson.core" library Implement JSON and JSONB codecs Sep 20, 2019
@mp911de mp911de mentioned this pull request Sep 20, 2019
@JoseLion
Copy link
Contributor Author

@mp911de awesome, good to know we're going the right way! Just to be clear, we'll need the following codecs based on an AbstractJsonCodec right?

  • ByteBufferJsonCodec, ByteBufferJsonbCodec
  • ByteBufJsonCodec, ByteBufJsonbCodec
  • ByteArrayJsonCodec, ByteArrayJsonbCodec
  • InputStreamJsonCodec, InputStreamJsonbCodec

I'm not sure we can have JSON and JSONB in the same codec file since the doEncode(Object value) still doesn't have a way to know which one to use. Does this make sense? 馃

@mp911de
Copy link
Collaborator

mp911de commented Sep 21, 2019

Partially. AbstractJsonCodec would resemble AbstractBinaryCodec to some extent. We don't require a distinction between Json and Jsonb codecs, they could be handled within the same codec.

Codec decoding is based on format, type oid and the requested Class. Encoding solely relies on the value/value type. For byte[] and ByteBuffer, all encoding would be handled by our binary codecs as we don't have a mechanism to enforce a JSON codec to encode the value.

We want to introduce with R2DBC 0.9 type descriptors so a value can be encoded using a more specific codec.

@mp911de
Copy link
Collaborator

mp911de commented Sep 24, 2019

Thanks for your PR. I'm going to take it from here.

@mp911de mp911de added this to the 0.8.0.RC1 milestone Sep 24, 2019
@mp911de mp911de added the type: enhancement A general enhancement label Sep 24, 2019
@mp911de
Copy link
Collaborator

mp911de commented Sep 24, 2019

There's one fundamental issue that became obvious during integration testing requiring us to rethink the API. Using byte[], ByteBuffer or String as interchange type leads to:

 column "value" is of type jsonb but expression is of type bytea

as Postgres isn't able to convert between json/jsonb and bytea or varchar.

It seems that we should add a dedicated Json type so we can enforce a proper JSON type to encode outgoing values.

mp911de added a commit that referenced this pull request Sep 24, 2019
Introduce Json wrapper type to avoid conflicts with BYTEA
(byte[], ByteBuffer) encoding. Split JSON codec into AbstractJsonCodec
and subclasses for proper wrapper and scalar type handling.

Json wrapper allows exchange of JSON payload without buffer copies.

Add license headers. Extend integration tests.

[#161]
mp911de added a commit that referenced this pull request Sep 24, 2019
@mp911de mp911de merged commit e186de0 into pgjdbc:master Sep 24, 2019
mp911de added a commit that referenced this pull request Sep 24, 2019
@mp911de
Copy link
Collaborator

mp911de commented Sep 24, 2019

That's merged and polished now.

@JoseLion
Copy link
Contributor Author

There's one fundamental issue that became obvious during integration testing requiring us to rethink the API. Using byte[], ByteBuffer or String as interchange type leads to:
column "value" is of type jsonb but expression is of type bytea

@mp911de I was actually looking into that. A dedicated Json type sounds like a great idea to me 馃檪

Thanks a lot for taking the time to check and work on the PR. I'm glad this was of help and it got merged.

Cheers!

@JoseLion JoseLion deleted the jsonb-codec-with-jackson branch October 22, 2019 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants