Hi experts, One of the remaining issues on our way to 1.1 is BVAL-336 [1], which is about what to return from Node#getElementDescriptor() for nodes added by the user via the ConstraintViolationBuilder API. Note that as per the spec only sub paths can be created by the user, i.e. user added nodes always represent properties but e.g. no methods or parameters. Based on that, it seems right to return the correct PropertyDescriptor in case a user adds a node for an existing property. Things get tricky though when nodes are added for non-existent properties. Emmanuel, Hardy and I identified the following options to address this situation: #1 Disallow adding non-existent nodes In case a user adds a node but no property with that names exist, an exception is thrown. The problem is elegantly avoided that way, but we might break some 1.0 user code (currently the spec is not really clear whether added nodes must exist or not). #2 return null from getElementDescriptor() When invoking getElementDescriptor() on a Node representing a non-existent property, null could be returned. This seems consistent (there is no element), but causes additional null checking when traversing a path. #3 return a PropertyDescriptor from getElementDescriptor() Since all user-added nodes represent properties, a PropertyDescriptor could be returned. hasConstraints() would return false, getConstraintDescriptors() the empty set etc. This would allow to handle all nodes and their descriptors uniformly when traversing a path. Question is what to return from PropertyDescriptor#getElementClass(). Options are to return null (signaling that the property is non-existent) or Object.class. #4 return a VirtualDescriptor from getElementDescriptor() We could create a new Kind, VIRTUAL, and an accompanying type VirtualDescriptor and return an instance of this. Behavior of the methods on the descriptor would be basically the same as in #3; getKind() returning Kind.VIRTUAL would allow for a very explicit checking whether the node exists or not. Any feedback on the options (ideally with arguments pro/con) or other alternatives are highly welcome. As life goes, Emmanuel's, Hardy's and my preferences are equally distributed between these options :) Thanks, --Gunnar [1] https://hibernate.onjira.com/browse/BVAL-336 _______________________________________________ beanvalidation-dev mailing list beanvalidation-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

At this time of day I do tend to like option #1 (ie making virtual nodes illegal). Otherwise my preferred option is #4 (VirtualElementDescriptor). Out of curiosity for people not preferring option #1. What use case do you see in having virtual nodes? Emmanuel On Fri 2013-02-08 11:08, Matt Benson wrote: > Hmm, a third option for the elementClass of such a descriptor might be > Void.class. > > I prefer option #3, followed by #4. > > Matt > > > On Fri, Feb 8, 2013 at 5:17 AM, Gunnar Morling <gunnar(a)hibernate.org&gt; wrote: > > > Hi experts, > > > > One of the remaining issues on our way to 1.1 is BVAL-336 [1], which is > > about what to return from Node#getElementDescriptor() for nodes added by > > the user via the ConstraintViolationBuilder API. > > > > Note that as per the spec only sub paths can be created by the user, i.e. > > user added nodes always represent properties but e.g. no methods or > > parameters. Based on that, it seems right to return the correct > > PropertyDescriptor in case a user adds a node for an existing property. > > > > Things get tricky though when nodes are added for non-existent properties. > > Emmanuel, Hardy and I identified the following options to address this > > situation: > > > > #1 Disallow adding non-existent nodes > > > > In case a user adds a node but no property with that names exist, an > > exception is thrown. The problem is elegantly avoided that way, but we > > might break some 1.0 user code (currently the spec is not really clear > > whether added nodes must exist or not). > > > > #2 return null from getElementDescriptor() > > > > When invoking getElementDescriptor() on a Node representing a non-existent > > property, null could be returned. This seems consistent (there is no > > element), but causes additional null checking when traversing a path. > > > > #3 return a PropertyDescriptor from getElementDescriptor() > > > > Since all user-added nodes represent properties, a PropertyDescriptor > > could be returned. hasConstraints() would return false, > > getConstraintDescriptors() the empty set etc. This would allow to handle > > all nodes and their descriptors uniformly when traversing a path. Question > > is what to return from PropertyDescriptor#getElementClass(). Options are to > > return null (signaling that the property is non-existent) or Object.class. > > > > #4 return a VirtualDescriptor from getElementDescriptor() > > > > We could create a new Kind, VIRTUAL, and an accompanying type > > VirtualDescriptor and return an instance of this. Behavior of the methods > > on the descriptor would be basically the same as in #3; getKind() returning > > Kind.VIRTUAL would allow for a very explicit checking whether the node > > exists or not. > > > > Any feedback on the options (ideally with arguments pro/con) or other > > alternatives are highly welcome. As life goes, Emmanuel's, Hardy's and my > > preferences are equally distributed between these options :) > > > > Thanks, > > > > --Gunnar > > > > [1] https://hibernate.onjira.com/browse/BVAL-336 > > > > > > _______________________________________________ > > 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

I had the same reserve as you but then reading the JavaDoc made me realize that the doc mentions properties so it is technically not as vague as I initially thought. Emmanuel On Fri 2013-02-08 11:48, Matt Benson wrote: > Personally my reason for eschewing option #1 is the possibility of breaking > existing code. > > Matt > > > On Fri, Feb 8, 2013 at 11:22 AM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt;wrote: > > > At this time of day I do tend to like option #1 (ie making virtual nodes > > illegal). Otherwise my preferred option is #4 > > (VirtualElementDescriptor). > > > > Out of curiosity for people not preferring option #1. What use case do > > you see in having virtual nodes? > > > > Emmanuel > > > > On Fri 2013-02-08 11:08, Matt Benson wrote: > > > Hmm, a third option for the elementClass of such a descriptor might be > > > Void.class. > > > > > > I prefer option #3, followed by #4. > > > > > > Matt > > > > > > > > > On Fri, Feb 8, 2013 at 5:17 AM, Gunnar Morling <gunnar(a)hibernate.org&gt; > > wrote: > > > > > > > Hi experts, > > > > > > > > One of the remaining issues on our way to 1.1 is BVAL-336 [1], which is > > > > about what to return from Node#getElementDescriptor() for nodes added > > by > > > > the user via the ConstraintViolationBuilder API. > > > > > > > > Note that as per the spec only sub paths can be created by the user, > > i.e. > > > > user added nodes always represent properties but e.g. no methods or > > > > parameters. Based on that, it seems right to return the correct > > > > PropertyDescriptor in case a user adds a node for an existing property. > > > > > > > > Things get tricky though when nodes are added for non-existent > > properties. > > > > Emmanuel, Hardy and I identified the following options to address this > > > > situation: > > > > > > > > #1 Disallow adding non-existent nodes > > > > > > > > In case a user adds a node but no property with that names exist, an > > > > exception is thrown. The problem is elegantly avoided that way, but we > > > > might break some 1.0 user code (currently the spec is not really clear > > > > whether added nodes must exist or not). > > > > > > > > #2 return null from getElementDescriptor() > > > > > > > > When invoking getElementDescriptor() on a Node representing a > > non-existent > > > > property, null could be returned. This seems consistent (there is no > > > > element), but causes additional null checking when traversing a path. > > > > > > > > #3 return a PropertyDescriptor from getElementDescriptor() > > > > > > > > Since all user-added nodes represent properties, a PropertyDescriptor > > > > could be returned. hasConstraints() would return false, > > > > getConstraintDescriptors() the empty set etc. This would allow to > > handle > > > > all nodes and their descriptors uniformly when traversing a path. > > Question > > > > is what to return from PropertyDescriptor#getElementClass(). Options > > are to > > > > return null (signaling that the property is non-existent) or > > Object.class. > > > > > > > > #4 return a VirtualDescriptor from getElementDescriptor() > > > > > > > > We could create a new Kind, VIRTUAL, and an accompanying type > > > > VirtualDescriptor and return an instance of this. Behavior of the > > methods > > > > on the descriptor would be basically the same as in #3; getKind() > > returning > > > > Kind.VIRTUAL would allow for a very explicit checking whether the node > > > > exists or not. > > > > > > > > Any feedback on the options (ideally with arguments pro/con) or other > > > > alternatives are highly welcome. As life goes, Emmanuel's, Hardy's and > > my > > > > preferences are equally distributed between these options :) > > > > > > > > Thanks, > > > > > > > > --Gunnar > > > > > > > > [1] https://hibernate.onjira.com/browse/BVAL-336 > > > > > > > > > > > > _______________________________________________ > > > > 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

My personal preference would be #1 as well. The docs don't say that non-existent nodes are allowed, so IMO it's ok to clarify now that it is not allowed. If code relied on adding non-existent nodes, it relied on behavior not guaranteed explicitly by the spec. --Gunnar 2013/2/8 Emmanuel Bernard <emmanuel(a)hibernate.org <mailto:emmanuel@hibernate.org>> I had the same reserve as you but then reading the JavaDoc made me realize that the doc mentions properties so it is technically not as vague as I initially thought. Emmanuel On Fri 2013-02-08 11:48, Matt Benson wrote: > Personally my reason for eschewing option #1 is the possibility of breaking > existing code. > > Matt > > > On Fri, Feb 8, 2013 at 11:22 AM, Emmanuel Bernard <emmanuel(a)hibernate.org <mailto:emmanuel@hibernate.org>>wrote: > > > At this time of day I do tend to like option #1 (ie making virtual nodes > > illegal). Otherwise my preferred option is #4 > > (VirtualElementDescriptor). > > > > Out of curiosity for people not preferring option #1. What use case do > > you see in having virtual nodes? > > > > Emmanuel > > > > On Fri 2013-02-08 11:08, Matt Benson wrote: > > > Hmm, a third option for the elementClass of such a descriptor might be > > > Void.class. > > > > > > I prefer option #3, followed by #4. > > > > > > Matt > > > > > > > > > On Fri, Feb 8, 2013 at 5:17 AM, Gunnar Morling <gunnar(a)hibernate.org <mailto:gunnar@hibernate.org>> > > wrote: > > > > > > > Hi experts, > > > > > > > > One of the remaining issues on our way to 1.1 is BVAL-336 [1], which is > > > > about what to return from Node#getElementDescriptor() for nodes added > > by > > > > the user via the ConstraintViolationBuilder API. > > > > > > > > Note that as per the spec only sub paths can be created by the user, > > i.e. > > > > user added nodes always represent properties but e.g. no methods or > > > > parameters. Based on that, it seems right to return the correct > > > > PropertyDescriptor in case a user adds a node for an existing property. > > > > > > > > Things get tricky though when nodes are added for non-existent > > properties. > > > > Emmanuel, Hardy and I identified the following options to address this > > > > situation: > > > > > > > > #1 Disallow adding non-existent nodes > > > > > > > > In case a user adds a node but no property with that names exist, an > > > > exception is thrown. The problem is elegantly avoided that way, but we > > > > might break some 1.0 user code (currently the spec is not really clear > > > > whether added nodes must exist or not). > > > > > > > > #2 return null from getElementDescriptor() > > > > > > > > When invoking getElementDescriptor() on a Node representing a > > non-existent > > > > property, null could be returned. This seems consistent (there is no > > > > element), but causes additional null checking when traversing a path. > > > > > > > > #3 return a PropertyDescriptor from getElementDescriptor() > > > > > > > > Since all user-added nodes represent properties, a PropertyDescriptor > > > > could be returned. hasConstraints() would return false, > > > > getConstraintDescriptors() the empty set etc. This would allow to > > handle > > > > all nodes and their descriptors uniformly when traversing a path. > > Question > > > > is what to return from PropertyDescriptor#getElementClass(). Options > > are to > > > > return null (signaling that the property is non-existent) or > > Object.class. > > > > > > > > #4 return a VirtualDescriptor from getElementDescriptor() > > > > > > > > We could create a new Kind, VIRTUAL, and an accompanying type > > > > VirtualDescriptor and return an instance of this. Behavior of the > > methods > > > > on the descriptor would be basically the same as in #3; getKind() > > returning > > > > Kind.VIRTUAL would allow for a very explicit checking whether the node > > > > exists or not. > > > > > > > > Any feedback on the options (ideally with arguments pro/con) or other > > > > alternatives are highly welcome. As life goes, Emmanuel's, Hardy's and > > my > > > > preferences are equally distributed between these options :) > > > > > > > > Thanks, > > > > > > > > --Gunnar > > > > > > > > [1] https://hibernate.onjira.com/browse/BVAL-336 > > > > > > > >

+1 for option #1. I never likes the fact that you could add arbitrary nodes (even though I have seen code doing it). If we not go for #1, I prefer #2 - null from getElementDescriptor(). I don't see the point intruding another virtual descriptor where not all methods are clearly defined or need to be defined with constructed values. In this case I prefer to return null directly which of course means that there is one more null check if you are going to use this API. On the other hand, I don't see this API used much by actual end users. It is probably more important for integrators of BV. In this case an additional null check is acceptable imo. --Hardy On 9 Jan 2013, at 3:01 PM, Sebastian Thomschke <sebastian.thomschke(a)web.de&gt; wrote: > I agree with Gunnar. I'm for option #1 as well. > > seb > > On 09.02.2013 12:12, Gunnar Morling wrote: >> My personal preference would be #1 as well. >> >> The docs don't say that non-existent nodes are allowed, so IMO it's ok to clarify now that it is not allowed. If code relied on adding non-existent nodes, it relied on behavior not guaranteed explicitly by the spec. >> >> --Gunnar >> >> >> >> 2013/2/8 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; >> I had the same reserve as you but then reading the JavaDoc made me >> realize that the doc mentions properties so it is technically not as >> vague as I initially thought. >> >> Emmanuel >> >> On Fri 2013-02-08 11:48, Matt Benson wrote: >> > Personally my reason for eschewing option #1 is the possibility of breaking >> > existing code. >> > >> > Matt >> > >> > >> > On Fri, Feb 8, 2013 at 11:22 AM, Emmanuel Bernard <emmanuel(a)hibernate.org&gt;wrote: >> > >> > > At this time of day I do tend to like option #1 (ie making virtual nodes >> > > illegal). Otherwise my preferred option is #4 >> > > (VirtualElementDescriptor). >> > > >> > > Out of curiosity for people not preferring option #1. What use case do >> > > you see in having virtual nodes? >> > > >> > > Emmanuel >> > > >> > > On Fri 2013-02-08 11:08, Matt Benson wrote: >> > > > Hmm, a third option for the elementClass of such a descriptor might be >> > > > Void.class. >> > > > >> > > > I prefer option #3, followed by #4. >> > > > >> > > > Matt >> > > > >> > > > >> > > > On Fri, Feb 8, 2013 at 5:17 AM, Gunnar Morling <gunnar(a)hibernate.org&gt; >> > > wrote: >> > > > >> > > > > Hi experts, >> > > > > >> > > > > One of the remaining issues on our way to 1.1 is BVAL-336 [1], which is >> > > > > about what to return from Node#getElementDescriptor() for nodes added >> > > by >> > > > > the user via the ConstraintViolationBuilder API. >> > > > > >> > > > > Note that as per the spec only sub paths can be created by the user, >> > > i.e. >> > > > > user added nodes always represent properties but e.g. no methods or >> > > > > parameters. Based on that, it seems right to return the correct >> > > > > PropertyDescriptor in case a user adds a node for an existing property. >> > > > > >> > > > > Things get tricky though when nodes are added for non-existent >> > > properties. >> > > > > Emmanuel, Hardy and I identified the following options to address this >> > > > > situation: >> > > > > >> > > > > #1 Disallow adding non-existent nodes >> > > > > >> > > > > In case a user adds a node but no property with that names exist, an >> > > > > exception is thrown. The problem is elegantly avoided that way, but we >> > > > > might break some 1.0 user code (currently the spec is not really clear >> > > > > whether added nodes must exist or not). >> > > > > >> > > > > #2 return null from getElementDescriptor() >> > > > > >> > > > > When invoking getElementDescriptor() on a Node representing a >> > > non-existent >> > > > > property, null could be returned. This seems consistent (there is no >> > > > > element), but causes additional null checking when traversing a path. >> > > > > >> > > > > #3 return a PropertyDescriptor from getElementDescriptor() >> > > > > >> > > > > Since all user-added nodes represent properties, a PropertyDescriptor >> > > > > could be returned. hasConstraints() would return false, >> > > > > getConstraintDescriptors() the empty set etc. This would allow to >> > > handle >> > > > > all nodes and their descriptors uniformly when traversing a path. >> > > Question >> > > > > is what to return from PropertyDescriptor#getElementClass(). Options >> > > are to >> > > > > return null (signaling that the property is non-existent) or >> > > Object.class. >> > > > > >> > > > > #4 return a VirtualDescriptor from getElementDescriptor() >> > > > > >> > > > > We could create a new Kind, VIRTUAL, and an accompanying type >> > > > > VirtualDescriptor and return an instance of this. Behavior of the >> > > methods >> > > > > on the descriptor would be basically the same as in #3; getKind() >> > > returning >> > > > > Kind.VIRTUAL would allow for a very explicit checking whether the node >> > > > > exists or not. >> > > > > >> > > > > Any feedback on the options (ideally with arguments pro/con) or other >> > > > > alternatives are highly welcome. As life goes, Emmanuel's, Hardy's and >> > > my >> > > > > preferences are equally distributed between these options :) >> > > > > >> > > > > Thanks, >> > > > > >> > > > > --Gunnar >> > > > > >> > > > > [1] https://hibernate.onjira.com/browse/BVAL-336 >> > > > > >> > > > > > > _______________________________________________ > 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