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

Allow creation of custom sub-types of NullNode, BooleanNode, MissingNode #1799

Closed
andylowry opened this issue Oct 18, 2017 · 6 comments
Closed
Milestone

Comments

@andylowry
Copy link

The JsonNodeFactory instance always uses canonical instances for certain values of certain node types, including empty arrays, empty objects, ints from -1 to 10, booleans, maybe others.

I have a use-case where I need to parse a JSON/YAML file to a JsonNode tree and use the nodes as keys in an IdentityHashMap. The canonical instances were, naturally, causing problems.

I've created my own factory and installed it in my mapper instances, and that works great. But I can't currently avoid canonical instances of BooleanNode, NullNode and MissingNode, because none of them have a visible constructor (so I can't call the constructors, and I can't extend the types).

I don't actually have a need for non-canonical MissingNode, and my inability to manufacture non-canonical boolean and null nodes is something I can live with (that prevents an unlikely edge-case for my application from working correctly, but I can document that and leave it as is for now).

Adding public constructors to these types would make it possible for me to complete my custom factory. Making existing constructors package-visible and adding a DeserializationFeature that avoids canonical instances would probably be even better.

@cowtowncoder
Copy link
Member

Interesting. Yes, from sub-classibility viewpoint it would make sense to allow use.
I don't think separate feature/option is needed; I am ok with adding a protected constructor for these node types.

But one thing I have to think about is versioning -- in theory this is something that changes API, and should not go in a patch. But the practical concern is that 2.9 is the last 2.x minor version, and 3.0 will take a while. So I may make an exception here; it is not part of public API, either, so it's probably ok.

@andylowry
Copy link
Author

Thanks @cowtowncoder .

To be clear, protected constructors wouldn't help me, since they wouldn't be accessible in my code.

One way to do this without changing the public API would be to give the new constructors package visibility (or more) and create new protected methods in JsonNodeFactory, e.g.:

BooleanNode booleanNode(boolean v, boolean useCanonical) {
    return useCanonical ? booleanNode(v) : new BooleanNode(v);
}

My custom factory, deriving from JsonNodeFactory, would have access to the new methods, and all would work out.

P.S. Technically, I could use protected constructors by creating my own class in the com.fasterxml.jackson.databind.node package, since the jar files I'm getting from maven central aren't sealed. But that would be an exceptionally gross hack! :-)

P.P.S. Speaking of gross hacks, it just occurred to me that I can get access to the private constructors now through reflection. Doing that - and gracefully degrading if the security manager doesn't allow it - might be a reasonable intermediate approach until my needs are met in Jackson.

@cowtowncoder
Copy link
Member

Right, protected constructors would allow sub-classing, that's what I was thinking.
I wasn't thinking you need to add your type in same package (isn't that what is needed with 'no modifier' case rather). But now you got me thinking.

Alternatively I could just add a separate factory method in BooleanNode itself. Or, perhaps, in addition .

@andylowry
Copy link
Author

@cowtowncoder Ah, I get it. My starting point is a custom JsonNodeFactory that I can configure into an ObjectMapper, so I can parse a JSON/YAML file without getting any canonical instances in the resulting tree.

Focusing on just BooleanNode (the other cases are identical), my custom factory would not have access to the protected BooleanNode constructor - since it's not extending BooleanNode - unless I put it in the same package. That's where my head was at.

I didn't make the obvious leap to creating my own custom MyBooleanNode class that extends the standard BooleanNode and therefore has access to a protected BooleanNode constructor. And of course, MyBooleanNode could produce non-canonical instances via either its own constructor or a factory method, visible to my custom factory.

So yes, protected constructors on BooleanNode, NullNode and (for completeness?) MissingNode would work for me. Sorry for confusion.

@cowtowncoder
Copy link
Member

@andylowry Right, makes sense.

Apologies for slow progress here. Will try to add protected constructors for 2.9.3 -- it is a minor deviation from patch guidelines, but given 2.9/3.0 discontinuity in versioning, I think addition at this point is necessary.

@andylowry
Copy link
Author

@cowtowncoder Thanks, much appreciated.

@cowtowncoder cowtowncoder changed the title FEATURE: Deserialization option to avoid use of canonical JsonNode instances Allow creation of custom sub-types of NullNode, BooleanNode, MissingNode Nov 1, 2017
@cowtowncoder cowtowncoder added this to the 2.9.3 milestone Nov 1, 2017
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

No branches or pull requests

2 participants