added default to from in ConvertGroup to Default

[lpandzic]

As per comments in BVAL-493, I've added default to ConvertGroup - Default.

For use cases where group conversion is done from Default this change will simplify code:

@ConvertGroup(from = Default.class, to = SecondLevelCheck.class)

can be changed to

@ConvertGroup(to = SecondLevelCheck.class).

[hendrikebbers]

Hi,
I ask myself why not to and from should have Default.class as default value?
In addition it would be good to add some JavaDoc to both of them since this is still missing.

[Hibernate-CI]

Can one of the admins add this person to the trusted builders? (reply with: "add to whitelist" or "ok to test")

[lpandzic]

Added Default to 'to'.
Added javadoc to both to and from.

[hendrikebbers]

@lpandzic Cool 👍

[lpandzic]

Are there any blockers to merging this PR?

[emmanuelbernard]

I am actually concerned and borderline -1 yo having defaults to both.
This is a complex feature and I am not certain defaulting helps people using it.

In particular the double default means I can write @ConvertGroup with no computer complaint.

[emmanuelbernard]

Btw, to merge the PR, we also need the corresponding PR for the spec and tck update for that change

[hendrikebbers]

mmh, I think converting from or to Default.class could make sense, right?
And at all writing @ConvertGroup() isn't wrong. Like @ConvertGroup(from= GroupA.class, to= GroupA.class) ... At the end it won't make much sense to do it that way but it should not be a problem. Maybe we can add a hint to the javadoc.
just my 5 cents

[emmanuelbernard]

Well that's a noop which does not make much functional sense. Hence my reluctance. If we have to fix a problem with JavaDoc where the type system could help, I'm reluctant to go there.

[lpandzic]

Ok, so should I revert back only to 'to' default?

For spec -> jakartaee/validation-spec#93

I'm not sure what to change on tck.

[emmanuelbernard]

Yes I would revert back to just to.
@gunnarmorling anything to change on the TCK?

[gunnarmorling]

I'm negative on having default values for both, from() and to() as the annotation gets more error-prone to be used with that. It allows to write a plain @ConvertGroup without from() and to() at all, which seems not desirable.

I can see how having the default value for from() (not to()!) being set to Default.class is helpful, as it kind of optimizes the likely most common case of converting the default group. Though even then I'm favoring the explicit usage of both parameters as it increases readability.

Re-reading the discussion on BVAL-493, I'm wondering whether the entire issue isn't an attempt of re-using @ConvertGroup for something it hasn't been meant for. The original issue mentions Spring's @Validated which seems to serve a different purpose, though. Instead of hi-jacking @ConvertGroup, should we rather add a groups() parameter to @ValidateOnExecution (which seems to correspond to @Validated)?

I think the confusion comes from the fact that Spring kind of mis-uses @Valid to trigger validation in addition to its original usage, the control of cascading validation of objects. The same should not be done for @ConvertGroup.

[emmanuelbernard]

@gunnarmorling @ValidateOnExecution is global to the method or constructor whereas Spring's @Validated is per parameter and also method level (so a bit undefined and not supporting return value overriding).

But maybe both @ConvertGroup for parameter / return value level and @ValidateOnExecution for method overriding would work. What is your use case for method level group overriding though? Some cross domain model object transversal group? Some example would be interesting.

[lpandzic]

Here is an example:

Service

X create(@NotNull @Valid @ConvertGroup(from = Default.class, to = Create.class)) X x);
X update(@NotNull @Valid @ConvertGroup(from = Default.class, to = Update.class)) X x)

Model

class X {
    @Null(groups = {Create.class})
    @NotNull(groups = {Update.class})
    private final Long id;
    ...
}

What I'd like to be able to do is:

X create(@NotNull @Valid @ConvertGroup(to = Create.class)) X x);
X update(@NotNull @Valid @ConvertGroup(to = Update.class)) X x)

[lpandzic]

The original issue mentions Spring's @validated which seems to serve a different purpose, though. Instead of hi-jacking @ConvertGroup, should we rather add a groups() parameter to @ValidateOnExecution (which seems to correspond to @validated)?

I'm also fine with that, maybe we should continue this discussion on the issue itself?

[gunnarmorling]

Hi, I admit to feel guilty about forgetting about this one for such a long time. We are very close to the Final release, and it seems too late to explore an expansion of @ValidateOnExecution. I could still image though to make Default.class the default value for @ConvertGroup#from() which seems like a reasonable change either way. @emmanuelbernard, any thoughts on doing this?

[gunnarmorling]

I'm going to close this in favor of #129 which makes Default.class the default value for @ConvertGroup as originally suggested. It remains to be done for BV 2.1 to explore a new annotation (or expanding @ValidateOnExecution) for specifying the groups to be validated during method validation. But making the most common case of converting the default group into something else a bit easier is a good improvement regardless. Thanks for hanging in on this on!