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

Make JsonPointer java.io.Serializable #762

Merged
merged 5 commits into from Jun 16, 2022
Merged

Conversation

egalpin
Copy link
Contributor

@egalpin egalpin commented Jun 9, 2022

Noting that Instances [of JsonPointer] are fully immutable and can be cached, shared between threads it would be nice to be able to pass these between workers that require object serialization to pass instances around.

Noting that `Instances [of JsonPointer] are fully immutable and can be cached, shared between threads` it would be nice to be able to pass these between workers that require object serialization to pass instances around.
@cowtowncoder
Copy link
Member

Seems reasonable, I like the idea. However, I wonder if this could be optimized to actually serialize as String, re-parse? (implement Externalizable)
I think this would be much more efficient, space-wise, than relying on JDK serialization.

@egalpin
Copy link
Contributor Author

egalpin commented Jun 10, 2022

@cowtowncoder good call. I ran into an issue where class properties that need to be set via readExternal cannot be declared final. I made 2 commits, each with a different approach to maintaining the final properties of JsonPointer as finals. The 3rd option, not implemented here but very easy to do, is to change the 4 properties (_nextSegment, _matchingPropertyName, _matchingElementIndex, _asString) to not be final; I assumed that was less preferable but I'll leave that up to you.

@cowtowncoder
Copy link
Member

Ok thank you. One thing I also need to consider is that I hope implement fix for #736 which will probably change internal layout of things anyway.

*/
protected JsonPointer() {
public JsonPointer() {
Copy link
Member

Choose a reason for hiding this comment

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

Does Externalizable really require that? TIL

@egalpin
Copy link
Contributor Author

egalpin commented Jun 10, 2022

Does Externalizable really require that? TIL

It turns out it's not 😅 It requires a no-argument constructor, which is already present. It does! I forgot that the second implementation has the JsonPointer class implementing serializable rather than Externalizable. In f53ef84 you would find that having the access be protected would yield the following error:

java.io.InvalidClassException: com.fasterxml.jackson.core.JsonPointer; no valid constructor

@cowtowncoder
Copy link
Member

Ok, this looks good. One last thing, CLA: unless you have already sent one, we'd need this:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(the usual way is to print, fill & sign, scan/photo, email to info at fasterxml dot com)

If you have already sent one already let me know (sometimes I forget as there are quite a few).

Once there's CLA I'll merge this. CLA works for any and all future contributions too so it's just a one-time hassle.

Looking forward to merging this PR!

@egalpin
Copy link
Contributor Author

egalpin commented Jun 14, 2022

Signed and sent 🙂

@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Jun 16, 2022
@cowtowncoder cowtowncoder merged commit ca18393 into FasterXML:2.14 Jun 16, 2022
@cowtowncoder cowtowncoder changed the title Propose that JsonPointer be Serializable Make JsonPointer java.io.Serializable Jun 16, 2022
cowtowncoder added a commit that referenced this pull request Jun 16, 2022
@cowtowncoder
Copy link
Member

Merged to be included in 2.14.0. Thank you very much for contributing this, @egalpin !

@egalpin
Copy link
Contributor Author

egalpin commented Jun 16, 2022

Thanks @cowtowncoder for the prompt and welcoming review! 🙏

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