[hibernate-issues] [Hibernate-JIRA] Commented: (HV-296) Apply constraints on the elements of an Iterable instance

Wed May 5 07:27:06 EDT 2010

    [ http://opensource.atlassian.com/projects/hibernate/browse/HV-296?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=36908#action_36908 ] 

Hardy Ferentschik commented on HV-296:
--------------------------------------

Added transcript of discussion between Emmanuel, Gunnar and Hardy about how to best implement this feature for HV:
{noformat}
9:05pm] epbernard: so let's get started and over with 
[9:05pm] epbernard: Let me try and summarize
[9:05pm] hardy_: let's get the link up  BVAL-202
[9:05pm] jbossbot: [BVAL-202] Apply constraints on the elements of an iterator [Open, Major, Hardy Ferentschik] http://opensource.atlassian.com/projects/hibernate/BVAL-202
[9:05pm] epbernard: (your new toy) the correct solution is to way for JDK 7 (hopefully) and get finer grained annotations
[9:06pm] epbernard: but we wnat to offer the feature now to users
[9:06pm] epbernard: at least in Hibernate Validator
[9:06pm] epbernard: (do we have a corresponding HV issue number btw?)
[9:06pm] gunnar__1: yep ... are we sure that JSR 308 comes with Java 7?
[9:06pm] epbernard: no we are not sure
[9:06pm] gunnar__1: HV-296
[9:06pm] jbossbot: [HV-296] Apply constraints on the elements of an Iterable instance [Open, Major, Hardy Ferentschik] http://opensource.atlassian.com/projects/hibernate/HV-296
[9:06pm] epbernard: Considering that, we ahev several approaches
[9:07pm] epbernard: #! use a special group as a placeholder
[9:07pm] epbernard: #1bis use payload as a placeholder
[9:07pm] hardy_: personally using groups or payload feels wrong
[9:07pm] epbernard: #2 use a specific validElements attribute to store that (though not allowed by the spec as valid is a reserved prefix)
[9:08pm] epbernard: #3 do some automagic and apply on elements for constraints not applicable on Iterable<?> and subclasses
[9:08pm] gunnar__1: #3 fails for things like @size ...
[9:08pm] epbernard: #4, create a dedicated annotation for elements constraints
[9:09pm] hardy_: there is some sort of solution #5
[9:10pm] epbernard: am I missing a solution?
[9:10pm] epbernard: hardy_: hit us
[9:10pm] hardy_: let people implement custom validators where they need them and maybe provide some our of the box where it makes sense
[9:10pm] epbernard: ah right
[9:10pm] gunnar__1: #4 splits into two solutions i think
[9:11pm] gunnar__1: having an inner anntation with all the attributes of the parent of have an inner that holds instances of the parent
[9:11pm] hardy_: gunnar__1: right, that was your latest suggestion
[9:12pm] hardy_: regarding my #5 - HV-264 was the issue which triggered this discussion
[9:12pm] jbossbot: [HV-264] Implement ConstraintValidator<Email, Collection<String>> [Open, Major, Hardy Ferentschik] http://opensource.atlassian.com/projects/hibernate/HV-264
[9:12pm] gunnar__1: wouldn't like #5 too much as this problem seems like very common so bascally many users would to the same
[9:12pm] epbernard: right #5 is out of the table for me
[9:12pm] epbernard: it's an existing but annoying solution
[9:13pm] gunnar__1: personally i feel it should be one from #4
[9:13pm] epbernard: #3 is pretty fragile and fails for a few like @NotNull and @Size
[9:13pm] hardy_: i am not sure, at least it is working with the current framework/spec and we are waiting for a true solution
[9:13pm] epbernard: #1 is bad as groups have a very specific semantic
[9:14pm] hardy_: if not #5 I also tend to #4a or #4b
[9:14pm] epbernard: I kinda like #1bis as payloads are here for extensions
[9:14pm] epbernard: and #1bis does nto force people to write specific constraints
[9:14pm] epbernard: (ie the annotations wrappers)
[9:15pm] gunnar__1: yep, if #1 then with payload
[9:15pm] hardy_: agreed
[9:15pm] epbernard: the problem with #1 is that it will fail on another BV provider
[9:15pm] epbernard: Which is not idea 
[9:15pm] gunnar__1: #2 seems pretty good if integrated into BV
[9:16pm] epbernard: As far as #4, it creates a bunch of work for us (including creating the wrapper for all the built-in constraints
[9:16pm] hardy_: it would also avoid the problem that we somehow would have to get this inner annotations into the constraints. How would be do that with the BV spec constraints?
[9:16pm] sjacobs__ joined the chat room.
[9:16pm] epbernard: and leaves one question opened: how do I create my @ValidateEach annotation?
[9:16pm] sjacobs__ left the chat room. (Read error: Connection reset by peer)
[9:17pm] hardy_: #2 btw has the same problem as #4 - how do we make this happen with the BV constraints
[9:17pm] epbernard: is it by convention
[9:17pm] epbernard: or is the annotation meta annotated with some Hibernate Validator specific annotations
[9:17pm] sjacobs__ joined the chat room.
[9:17pm] gunnar__1: yep #2 is the right thing when tackling this in the spec
[9:17pm] epbernard: #2 violates the spec. I'd rather avoid it
[9:17pm] sjacobs__ left the chat room. (Read error: Connection reset by peer)
[9:17pm] gunnar__1: dont see a good way to do this now for HV
[9:18pm] sjacobs__ joined the chat room.
[9:18pm] epbernard: gunnar__1: you don't see a good way at all or on a specific problem?
[9:19pm] gunnar__1: i would like #2 to be part of the spec. for now I wouldnt know a good way to put this new attribute into the existing BV annotatoins
[9:19pm] hardy_: it seems to me that the only solution which does not cause some spec headache is the payload one (#1b)
[9:19pm] gunnar__1: without providing a pimped BV.jar of course 
[9:19pm] hardy_: autsch
[9:20pm] epbernard: http://pastebin.org/201668
[9:20pm] sjacobs_ left the chat room. (Ping timeout: 252 seconds)
[9:21pm] epbernard: gunnar__1: nan you can't change the bv.jar  and spec wise, using a vlid* attribute in a constraint is not legal (reserved to the EG)
[9:21pm] sjacobs joined the chat room.
[9:21pm] sjacobs__ left the chat room. (Read error: Operation timed out)
[9:21pm] hardy_: epbernard: so one ValidateEach for each constraint?
[9:21pm] epbernard: hardy_: that's the problem yes
[9:22pm] gunnar__1: yep, thats what i mean. solution #2 would be great if it was a part of the official bv spec
[9:22pm] gunnar__1: but its out for an ad-hoc solutioin for HV i think
[9:22pm] gunnar__1: hardy_: yes unfortunately
[9:23pm] epbernard: reasonably we have #4 http://pastebin.org/201668 or #1bis http://pastebin.org/201673
[9:23pm] gunnar__1: its really a pity that one cant have something like @interface ValidateEach { Annotation[] value(); }
[9:23pm] gunnar__1: this would have made a great proposal for project coin
[9:24pm] epbernard: ok here is what we're going to do: propose #4 and #1bis to the community via a blog and see the arguments
[9:24pm] epbernard: but before that, let's see the best approach for #4
[9:24pm] hardy_: we could have a single ValidateEach with one attribute for each constraint type
[9:24pm] epbernard: hardy_: custom constraints?
[9:25pm] gunnar__1: we could provide this for the BV constraints
[9:25pm] hardy_: are in issue either way, right?
[9:25pm] gunnar__1: but what would the attributes names?
[9:25pm] hardy_: we could allow to provide a custom ValidateEach together with the custom constraint
[9:26pm] hardy_: but the one HV would provide would contain attributes for all builtin constraints
[9:26pm] epbernard: try it on pastebin
[9:26pm] epbernard: but I think it dpes not look good
[9:26pm] epbernard: and I am not a fan of multiple solutions for the same problem
[9:27pm] epbernard: and what about non built-in custom constraints not written by the app developer?
[9:27pm] gunnar__1: dont see that as a problem. its in the interest of the constraint developer to provide everything. same as with @List
[9:28pm] hardy_: http://pastebin.com/08aUZri6
[9:29pm] epbernard: http://pastebin.org/201696
[9:29pm] hardy_: it's not the nicest, but an alternative to the one ValidateEach annotation for each
[9:30pm] gunnar__1: thats 4#a, isnt it?
[9:30pm] epbernard: gunnar__1: who are you talking to?
[9:30pm] gunnar__1: sorry. to you epbernar
[9:31pm] gunnar__1: I think this causes kind a lot of work for constraint developers
[9:31pm] epbernard: gunnar__1: that's #4a and #4b (two approaches are depicted)
[9:31pm] gunnar__1: yep. wanted to say "approach 2" from pastebin is #4a
[9:31pm] epbernard: I think approach 1 is less intrusive IMO
[9:32pm] epbernard: Though it's likely to be @ValidateEachSize(@Size) @ValidateEachNotNull(@NotNull) etc
[9:33pm] epbernard: or we would need a different package name per built-in constraint
[9:33pm] hardy_: we probably would also need another marker annotation
[9:33pm] epbernard: hardy_: so hardy for your solution, what would be the approach for custom constraints?
[9:33pm] hardy_: for custom constraints
[9:35pm] hardy_: somehow you would have to know that ValidateEachXYZ is a annotation indication that XYX has to be applied to each element of an iterable
[9:35pm] epbernard: well not if these annotations are annotated with @ValidateElements
[9:36pm] gunnar__1: isnt that the same as @List
[9:36pm] gunnar__1: an annotation having an array-typed value of constraint annotations
[9:37pm] epbernard: gunnar__1: no @List does not need a meta annotation, the spec implies that it's considered as all the annotations in the array
[9:37pm] epbernard: but that means that #4 is not 100% spec compliant either 
[9:37pm] epbernard: both #1bis and #4 fail with another BV provider
[9:38pm] gunnar__1: oh yes ... it would be detected as multi-valued constraint 
[9:38pm] epbernard: if we don't find a way out of this, I prefer the payload approach
[9:38pm] epbernard: much simpler
[9:38pm] gunnar__1: seems like that
[9:38pm] epbernard: I guess I scrwed up by not requiring a @List meta annotation 
[9:39pm] hardy_: payload is less intrusive
[9:39pm] epbernard: lemme read the spec
[9:40pm] gunnar__1: and actually we'd be using it the intended way ... implementing provider-specific extensions
[9:41pm] hardy_: hmm, except a payload means for me that a value is just carried through the system without any specific menaing to it
[9:41pm] epbernard: ah sothe spec says that arrays of constraints whose attribute name is value should be considered as a multivalue annotation
[9:41pm] hardy_: it's then the cient which applies meaning/semantics
[9:42pm] epbernard: @Apply(onElements=@Size(min=30)) is valid as per the spec
[9:42pm] gunnar__1: @hardy_: ok, but i wouldn't consider it to bad if HV adds a meaning to payload
[9:43pm] hardy_: i guess i could live with it as well
[9:43pm] hardy_: it's just not 100% compliant with my understanding of a payload 
[9:43pm] epbernard: hardy_: where does it say that actually?
[9:43pm] gunnar__1: well, its a payload for the provider 
[9:43pm] hardy_:
[9:43pm] epbernard: the spec says Payloads are typically non-portable.
[9:43pm] hardy_: if you put it this way
[9:44pm] hardy_: epbernard: which is fair enough right? It is a HV specific feature
[9:44pm] gunnar__1: epbernard: the @Apply approach would work, but I think using it would become pretty lengthy
[9:45pm] epbernard: hardy_: right. but to be honest I am a little bothered by the non portability across providers as well
[9:45pm] epbernard: let's propose botha pproaches in a blog with detailed pros and cons
[9:45pm] epbernard: and shoot based on feedback
[9:45pm] epbernard: hardy_: take the lead on that?
[9:45pm] hardy_: ok
[9:46pm] gunnar__1: sounds fine to me
[9:46pm] epbernard: cool 
[9:46pm] hardy_: btw, how would any of the annotation based approaches be portable? They are not either
[9:46pm] epbernard: @Apply is
[9:46pm] epbernard: ahh right
[9:46pm] epbernard: theya re not portable
[9:47pm] epbernard: but they don't *change* the meaning of existing annotations
[9:47pm] gunnar__1: you couldn't compile without HV
[9:47pm] epbernard: so some constraints would not be applied
[9:47pm] epbernard: but they would not change semantic
[9:47pm] hardy_: right
[9:47pm] gustavo joined the chat room.
[9:47pm] epbernard: (hum actually if HV is not in the CP, the payload class being unresolvable, the constraint would not apply either. This needs testing)
[9:48pm] hardy_: for me this is just a tmp solution, since hopefully the true solution would be JSR 308 further down the track
[9:48pm] epbernard: (but if HV and some other provider are both in the CP then we could ahve different semantics for the same constraint)
[9:48pm] hardy_: so our approach should be as unintrusive as possible
[9:48pm] epbernard: I agree
{noformat}

> Apply constraints on the elements of an Iterable instance
> ---------------------------------------------------------
>
>                 Key: HV-296
>                 URL: http://opensource.atlassian.com/projects/hibernate/browse/HV-296
>             Project: Hibernate Validator
>          Issue Type: New Feature
>          Components: engine
>    Affects Versions: 4.0.2.GA
>            Reporter: Hardy Ferentschik
>            Assignee: Hardy Ferentschik
>             Fix For: 4.1.0
>
>
> See BVAL-202

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://opensource.atlassian.com/projects/hibernate/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira