HV-1942 Update DefaultGroupSequenceProvider add default method provide the class of instance

[iimik]

The provider can not get current class of instance when the instance is null When use a common DefaultGroupSequenceProvider for multi class. And the method of BeanMetaDataImpl#getValidDefaultGroupSequence checked must have the special group of beanClass.getName() and also can not return the default group of Default.class.

[hibernate-github-bot]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HV-\d+
    ↳ Offending commits: [0d16b27]

› This message was automatically generated.

[marko-bekhta]

Hey @ilikly, thanks for submitting a pull request. Could you please provide a test case showing a problem you are trying to solve with these changes?
Since the group sequence is resolved at runtime based on a type of a bean under validation, if an object passed to the DefaultGroupSequenceProvider is null, there's no point in continuing validation, and a provider can just return null.

But if you are trying to use Validator#validateValue(..) and that's where you get the null, then it is somewhat by design.

[iimik]

Hey @ilikly, thanks for submitting a pull request. Could you please provide a test case showing a problem you are trying to solve with these changes? Since the group sequence is resolved at runtime based on a type of a bean under validation, if an object passed to the DefaultGroupSequenceProvider is null, there's no point in continuing validation, and a provider can just return null.

But if you are trying to use Validator#validateValue(..) and that's where you get the null, then it is somewhat by design.

But if the prodiver just return null will be thrown an exception of GroupDefinitionException at the method org.hibernate.validator.internal.metadata.aggregated.BeanMetaDataImpl#getValidDefaultGroupSequence

[gsmet]

@ilikly please provide a sample project showing what you are trying to achieve and what doesn't work. It's hard to understand your requirement with the current details.

Thanks!

[iimik]

@ilikly please provide a sample project showing what you are trying to achieve and what doesn't work. It's hard to understand your requirement with the current details.

Thanks!

public class Test {


    public static void main(String[] args) {
        final ValidatorFactory validatorFactory = Validation.buildDefaultValidatorFactory();
        final Validator validator = validatorFactory.getValidator();
        A a = new A();
        validator.validate(a);
    }


    public static class GlobalDefaultGroupSequenceProvider implements DefaultGroupSequenceProvider<Object> {

        @Override
        public List<Class<?>> getValidationGroups(Object object) {
            List<Class<?>> groups = new ArrayList<>();

            if (object != null) {
                groups.add(object.getClass());
            }

            return groups;
        }
    }

    @Setter
    @Getter
    @GroupSequenceProvider(GlobalDefaultGroupSequenceProvider.class)
    public static class A {
        @NotNull
        private String name;
    }

    @Setter
    @Getter
    @GroupSequenceProvider(GlobalDefaultGroupSequenceProvider.class)
    public static class B {
        @NotNull
        private String name;
    }
}

The parameter in callback of method getValidationGroups is null, so the GlobalDefaultGroupSequenceProvider can not provide the Default group for A or B class.

[gsmet]

I'm OK to incorporate this change but we need a test added.

Thanks!

[gsmet]

That being said, I'm not sure I understand when having a specific group sequence provider make sense if the bean class is actually null? I would expect it to not be validated and that having specific groups wouldn't make any difference?

So if you contribute a test case, make sure I can see why it makes sense to actually have this feature.

Thanks.

[iimik]

I had add some test for DefaultGroupSequenceProvider.

[valh1996]

@Override
        public List<Class<?>> getValidationGroups(Object object) {
            List<Class<?>> groups = new ArrayList<>();

            if (object != null) {
                groups.add(object.getClass());
            }

            return groups;
        }

Does a workaround exist for this please?

[marko-bekhta]

Hey @valh1996 ,
What exactly are you trying to work around? Any chance you could provide a realistic test case ?

It's been a while since I've looked at the group sequences...
But generally speaking if the object under validation is null then there's not much to validate and the group sequences should be irrelevant... That's why having a realistic test would help to identify where the fix should be applied if any.

[valh1996]

Hi @marko-bekhta
Basically I use the same DTO between user roles to modify an entity. So for example the administrator role can modify all fields, while the lambda user can only modify part of the fields.

To do this, I use @JsonView to indicate which fields are serialized or not, based on role.

@JsonView({View.Admin.class, View.Moderator.class})
@NotNull
private Date name;

The problem is that validation takes place in all cases, so the @NotNull rule doesn't work if the user is neither administrator nor moderator.

So I also want to enable/disable validation rules based on user role using groups :

@JsonView({View.Admin.class, View.Moderator.class})
@NotNull(groups = {View.Admin.class, View.Moderator.class})
private Date name;

But to apply the right groups according to the user's role, I have to use an @GroupSequenceProvider :

public class RoleBasedSequenceProvider implements DefaultGroupSequenceProvider<Object> {
            
    @Override
    public List<Class<?>> getValidationGroups(Object request) {
        List<Class<?>> sequence = new ArrayList<>();
        if (request == null) {
            return sequence;
        }

        sequence.add(request.getClass());

        Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
        if (authentication != null && authentication.getAuthorities() != null) {
            Set<Class<?>> roleGroups = authentication.getAuthorities().stream()
                    .map(GrantedAuthority::getAuthority)
                    .map(ERole::valueOf)
                    .map(View.MAPPING::get)
                    .collect(Collectors.toSet());

            sequence.addAll(roleGroups);
        }
        
        return sequence;
    }
}

Then i can add @GroupSequenceProvider(RoleBasedSequenceProvider.class) in my DTO.

But as the PR indicates, it doesn't work to mark <Object> to make the sequence reusable. It's is null so we cannot add default sequence.

[marko-bekhta]

Hey,

ok, this brings more clarity to what you are trying to achieve. See https://docs.jboss.org/hibernate/validator/9.0/api/org/hibernate/validator/spi/group/DefaultGroupSequenceProvider.html, the DefaultGroupSequenceProvider wasn't intended to use on multiple types and the docs state:

In order to redefine dynamically the default group sequence for a type T, the GroupSequenceProvider annotation must be placed on T, specifying as its value a concrete implementation of DefaultGroupSequenceProvider, which must be parametrized with the type T.

So that means that the provider you are trying to implement:

public class RoleBasedSequenceProvider implements DefaultGroupSequenceProvider<Object> 

should be used on an Object 😔... Now if you are saying

Basically I use the same DTO between user roles to modify an entity

and it means that you are working with a single DTO then just use that DTO instead of the Object:

public class RoleBasedSequenceProvider implements DefaultGroupSequenceProvider<MySingleDto> {
            
    @Override
    public List<Class<?>> getValidationGroups(MySingleDto request) {
        List<Class<?>> sequence = new ArrayList<>();
        sequence.add(MySingleDto.class);
        // other stuff ....        
        return sequence;
    }
}

And that should do it. But... If you are trying to reuse the same sequence on different types and that's why you had <Object> then I'd say it does sound like a reasonable use case, and we can work on a change to support it.
But since it'll be a breaking change (at least according to the javadocs, I've shared at the beginning of this comment) we'd only be able to try including such change in an upcoming 9.0 release.

[marko-bekhta]

Thanks
superseded by #1447