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

Missing JsonView annotation interpreted wrong #4127

Closed
Plunts opened this issue Feb 22, 2022 · 4 comments · Fixed by #4391
Closed

Missing JsonView annotation interpreted wrong #4127

Plunts opened this issue Feb 22, 2022 · 4 comments · Fixed by #4391

Comments

@Plunts
Copy link

Plunts commented Feb 22, 2022

The JsonView annotation is interpreted slightly wrong: fields which are not annotated with it are actually not serialized if a view is specified, they are only included in the default serialization. This is described in this blogpost and I'm using this behaviour in my own code. The original MR for this (#2662) stated it differently, namely that it's always included.

The affected method is here:

@frantuma
Copy link
Member

frantuma commented Nov 2, 2022

Thanks for reporting this and for related #4128. I am however not sure about reversing current behavior, and not sure about your comment:

According to docs DEFAULT_VIEW_INCLUSION is enabled by default in Jackson, which means that all fields NOT annotated with a @JsonView are serialized regardless. This matches current behavior in Swagger Core.

This is also clarified in the Baeldung tutorial you mentioned:

It's also important to understand, that – by default – all properties not explicitly marked as being part of a view, are serialized. We are disabling that behavior with the handy DEFAULT_VIEW_INCLUSION feature.

Applying #4128 would reverse this behavior to match scenarios with DEFAULT_VIEW_INCLUSION disabled (not the default).

Possibly many user disable DEFAULT_VIEW_INCLUSION, therefore it would make sense to also support this case, this would involve however some more effort (PRs more than welcome) to allow to provide an option. At this moment this can be achieved either e.g. by using a system property or by adding an annotation e.g. at method level. The annotation way would involve more effort as all the call chain would be affected.

@Plunts
Copy link
Author

Plunts commented Nov 2, 2022

Thanks for replying. I didn't know that this was configurable (and that the default is different). Therefore I investigated and found the reasons for my ignorance: the default is actually the opposite in spring only, see Jackson2ObjectMapperBuilder.

Now that I know about that, I'll try to create a second PR which checks for the actual value and adapts it dynamically.

@Plunts
Copy link
Author

Plunts commented Nov 3, 2022

As promised I created a new PR with a much more gentle change: if no annotation is present, check said feature and return accordingly. This of course has to be combined with a change in the springdoc library which calls swagger-core, which I also created a PR for: springdoc/springdoc-openapi#1924

@frantuma
Copy link
Member

frantuma commented Nov 4, 2022

Thanks for your effort!

Unfortunately the solution in #4295 is targeting a "different mapper" and wouldn't work in most scenarios. When using Swagger Core usually two mappers are involved (when using Jackson based payloads binding framework/lib):

  1. The mapper in use / configured by the library/code providing payload binding, it's the one (de)serializing request/response payloads for your project endpoints, e.g. the one in use by Spring, Jersey, Resteasy, etc.
  2. The mapper in use by Swagger Core, heavily configured to server Swagger needs, used while resolving the specification and serializing correctly this specification into JSON/YAML.

These two mappers can and usually have different configurations, as they serve different needs, just as example for date serialization, null handling, mixins, etc.

#4295 is targeting 2. (the swagger one), while DEFAULT_VIEW_INCLUSION is configured on 1. (the Spring one).

One solution can be one of the options mentioned in comment above, with no "auto sync" with the project mapper configuration (you would need to manually pass the option/annotation to inform swagger about wished behavior)

Another better solution, also open more possibilities for other "configuration sync" would be allowing to pass to swagger a ref to the second mapper (the spring one). Then it would be easy to check its configuration and update behavior accordingly, also for other areas.

Both are possibly not trivial unfortunately, but not terribly complex either, they involve however an update of various parts of the code, e.g. configuration, resolving logic, ModelResolver/Context.

A different approach with current code is providing a custom ModelResolver extending the default one as detailed in wiki.

You would override hiddenByJsonView, returning true instead of containsJsonViewAnnotation and this would do the trick, limited to your case; something like this:

public class JsonViewAwareModelResolver extends ModelResolver {

    public JsonViewAwareModelResolver(ObjectMapper mapper) {
        super(mapper);
    }


    @Override
    protected boolean hiddenByJsonView(Annotation[] annotations,
                                       AnnotatedType type) {
        JsonView jsonView = type.getJsonViewAnnotation();
        if (jsonView == null)
            return false;
        Class<?>[] filters = jsonView.value();
        for (Annotation ant : annotations) {
            if (ant instanceof JsonView) {
                Class<?>[] views = ((JsonView) ant).value();
                for (Class<?> f : filters) {
                    for (Class<?> v : views) {
                        if (v == f || v.isAssignableFrom(f)) {
                            return false;
                        }
                    }
                }
            }
        }
        return true;
    }
}

registering it either programmatically with ModelConverters.getInstance().addConverter(new JsonViewAwareModelResolver(Json.mapper())); or using modelConverterClasses property in configuration (not sure if/how springdoc allows to pass configuration to swagger)

Just for future ref, here is a comment with some more details about mappers and Swagger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment