Hi, > Another approach would be to make NodeBuilderCustomizableContext a sub insterface of NodeBuilderDefinedContext. I think changing the return type of ConstraintViolationBuilder#addNode() to NodeBuilderCustomizableContext is a problem. While that change is source compatible (code using addNode() can be compiled against the new API version without having to be adapted) it is not binary compatible. That is, code compiled against the previous version will fail at runtime: java.lang.NoSuchMethodError: javax.validation.ConstraintValidatorContext$ConstraintViolationBuilder.addNode(Ljava/lang/String;)Ljavax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext; This error occurs, no matter wether the result of addNode() is assigned to a variable or not. What we might try to do is to return an intersection type from ConstraintViolationBuilder#addNode() like this: interface ConstraintViolationBuilder { <T extends NodeBuilderDefinedContext & NodeBuilderCustomizableContext> T addNode(String name); ConstraintValidatorContext addConstraintViolation(); } This is source compatible and AFAICS it's also binary compatible because the erasure of T is its leftmost bound, that is NodeBuilderDefinedContext. That way code compiled against the old API can still dispatch the addNode() method, while code written against the new version can invoke all methods defined on NodeBuilderDefinedContext and NodeBuilderCustomizableContext. Btw. I stumbled upon [1] which gives a very good overview on the compatibility of all sorts of API changes. --Gunnar [1] http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces 2012/11/22 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > Unless anyone has concern, I plan to go for option 3 on > http://beanvalidation.org/proposals/BVAL-221/ > _______________________________________________ > beanvalidation-dev mailing list > beanvalidation-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev >

_______________________________________________ beanvalidation-dev mailing list beanvalidation-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

> - relaxing NodeBuilderDefinedContext and adding the new contract on it I wouldn't really like that as it reduces safety of the API.

> - an explicit new method deprecating addNode Related to this is BVAL-336 [1]. We've added Node#getDescriptor(), so the question is what should that return for nodes created by the user via the node builder API? We could have a new method addNode(String name, ElementDescriptor descriptor) or even addNode(ElementDescriptor descriptor) Leaves the question where does the user get the descriptor from? One way could be to provide access to BeanDescriptor via ConstraintValidatorContext. For nodes added via the deprecated method getDescriptor() we would likely have to return null.

> - relaxing NodeBuilderDefinedContext and adding the new contract on it I wouldn't really like that as it reduces safety of the API. > - an explicit new method deprecating addNode Related to this is BVAL-336 [1]. We've added Node#getDescriptor(), so the question is what should that return for nodes created by the user via the node builder API? We could have a new method addNode(String name, ElementDescriptor descriptor) or even addNode(ElementDescriptor descriptor) Leaves the question where does the user get the descriptor from? One way could be to provide access to BeanDescriptor via ConstraintValidatorContext. For nodes added via the deprecated method getDescriptor() we would likely have to return null. --Gunnar [1] https://hibernate.onjira.com/browse/BVAL-336 2012/11/23 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > I guess you always learn something new every day :) > This is indeed only working if we use interception types. > > The question is do we want to do that or lean towards one of the > alternative methods: > > - an explicit new method deprecating addNode > - relaxing NodeBuilderDefinedContext and adding the new contract on it > > Emmanuel > > On Fri 2012-11-23 12:57, Gunnar Morling wrote: > > Hi, > > > > > Another approach would be to make NodeBuilderCustomizableContext a sub > > insterface of NodeBuilderDefinedContext. > > > > I think changing the return type of ConstraintViolationBuilder#addNode() > > to NodeBuilderCustomizableContext is a problem. > > > > While that change is source compatible (code using addNode() can be > > compiled against the new API version without having to be adapted) it is > > not binary compatible. That is, code compiled against the previous > version > > will fail at runtime: > > > > java.lang.NoSuchMethodError: > > > javax.validation.ConstraintValidatorContext$ConstraintViolationBuilder.addNode(Ljava/lang/String;)Ljavax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext; > > > > This error occurs, no matter wether the result of addNode() is assigned > to > > a variable or not. > > > > What we might try to do is to return an intersection type from > > ConstraintViolationBuilder#addNode() like this: > > > > interface ConstraintViolationBuilder { > > > > <T extends NodeBuilderDefinedContext & > NodeBuilderCustomizableContext> > > T addNode(String name); > > > > ConstraintValidatorContext addConstraintViolation(); > > } > > > > This is source compatible and AFAICS it's also binary compatible because > > the erasure of T is its leftmost bound, that is > NodeBuilderDefinedContext. > > That way code compiled against the old API can still dispatch the > addNode() > > method, while code written against the new version can invoke all methods > > defined on NodeBuilderDefinedContext and NodeBuilderCustomizableContext. > > > > Btw. I stumbled upon [1] which gives a very good overview on the > > compatibility of all sorts of API changes. > > > > --Gunnar > > > > [1] > > > http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces > > > > > > 2012/11/22 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > > > > > Unless anyone has concern, I plan to go for option 3 on > > > http://beanvalidation.org/proposals/BVAL-221/ > > > _______________________________________________ > > > beanvalidation-dev mailing list > > > beanvalidation-dev(a)lists.jboss.org > > > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev > > > > > > _______________________________________________ > > beanvalidation-dev mailing list > > beanvalidation-dev(a)lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev > > _______________________________________________ > beanvalidation-dev mailing list > beanvalidation-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev >

_______________________________________________ beanvalidation-dev mailing list beanvalidation-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

On 23 Jan 2012, at 2:48 PM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: > The question is do we want to do that or lean towards one of the > alternative methods: > > - an explicit new method deprecating addNode > - relaxing NodeBuilderDefinedContext and adding the new contract on it In the proposal you are saying about relaxing NodeBuilderDefinedContext: "But that would break the contextual and safety of the API as this interface is also returned by atKey and atIndex." Can you provide an example? Also in the proposal you are saying that: context.buildConstraintViolationWithTemplate("oh noes") .addNode(null) //ie the contained element inIterable().atKey(someKey) .addConstraintViolation(); would be the natural solution. Why is ".addNode(null)" natural and why could we not have inIterable() directly on ConstraintViolationBuilder? --Hardy _______________________________________________ beanvalidation-dev mailing list beanvalidation-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

Hi, Thanks for the clarification. Indeed I was forgetting about the special meaning of null in this case (should have read section 5.2 of the spec). In this context adding addNode(null) is indeed "natural" in the sense that it strictly follows the spec. That said, it still looks awkward to me. Couldn't there be for example a constant so that you could do something like addNode(CLASS_LEVEL). I find this much more telling. In this light option 4 - adding a different method - of your proposed solution on http://beanvalidation.org/proposals/BVAL-221/ sounds more appealing again. What about adding addClassLevelNode()? --Hardy On 4 Jan 2012, at 4:42 PM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: > These are good questions > > ## Natural way to do it? > > Assuming > > @HasCorrectHomeAddressFromTop > class Order { > @HasCorrectHomeAddressFromUser > User user; > } > > class User { > @CollectionNotEmpty > @HasCorrectHomeAddressFromAddresses > Map<String,Address> addresses; > } > > When `HasCorrectHomeAddressFromTop` is validated, you can attach a > `ConstraintViolation` to Order.user.addresses["home"] with the > following call and it works in BV 1.0 > This is how the API is supposed to be used. > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > .addNode( "user" ) > .addNode( "addresses" ) > .inIterable().atKey( "home" ) > .addConstraintViolation(); > > following this logic, when `HasCorrectHomeAddressFromUser` is validated, > you can attach a `ConstraintViolation` with the following call but that > does not work in BV 1.0. > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > .addNode( "addresses" ) > .inIterable().atKey( "home" ) > .addConstraintViolation(); > > When `HasCorrectHomeAddressFromAddresses` is validated, you can attach a > `ConstraintViolation` with the following call but that does not work > in BV 1.0. > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > .addNode( null ) > .inIterable().atKey( "home" ) > .addConstraintViolation(); > > Not that we use null as we add a class level constraint violation to the > element in `addresses["home"]`. > > For you info, when `CollectionNotEmpty` is validated, you can attach a > `ConstraintViolation` with the following call. > > context.buildConstraintViolationWithTemplate( "collection empty" ) > .addConstraintViolation(); > > ## Loss in type-safety > > If we add `inIterable()` to `NodeBuilderDefinedContext` we solve our > problem but we introduce a leak in the type safety. I could do the > following calls > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > .addNode( null ) > .inIterable().atKey( "home" ) > .inIterable().atKey( "wc" ) > .addConstraintViolation(); > > This call is not correct as no node has been (explicitly) created > between the two inIterable() calls. The correct call would be > > context.buildConstraintViolationWithTemplate( "Incorrect WC location" ) > .addNode( null ) > .inIterable().atKey( "home" ) > .addNode( null ) > .inIterable().atKey( "wc" ) > .addConstraintViolation(); > > Here we are assuming that the `addresses` elements contain map. > > We could add in the spec that subsequent calls to `inIterable()` > implicitly create a node without name. > > The problem is not as critical as I imagined but that would require > adding some magic rules wrt node creation. > > Also imagine a user willing to validate a property of Address, could he > forget to call the node creation method? > > Emmanuel > > On Mon 2012-11-26 13:07, Hardy Ferentschik wrote: >> >> On 23 Jan 2012, at 2:48 PM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: >> >>> The question is do we want to do that or lean towards one of the >>> alternative methods: >>> >>> - an explicit new method deprecating addNode >>> - relaxing NodeBuilderDefinedContext and adding the new contract on it >> >> In the proposal you are saying about relaxing NodeBuilderDefinedContext: "But that would break the contextual >> and safety of the API as this interface is also returned by atKey and atIndex." >> Can you provide an example? >> >> Also in the proposal you are saying that: >> >> context.buildConstraintViolationWithTemplate("oh noes") >> .addNode(null) //ie the contained element >> inIterable().atKey(someKey) >> .addConstraintViolation(); >> >> would be the natural solution. Why is ".addNode(null)" natural and why could we not have inIterable() directly >> on ConstraintViolationBuilder? >> >> --Hardy >> >> >> _______________________________________________ >> beanvalidation-dev mailing list >> beanvalidation-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev > _______________________________________________ > beanvalidation-dev mailing list > beanvalidation-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev _______________________________________________ beanvalidation-dev mailing list beanvalidation-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

Regarding BVAL-221, wouldn't that be fixed by the intersection type approach I suggested? Or do you think it we shouldn't pursue it because its too magic or uncommon? 2012/12/5 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > addNode(CLASS_LEVEL) is not a bad idea. The alternative is a full method > as proposed in BVAL-191. > > Unfortunately, we cannot solve the problem with this method because of > the second use case I describe in the email below. This example does not > work even if does not make use of addNode(null) :( > > Emmanuel > > On Wed 2012-12-05 14:10, Hardy Ferentschik wrote: > > Hi, > > > > Thanks for the clarification. Indeed I was forgetting about the special meaning of null in this case (should have read section 5.2 of the spec). > > In this context adding addNode(null) is indeed "natural" in the sense that it strictly follows the spec. That said, it still looks awkward to me. > > Couldn't there be for example a constant so that you could do something like addNode(CLASS_LEVEL). I find this much more telling. > > In this light option 4 - adding a different method - of your proposed solution on http://beanvalidation.org/proposals/BVAL-221/ sounds more appealing again. > > What about adding addClassLevelNode()? > > > > --Hardy > > > > > > > > On 4 Jan 2012, at 4:42 PM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: > > > > > These are good questions > > > > > > ## Natural way to do it? > > > > > > Assuming > > > > > > @HasCorrectHomeAddressFromTop > > > class Order { > > > @HasCorrectHomeAddressFromUser > > > User user; > > > } > > > > > > class User { > > > @CollectionNotEmpty > > > @HasCorrectHomeAddressFromAddresses > > > Map<String,Address> addresses; > > > } > > > > > > When `HasCorrectHomeAddressFromTop` is validated, you can attach a > > > `ConstraintViolation` to Order.user.addresses["home"] with the > > > following call and it works in BV 1.0 > > > This is how the API is supposed to be used. > > > > > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > > > .addNode( "user" ) > > > .addNode( "addresses" ) > > > .inIterable().atKey( "home" ) > > > .addConstraintViolation(); > > > > > > following this logic, when `HasCorrectHomeAddressFromUser` is validated, > > > you can attach a `ConstraintViolation` with the following call but that > > > does not work in BV 1.0. > > > > > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > > > .addNode( "addresses" ) > > > .inIterable().atKey( "home" ) > > > .addConstraintViolation(); > > > > > > When `HasCorrectHomeAddressFromAddresses` is validated, you can attach a > > > `ConstraintViolation` with the following call but that does not work > > > in BV 1.0. > > > > > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > > > .addNode( null ) > > > .inIterable().atKey( "home" ) > > > .addConstraintViolation(); > > > > > > Not that we use null as we add a class level constraint violation to the > > > element in `addresses["home"]`. > > > > > > For you info, when `CollectionNotEmpty` is validated, you can attach a > > > `ConstraintViolation` with the following call. > > > > > > context.buildConstraintViolationWithTemplate( "collection empty" ) > > > .addConstraintViolation(); > > > > > > ## Loss in type-safety > > > > > > If we add `inIterable()` to `NodeBuilderDefinedContext` we solve our > > > problem but we introduce a leak in the type safety. I could do the > > > following calls > > > > > > context.buildConstraintViolationWithTemplate( "Incorrect home address" ) > > > .addNode( null ) > > > .inIterable().atKey( "home" ) > > > .inIterable().atKey( "wc" ) > > > .addConstraintViolation(); > > > > > > This call is not correct as no node has been (explicitly) created > > > between the two inIterable() calls. The correct call would be > > > > > > context.buildConstraintViolationWithTemplate( "Incorrect WC location" ) > > > .addNode( null ) > > > .inIterable().atKey( "home" ) > > > .addNode( null ) > > > .inIterable().atKey( "wc" ) > > > .addConstraintViolation(); > > > > > > Here we are assuming that the `addresses` elements contain map. > > > > > > We could add in the spec that subsequent calls to `inIterable()` > > > implicitly create a node without name. > > > > > > The problem is not as critical as I imagined but that would require > > > adding some magic rules wrt node creation. > > > > > > Also imagine a user willing to validate a property of Address, could he > > > forget to call the node creation method? > > > > > > Emmanuel > > > > > > On Mon 2012-11-26 13:07, Hardy Ferentschik wrote: > > >> > > >> On 23 Jan 2012, at 2:48 PM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: > > >> > > >>> The question is do we want to do that or lean towards one of the > > >>> alternative methods: > > >>> > > >>> - an explicit new method deprecating addNode > > >>> - relaxing NodeBuilderDefinedContext and adding the new contract on it > > >> > > >> In the proposal you are saying about relaxing NodeBuilderDefinedContext: "But that would break the contextual > > >> and safety of the API as this interface is also returned by atKey and atIndex." > > >> Can you provide an example? > > >> > > >> Also in the proposal you are saying that: > > >> > > >> context.buildConstraintViolationWithTemplate("oh noes") > > >> .addNode(null) //ie the contained element > > >> inIterable().atKey(someKey) > > >> .addConstraintViolation(); > > >> > > >> would be the natural solution. Why is ".addNode(null)" natural and why could we not have inIterable() directly > > >> on ConstraintViolationBuilder? > > >> > > >> --Hardy > > >> > > >> > > >> _______________________________________________ > > >> beanvalidation-dev mailing list > > >> beanvalidation-dev(a)lists.jboss.org > > >> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev > > > _______________________________________________ > > > beanvalidation-dev mailing list > > > beanvalidation-dev(a)lists.jboss.org > > > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev > > > > > > _______________________________________________ > > beanvalidation-dev mailing list > > beanvalidation-dev(a)lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev > _______________________________________________ > beanvalidation-dev mailing list > beanvalidation-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev _______________________________________________ beanvalidation-dev mailing list beanvalidation-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

If we test is and it works I am fine with it.

If we test is and it works I am fine with it.