#2208 Make SubTypeValidator configurable #2269
Conversation
| * default sub-type validator blocks deserialization widgets, i.e. classes that | ||
| * are known to be dangerous when deserialized from untrusted sources. | ||
| */ | ||
| protected final SubTypeValidator _subTypeValidator; |
There was a problem hiding this comment.
Should we support an array of SubTypeValidators instead?
That would be more in line with the other configuration options in this class.
Moreover, it could help ensuring that the default SubTypeValidator cannot be bypassed.
There was a problem hiding this comment.
No, just one is enough. But I do have other concerns.
|
Currently, this PR leaves It might be desirable to convert API compatibility could be preserved for most part. All public methods would work as previously. However, |
|
|
||
| /* | ||
| /********************************************************** | ||
| /* Helper classes (beans) |
There was a problem hiding this comment.
I have created helper beans for this test from scratch.
Could I reuse existing beans that make use of @JsonTypeInfo? Where to find these?
| } | ||
| } | ||
|
|
||
| static class FriendlyAnimalValidator |
There was a problem hiding this comment.
This validator combines the black-list approach from the default SubTypeValidator with a white-list approach for one specific part of the type hierarchy.
Ultimately, it is the goal of #2208 to make the black-list from SubTypeValidator extensible. This is not demonstrated here. But I could add a suitable SubTypeValidator sub-class to this PR, if desired? Or, leave that exercise to the users of Jackson databind?
There was a problem hiding this comment.
Fyi, I've added separate class ExtensibleSubTypeValidator to this PR now, which addresses extensibility of the default black-list. Tests for that are below...
|
|
||
| /* | ||
| /********************************************************** | ||
| /* Unit tests |
There was a problem hiding this comment.
I have left the number of assertions at a minimum. Ultimately, I focused on testing whether a specific validator will reject inputs or not. I can certainly add more assertions on the deserialized data?
| mapper.readValue(PETTING_ZOO_JSON, Zoo.class); | ||
| } | ||
|
|
||
| public void testDenyAllValidator() throws Exception |
There was a problem hiding this comment.
Each test in this class currently test 3 different JSON inputs. Should I rather split each test into 3 separate ones?
| /********************************************************** | ||
| */ | ||
|
|
||
| protected ObjectMapper objectMapper(SubTypeValidator validator) |
There was a problem hiding this comment.
Fyi, this factory method demonstrates how an ObjectMapper could be configured with a custom SubTypeValidator in real-life.
I chose to use sub-class JsonMapper, because ObjectMapper does not seem to be configurable enough. Could that limit the use of custom SubTypeValidators? Is there a way to use ObjectMapper directly, after all?
| extends SubTypeValidator | ||
| { | ||
| private static Class<?>[] FRIENDLY_ANIMAL_WHITELIST = new Class<?>[] { | ||
| Animal.class, |
There was a problem hiding this comment.
Originally, I did not plan to include Animal itself here. It is an abstract class, and I always specify concrete sub-types in my JSON payloads.
However, during debugging I noticed that Animal itself sometimes gets passed as JavaType into SubTypeValidator.validateSubType(...). I could not figure out why yet. I fear it might be an unrelated bug in how the DeserializerFactory calls SubTypeValidator. If that were true, the existing black-list may not be effective in all situations!?! More likely, I have missed something trivial.
|
Ok. So, there are 2 main concerns:
So, while I appreciate help, I plan on proceeding with #2195, to rewrite this functionality. |
|
Thanks for the quick feedback, @cowtowncoder! Regarding 1.) I could not find There are Regarding 2.) I'm a little confused now. I've read #2195 and what you said there:
Did you mean the name only? Or could we reuse the signature of its public method, too? I know that signature may not be ideal. (E.g., I think validation result could be returned or thrown, rather than set on the given Where would you put that new interface? How would you adjust the signature? |
…implementation: ExtensibleSubTypeValidator.
|
One more thing regarding 2.) I had already experimented with extracting an interface from The solution is rather crude, but afaikt it fully preserves backward compatibility. The interface and the existing implementation both have the same name, but different packages. |
|
Hello @cowtowncoder, |
|
So, similar to notes I added on #2208 itself, I do not actually think there is future for this half-thought theoretical extension point. I am sorry for this ending wasted time for you. |
|
OK, no worries. I still learned a lot about Jackson while working on this. In particular, it taught me how to extend the However, you're right that all of that is indeed half-thought. Thus, I'm closing this PR now. |
|
@meeque Ok glad there was some benefit at least. On plus side, I started doing basic scaffolding for the main ticket last night. Baby steps, but getting there. |
This PR is just a first draft towards a solution for #2208. Feel free to comment, so I can refine it. Do not merge as-is, for the following reasons:
PolymorphicTypeValidator, for limiting subtypes allowed by default typing,@JsonTypeInfo#2195 may be necessary.Moreover, I'm still uncertain about several aspects. I'll add respective comments/questions to the code. Maybe someone can help me getting this to a point where the PR could be accepted?