UnrecognizedPropertyException is not thrown when deserializing properties that are not part of the view

[sebdotv]

Using a deserialization view and DeserializationFeature.FAIL_ON_IGNORED_PROPERTIES, it could be convenient if an error was raised when attempting to deserialize a property that is not part of the view.

In the following example code, the property annotatedWithView2 (which is not part of the deserialization view, View1) is correctly ignored during deserialization, but no error is raised.

This could be seen as an extension of #343 for views.

    interface View1 { }
    interface View2 { }

    static class T {
        public Long unannotated;
        @JsonView(View1.class)
        public Long annotatedWithView1;
        @JsonView(View2.class)
        public Long annotatedWithView2;
    }

    public static void main(String[] args) throws IOException {
        T t = new ObjectMapper()
            .enable(DeserializationFeature.FAIL_ON_IGNORED_PROPERTIES)
            .readerWithView(View1.class)
            .withType(T.class)
            .readValue("{ \"unannotated\":1, \"annotatedWithView1\":1, \"annotatedWithView2\":1 }");

        System.out.println(t.unannotated + " " + t.annotatedWithView1 + " " + t.annotatedWithView2);
    }

[cowtowncoder]

Ok. I think I understand the request. Will need to see how easy this would be to implement. Also wondering what the backwards compatibility constraints are (that is, would this handling need to be behind an on/off feature), given that it would make previously working cases fail.

[sebdotv]

Maybe the semantics of the DeserializationFeature.FAIL_ON_IGNORED_PROPERTIES optin is large enough to cover the case already? Would need feedback from actual deserialization view users, which I'm not yet.

[cowtowncoder]

Yes, semantics could cover, but behavioral changes are always risky.

[davideanderson]

I just ran into this myself. For what it is worth, I believe this would be the place to do the test and throw

com.fasterxml.jackson.databind.deser.BeanDeserializer#deserializeWithView:

                if (!prop.visibleInView(activeView)) {
                    jp.skipChildren();
                    continue;
                }

So, rather than simply skipping children and continuing, test the configuration and throw if appropriate.

[cowtowncoder]

@davideanderson thank you; this should help in figuring out how to make it work; sounds like there'd be all access needed to see what should be the action.

[cowtowncoder]

Hmmh. Just realized something: number of DeserializationFeatures has grown rapidly and there are some relevant limits to consider. So will have to take my time to consider how this should actually be integrated...

[simontb]

I think the place that @davideanderson pointed out is appropriate and jp.skipChildren(); could be replaced with handleUnknownProperty(p, ctxt, bean, propName);.

@cowtowncoder I get your point, that this could break working code, but since I see this as a bug, I think it's okay to make this change without introducing a new DeserializationFeature like FAIL_ON_DISABLED_BY_VIEW. Otherwise the call to handleUnknownProperty would need to be toggled by this feature.

[cowtowncoder]

@simontb Yes, I am sure you would be perfectly happy by my changing of feature to make it work like you want, and then my getting to support literally dozens of bug reports by very annoyed users with production system that start failing when do patch-level update. :-D

Seriously tho I take backwards compatibility seriously, based on my experiences as the maintainer and think that such a change would be a Wrong Thing To Do in 2.9.x patch. And risky even in minor version (2.9 -> 2.10, if such was released).

I do actually agree that behavior can be surprising; and that expected behavior is not specified one way or another -- consider the fact that properties ARE known to deserializer, they just are not to be included in the view. Because of this, I do think another setting is needed. I am pretty sure there are users that rely on existing behavior as simple filter, to avoid overriding values of POJOs in some cases.

[simontb]

I totally agree, that an incompatible change shouldn't be a patch.

consider the fact that properties ARE known to deserializer, they just are not to be included in the view.

Very good point. I didn't see it from that perspective. Maybe DeserializationFeature.FAIL_ON_IGNORED_PROPERTIES would be a better fit? I am also open to a new feature, but since you seemed a bit reluctant to introduce another deserialization feature, I just wanted to give feedback. In the meantime I worked around this issue with some customization on my side.

[cowtowncoder]

@simontb I appreciate your feedback and ideas. I often find my view that seems intuitive and self-evident to me is nothing of kind to many others. It is difficult to consider things from other perspectives after seeing them make sense from one.

So... I think I need to think about how to handle this: I think expectation of being able to sort of prohibit properties that are excluded by active View is perfectly reasonable one, among alternative expectations. And as such it should be possible to change.

A new DeserializationFeature might be ok as this is sort of general purpose feature, and not type-specific. Or, if introducing new sets of features (ViewFeature?), could naturally fit into subsection.

[evgenivanovi]

Any updates on this?

[toolforger]

Just another data point from my side.

CONTEXT:

I am building a new REST service so legacy considerations do not affect me (not currently, I am very aware how important legacy is).

I am currently using a view to restrict what fields can be updated via HTTP PATCH requests with JSON payloads, i.e. the allowed fields are annotated @JsonView(Patchable.class).

CONCLUSION 1: Currently, Jackson will silently ignore such fields. That's bad, because I need to inform clients if they try to update a field that's there but not allowed to be updated that way. Right now, the result is that the patch is partly applied; in situations where multi-field consistency conditions are involved, this could actually be worse than failing the PATCH (fortunately, I do not have that problem - yet).

CONCLUSION 2: I'm coming from a RDBMS perspective. There an updatable view is essentially equivalent to a table, trying to access a column that is not present in the view is a hard error. Jackson silently ignoring such a field, even with FAIL_ON_UNRECOGNIZED_PROPERTIES switched on, was pretty surprising to me.

RECOMMENDATION: I agree with @simontb that FAIL_ON_IGNORED_PROPERTIES would be the best fit. However, since backwards compatibility is paramount, I'd add VIEW_EXCLUDED_FIELDS_ARE_UNKOWN as a new feature, with a default value of false. The list of features is far too long already so for 3.0, I'd either declare that a view is a derived schema, or keep the feature and change the default to true.

Hope this helps.