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@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@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@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
> > > >
> > > >