1:01 a.m.
consider
#validate_implicit_unwrapping_having_two_type_parameters_and_only_one_maximally_specific_value_extractor_marked_with_unwrap_by_default_is_ok():
What the spec says is:
...
If the constraint carries neither the Unwrapping.Unwrap nor the
Unwrapping.Skip payload:
If there is exactly one maximally-specific type-compliant value
extractor and this extractor is marked with @UnwrapByDefault, this
extractor is applied;
Otherwise, no value extractor is applied.
...
The test actually implements something more like:
If there is exactly one maximally-specific type-compliant value
extractor marked with @UnwrapByDefault, this extractor is applied,
etc.
The distinction is perhaps subtle, and I agree that the behavior
itself is reasonable; it just doesn't precisely agree with the current
wording, and I felt I should raise it as an issue.
Thanks,
Matt
2:46 p.m.
Hi Matt,
On Fri, Mar 30, 2018 at 1:01 AM, Matt Benson <mbenson(a)apache.org> wrote:
The distinction is perhaps subtle, and I agree that the behavior
itself is reasonable; it just doesn't precisely agree with the current
wording, and I felt I should raise it as an issue.
I remember us having discussions about this and I think the TCK tests are
the outcome of this discussion.
It looks like we haven't updated the spec though.
Not sure what to do about this one. The TCK tests make more sense than the
spec and IIRC that's what we decided to do.
--
Guillaume
3 p.m.
Can we just make the change in the source of the spec so that it will have
been handled should we do a 2.1 version? Failing that, could this
particular test be somehow marked as optional for an implementation to
pass? I've made the changes to permit it in the Apache implementation in
any case.
Matt
On Fri, Mar 30, 2018, 7:46 AM Guillaume Smet <guillaume.smet(a)gmail.com>
wrote:
Hi Matt,
On Fri, Mar 30, 2018 at 1:01 AM, Matt Benson <mbenson(a)apache.org> wrote:
> The distinction is perhaps subtle, and I agree that the behavior
> itself is reasonable; it just doesn't precisely agree with the current
> wording, and I felt I should raise it as an issue.
>
I remember us having discussions about this and I think the TCK tests are
the outcome of this discussion.
It looks like we haven't updated the spec though.
Not sure what to do about this one. The TCK tests make more sense than the
spec and IIRC that's what we decided to do.
--
Guillaume
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
4:06 p.m.
On Fri, Mar 30, 2018 at 3:00 PM, Matt Benson <mbenson(a)apache.org> wrote:
Can we just make the change in the source of the spec so that it will
have
been handled should we do a 2.1 version? Failing that, could this
particular test be somehow marked as optional for an implementation to
pass? I've made the changes to permit it in the Apache implementation in
any case.
Changing the spec for 2.1 would be my preference.
Let's wait for Gunnar insights. He's the master of the spec.
--
Guillaume
11:51 a.m.
That's an interesting one; it was originally worded exactly like Matt
suggests, but then we changed into the current version:
https://hibernate.atlassian.net/browse/BVAL-678.
I'm sympathetic to the current behaviour of that TCK test and to adjusting
the spec wording accordingly. Only you, Guillaume, didn't seem to like that
alternative as per our discussion on June 26th, in the Hibernate Validator
chat room. Perhaps you remember what made you dislike it back then?
2018-03-30 16:06 GMT+02:00 Guillaume Smet <guillaume.smet(a)gmail.com>:
On Fri, Mar 30, 2018 at 3:00 PM, Matt Benson
<mbenson(a)apache.org> wrote:
> Can we just make the change in the source of the spec so that it will
> have been handled should we do a 2.1 version? Failing that, could this
> particular test be somehow marked as optional for an implementation to
> pass? I've made the changes to permit it in the Apache implementation in
> any case.
>
Changing the spec for 2.1 would be my preference.
Let's wait for Gunnar insights. He's the master of the spec.
--
Guillaume
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
1:42 p.m.
On Sat, Mar 31, 2018 at 11:51 AM, Gunnar Morling <gunnar(a)hibernate.org>
wrote:
That's an interesting one; it was originally worded exactly like
Matt
suggests, but then we changed into the current version:
https://hibernate.atlassian.net/browse/BVAL-678.
I'm sympathetic to the current behaviour of that TCK test and to adjusting
the spec wording accordingly. Only you, Guillaume, didn't seem to like that
alternative as per our discussion on June 26th, in the Hibernate Validator
chat room. Perhaps you remember what made you dislike it back then?
Ah, yes, I remember now.
The paragraph is:
- If the constraint carries neither the Unwrapping.Unwrap nor the
Unwrapping.Skip payload:
- If there is exactly one maximally-specific type-compliant value
extractor and this extractor is marked with @UnwrapByDefault, this
extractor is applied;
- Otherwise, no value extractor is applied.
The initial wording "exactly one maximally-specific type-compliant value
extractor marked with @UnwrapByDefault" was not accurate because it was not
clear if we should determine the "maximally-specific type-compliant value
extractor" first and then check if it is @UnwrapByDefault. This is
important because, for a given type parameter, we want to get the maximally
specific value extractor first and then we want to check if it is marked
with @UnwrapByDefault. We don't want to get the maximally specific (value
extractor marked with @UnwrapByDefault).
We also miss a mention of the case when there are 2 maximally-specific
type-compliant value extractors marked with @UnwrapByDefault.
I think a wording more in line with our intentions would be:
- We determine the maximally-specific type-compliant value extractors.
- If there are no matching value extractors or none is marked with
@UnwrapByDefault, no value extractor is applied.
- If exactly one is marked with @UnwrapByDefault, this very extractor
is applied.
- If more than one are marked with @UnwrapByDefault, a
ConstraintDeclarationException is raised.
I would lean towards implementing it this way in the implementations and
fix it for BV 2.1.
--
Guillaume
2:19 p.m.
2018-03-31 13:42 GMT+02:00 Guillaume Smet <guillaume.smet(a)gmail.com>:
On Sat, Mar 31, 2018 at 11:51 AM, Gunnar Morling
<gunnar(a)hibernate.org>
wrote:
> That's an interesting one; it was originally worded exactly like Matt
> suggests, but then we changed into the current version:
> https://hibernate.atlassian.net/browse/BVAL-678.
>
> I'm sympathetic to the current behaviour of that TCK test and to
> adjusting the spec wording accordingly. Only you, Guillaume, didn't seem to
> like that alternative as per our discussion on June 26th, in the Hibernate
> Validator chat room. Perhaps you remember what made you dislike it back
> then?
>
Ah, yes, I remember now.
The paragraph is:
- If the constraint carries neither the Unwrapping.Unwrap nor the
Unwrapping.Skip payload:
- If there is exactly one maximally-specific type-compliant value
extractor and this extractor is marked with @UnwrapByDefault, this
extractor is applied;
- Otherwise, no value extractor is applied.
The initial wording "exactly one maximally-specific type-compliant value
extractor marked with @UnwrapByDefault" was not accurate because it was not
clear if we should determine the "maximally-specific type-compliant value
extractor" first and then check if it is @UnwrapByDefault. This is
important because, for a given type parameter, we want to get the maximally
specific value extractor first and then we want to check if it is marked
with @UnwrapByDefault. We don't want to get the maximally specific (value
extractor marked with @UnwrapByDefault).
We also miss a mention of the case when there are 2 maximally-specific
type-compliant value extractors marked with @UnwrapByDefault.
I think a wording more in line with our intentions would be:
- We determine the maximally-specific type-compliant value extractors.
- If there are no matching value extractors or none is marked with
@UnwrapByDefault, no value extractor is applied.
- If exactly one is marked with @UnwrapByDefault, this very
extractor is applied.
Ok so far.
- If more than one are marked with @UnwrapByDefault, a
ConstraintDeclarationException is raised.
I don't think we can or should do this, instead no extractor should be
applied
in this case. It'd contradict quite clearly the spec's wording
"Otherwise, no value extractor is applied." Also the idea was that
implicitly applied extractors should be applied implicitly if it's doable
unambiguously, but they shouldn't cause any sort of configuration exception
otherwise.
The original wording could be interpreted in the way above, but +1 for
making it more exhaustive. Can you log a BVAL issue for 2.1?
I would lean towards implementing it this way in the implementations
and
fix it for BV 2.1.
+1 apart from the case where there are multiple maximally-specific
type-compliant value extractors marked with @ UBD as discussed above.
--
Guillaume
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
12:56 p.m.
On Sat, Mar 31, 2018 at 2:19 PM, Gunnar Morling <gunnar(a)hibernate.org>
wrote:
> - If more than one are marked with @UnwrapByDefault, a
> ConstraintDeclarationException is raised.
>
> I don't think we can or should do this, instead no extractor should be
applied in this case. It'd contradict quite clearly the spec's wording
"Otherwise, no value extractor is applied." Also the idea was that
implicitly applied extractors should be applied implicitly if it's doable
unambiguously, but they shouldn't cause any sort of configuration exception
otherwise.
Yes, I'm stating that the spec is incorrect here and should be corrected.
In all the other cases when we have competing value extractors, we throw an
error. Why not in this case?
Typically, you would have your constraints applied on the container
elements, then you add another value extractor, and your constraints are
then applied silently to the container instead of the container elements.
That sounds like the wrong approach.
--
Guillaume
3:13 p.m.
2018-04-01 12:56 GMT+02:00 Guillaume Smet <guillaume.smet(a)gmail.com>:
On Sat, Mar 31, 2018 at 2:19 PM, Gunnar Morling
<gunnar(a)hibernate.org>
wrote:
>
>> - If more than one are marked with @UnwrapByDefault, a
>> ConstraintDeclarationException is raised.
>>
>> I don't think we can or should do this, instead no extractor should be
> applied in this case. It'd contradict quite clearly the spec's wording
> "Otherwise, no value extractor is applied." Also the idea was that
> implicitly applied extractors should be applied implicitly if it's doable
> unambiguously, but they shouldn't cause any sort of configuration exception
> otherwise.
>
Yes, I'm stating that the spec is incorrect here and should be corrected.
We quite clearly say that an exception is raised, so we cannot really
change that in the spec.
In all the other cases when we have competing value extractors, we throw an
error. Why not in this case?
The thinking was that *implicit* unwrapping should only be done if
unambiguously possible. I.e. this implicit process shouldn't cause a
configuration exception if it cannot be performed.
Typically, you would have your constraints applied on the container
elements, then you add another value extractor, and your constraints
are
then applied silently to the container instead of the container elements.
That sounds like the wrong approach.
I think that's just the nature of implicit application of extractors. You
also could say "you've applied constraints to the container, then you add a
value extractor, and your constraints are then applied silently to the
container elements". We went this route to make it comfortable (no Unwrap
payload needed) to apply constraints to non-generic wrappers such as
OptionalInt.
--
Guillaume
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
2211
days inactive
2219
days old
beanvalidation-dev@lists.jboss.org
8 comments
3 participants
participants (3)
-
Guillaume Smet -
Gunnar Morling -
Matt Benson