Adding Sql Blob/SerialBlob Serializer #2924
Conversation
|
Sounds reasonable, although there is one challenge here: recent change #2913 will make all of "java.sql" optional wrt Java 9+ module system, and so we'd need new (de)serializer to be similarly dynamic. The simplest (even if not very clean) way would probably to register them via Similarly I think tests probably belong somewhere else: I can move them after merge as well. So if you could move deserialization registration I think this would be mergeable. The other thing I will need before merging is CLA (unless I have asked and received one before?). It's this: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and is easiest to print, fill & sign, scan/photo, email to Thank you again for this PR, looking forward to getting it merged! |
|
Hi, Thanks, |
|
@rsatrio ah, I only skimmed through changes. Adding just serializer first is fine by me -- deserializer would be nice but that can be added as a later step too. I moved "java.sql" based tests to under |
| int bLength = (int) value.length(); | ||
| byte[] blob1 = value.getBytes(1, bLength); | ||
|
|
||
| gen.writeString(Base64.getEncoder().encodeToString(blob1)); |
There was a problem hiding this comment.
Why? call JsonGenerator.writeBinary() which handles base64 encoding.
We also couldn't use java.util.Base64 with 2.12 anyway, as that was added in JDK 8 (and baseline for now is still JDK7), but fortunately no need.
| @JacksonStdImpl | ||
| @SuppressWarnings("serial") | ||
| public class SqlBlobSerializer | ||
| extends JsonSerializer<Blob> |
There was a problem hiding this comment.
Let's use StdScalarSerializer as base instead (serializes as scalar value, JSON String; as opposed to structured Object/Array).
| @@ -48,12 +64,16 @@ | |||
| sers.put(Void.class, NullSerializer.instance); | |||
| sers.put(Void.TYPE, NullSerializer.instance); | |||
|
|
|||
| //And Sql Blob | |||
| sers.put(Blob.class, new SqlBlobSerializer()); | |||
There was a problem hiding this comment.
Move this down to next block, along with java.sql.Timestamp etc; that way it can be optional wrt Module system.
| @@ -48,12 +64,16 @@ | |||
| sers.put(Void.class, NullSerializer.instance); | |||
| sers.put(Void.TYPE, NullSerializer.instance); | |||
|
|
|||
| //And Sql Blob | |||
| sers.put(Blob.class, new SqlBlobSerializer()); | |||
| sers.put(SerialBlob.class, SqlBlobSerializer.class); | |||
There was a problem hiding this comment.
Not sure this is needed as it implements Blob? I guess unit test should show either way.
But if needed, should also be moved within try-block and register SqlBlobSerializer.class and not pre-create serializer.
There was a problem hiding this comment.
It is needed because when I use SerialBlob in the unit test, it will show error "No Serializer". I think this is because somehow it cannot comprehend that SerialBlob is actually implementation of Blob. If you have other suggestion, please do share.
|
Actually going through PR, registering via |
I have sent the CLA through email. |
Hi,
I'm adding Sql Blob/SerialBlob automatic serializer to base64 String. I hope it helps, especially when trying to serializes POJO from DB Query to Json Form.
Thanks,