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

Add support for serializing java.sql.Blob #2925

Merged
merged 2 commits into from Nov 9, 2020
Merged

Conversation

rsatrio
Copy link
Contributor

@rsatrio rsatrio commented Nov 7, 2020

Hi,
My previous PR is missing because I delete my commit. Anyway, this PR is for serializer/deserializer SqlBlob into Base64 String. This will be very useful in cases send query result through json. What I have done since the last commit are:

  1. Move the serializer/deserializer registration into OptionalHandlerFactory (following your example in your latest commit),
  2. Refactor to use Jackson's Base64 Function,
  3. Move the unit test to new class
  4. I still put SerialBlob because it is the standard implementation of Sql Blob
  5. Sent CLA doc via email

Thanks,

Rizky

@rsatrio rsatrio changed the title 2.12 Adding serializer/deserializer for sqlblob Nov 7, 2020
@cowtowncoder
Copy link
Member

Looks good, will merge. Thank you!

@cowtowncoder cowtowncoder merged commit ce2b38a into FasterXML:2.12 Nov 9, 2020
@cowtowncoder cowtowncoder changed the title Adding serializer/deserializer for sqlblob Add support for java.sql.Blob Nov 9, 2020
@cowtowncoder cowtowncoder added this to the 2.12.0-rc2 milestone Nov 9, 2020
cowtowncoder added a commit that referenced this pull request Nov 9, 2020
…va.sql.Blob to avoid add'l dependency

Assumption is that serialization more likely needed; also since implementation of deser bit bare-bones,
has issues; may be added if a user requests support.
@cowtowncoder cowtowncoder changed the title Add support for java.sql.Blob Add support for serializing java.sql.Blob Nov 9, 2020
@cowtowncoder
Copy link
Member

Ended up dropping deserialization support from 2.12, as I think it gets tricky, requires new Java 9 module dependency, and has not yet been requested. There are also some technical things about deserialization itself to consider (it's not just json, so incoming value may be "true" binary or base64-encoded data etc). So will want to wait for it being requested -- should probably not have suggested addition yet.
Serialization part may need improvement too, as actual Blob implementations may not get mapped... but user feedback can help there.

@rsatrio
Copy link
Contributor Author

rsatrio commented Nov 10, 2020 via email

@cowtowncoder
Copy link
Member

I think at this point users will need to have a look at PR, if they need to add it. If there was a convenient way to add example code I would include it; but otherwise I don't think I want to add unused code. That tends to lead to maintenance challenges.
But like I said, if someone actually requests deserializer (support for this type has never been requested before that), a new implementation may be added.

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.

None yet

2 participants