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

[ http://opensource.atlassian.com/projects/hibernate/browse/HV-296?page=com... ] 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} -- 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.... - For more information on JIRA, see: http://www.atlassian.com/software/jira