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

New feautre: @JsonIgnoreIf on base 2.14 #3375

Open
wants to merge 1 commit into
base: 2.14
Choose a base branch
from

Conversation

sebastianbrunnert
Copy link

Hello everyone,

We discussed this new feature already in a closed PR. It was on the wrong branch. Therefore here on branch 2.14:

In several projects (especially REST APIs) in which I used Jackson I needed the feature to ignore an Jackson property dynamically (e.g. based on authorization). I always used some kind of workaround.

So I decided to build the @JsonIgnoreIf annotation. You can specify a class extending the abstract class JsonIgnoreValidator.
The usage is basically very easy.

public static class JsonIgnoreForGuests extends JsonIgnoreValidator {
    @Override
    public boolean ignore() {
        // Example with Spring: Ignoring if there is no Authentication
        return !SecurityContextHolder.getContext().getAuthentication().isAuthenticated();
    }
}

Defining properties that should be checked before transfering into JSON works by:

@JsonIgnoreIf(JsonIgnoreForGuests.class)
public String sensibleData = "Lorem Ipsum";

Best regards from Germany
Sebastian

JsonIgnoreValidator jsonIgnoreValidator = jsonIgnoreIfAnn.value().newInstance();
return jsonIgnoreValidator.ignore();
} catch (InstantiationException | IllegalAccessException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

um, definitely not. :)

JsonIgnoreIf jsonIgnoreIfAnn = _findAnnotation(a, JsonIgnoreIf.class);
if(jsonIgnoreIfAnn != null) {
try {
JsonIgnoreValidator jsonIgnoreValidator = jsonIgnoreIfAnn.value().newInstance();
Copy link
Member

Choose a reason for hiding this comment

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

This has to be changed, handlers would need to be instantiated using HandlerInstantiator as is done with other helper objects. Alas, method is not yet passed MapperConfig and it's not trivial to get it there...

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 17, 2022

Ok, I think that if this was to be merged, I'd like to make this bit more contextual, partly to allow HandlerInstantiator to be used, and partly to allow JsonIgnoreValidator to not have to rely on ThreadLocal or similar work-arounds (like static contexts). I might do some refactoring for

But what I am also thinking is this: I am not 100% sure this makes sense as an out-of-the-box feature, because you can actually implement this by custom AnnotationIntrospector already. You can either sub-class JacksonAnnotationIntrospector, or just AnnotationIntrospector and add insert/append it.

So at this point I am still evaluating whether this should be included, and if so in what form.

But I will keep thinking about it: I think the idea is good, I am just not sure how it should be supported.

@@ -1430,6 +1431,15 @@ protected boolean _isIgnorable(Annotated a)
if (ann != null) {
return ann.value();
}
JsonIgnoreIf jsonIgnoreIfAnn = _findAnnotation(a, JsonIgnoreIf.class);

Choose a reason for hiding this comment

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

I could be wrong, but isn't the issue with implementing this in JacksonAnnotationIntrospector that introspection is supposed to be cached in the ObjectMapper which would mean that these conditionals would only be computed statically? I guess the use-case for the annotation introduced here actually meant just-in-time / during serialization authorization checks. I think currently only a @PropertyFilter is suitable for that.

Copy link
Member

Choose a reason for hiding this comment

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

@jwgmeligmeyling Yes, you are correct: this would not work as intended for reasons you outline.

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.

3 participants