Bugzilla – Full Text Bug Listing
| Summary: | Investigate support for Constraints directly annotated to Ecore | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Tristan Faure <faure.tristan> |
| Component: | Core | Assignee: | Kenn Hussey <Kenn.Hussey> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | achim.demelt, adolfosbh, alexander.igdalov, Alfredo.Bencomo, andrew987650000, Andy.Carpenter, arash.aghevli, boris.gruschko, davidms, ed, give.a.damus, greg.fenton, janreimone, Kenn.Hussey, mgil |
| Version: | 2.5.0 | Keywords: | plan |
| Target Milestone: | M2 | Flags: | Kenn.Hussey:
helios+
|
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 191689, 286329 | ||
| Attachments: | |||
Created attachment 118244 [details]
the patch for the feature
Created attachment 118245 [details]
The GOOD patch for the feature
i want to precise the way to fill constraints
it's not
EClass : A
EAnnotation : Ecore -> constraint1 constraint2
factory -> factoryID
but
EClass : A
EAnnotation : Ecore
constraints -> constraint1 constraint2
factory -> factoryID
sorry for the error
Created attachment 118338 [details]
some improvements
improvements constraints inheritance is managed
for each element the software checks constraints annotated to eSuperTypes
this can be disabled with this option :
EClass : A
EAnnotation : Ecore
constraints -> constraint1 constraint2
factory -> factoryID
inheritsConstraints -> false
with this option (by default to true) you can specify if you want to apply constraints inherited
the behaviour is all or nothing you can't specify if you want to inherit from esuperTypes 1 or 2
Created attachment 123880 [details]
new version with last EMF version
this version adds some improvements for inheritance management
Created attachment 123881 [details]
simple plugin for ocl
this plugin adds a factory for constraints delegator and checks OCL rules
Created attachment 123883 [details]
a metamodel and dynamic instance
this zip contains one ecore file : the metamodel with very simple constraints
and one dynamic instance to check it
Created attachment 123885 [details]
some improvements
permits to user to stop inheritance for an element and to not define constraints :
example with no constraints
Element e1 inherits e
Ecore
inheritsConstraints --> false
Hi
i would ask for (maybe) modifying the design
Currently :
for each ECLass
Eclass A
EAnnotation : Ecore
constraints -> constraint1 constraint2
factory -> factoryID
EAnnotation : factoryID
constraint1 -> String interpreted by constraint delegator
EAnnotation : factoryID
constraint2 -> String interpreted by constraint delegator
Eclass B
EAnnotation : Ecore
constraints -> constraint1 constraint2
factory -> factoryID
EAnnotation : factoryID
constraint1 -> String interpreted by constraint delegator
EAnnotation : factoryID
constraint2 -> String interpreted by constraint delegator
Do you think filling factory for each ECLass is irritating ? maybe we should consider an EAnnotation in EPackage defining Factory example :
EPackage
Eannotation : Ecore
factory -> factoryID#Key
Eclass A
EAnnotation : Ecore
constraints -> constraint1 constraint2
EAnnotation : Key
constraint1 -> String interpreted by constraint delegator
EAnnotation : Key
constraint2 -> String interpreted by constraint delegator
Eclass B
EAnnotation : Ecore
constraints -> constraint1 constraint2
EAnnotation : Key
constraint1 -> String interpreted by constraint delegator
EAnnotation : Key
constraint2 -> String interpreted by constraint delegator
In this example I used the notation with the Key but we keep the factory id
what is your opinion ?
Created attachment 126724 [details]
improvements and new management of constraint
Created attachment 126725 [details]
a new version of ocl checker for user constraints
i changed the management of constraints regarding to newsgroup posts : the new version to enter constraints :
EPackage packageName
Eannotation
source : http://www.eclipse.org/emf/2002/Ecore
factory : nameOfThefactory#key
...
EClass aClassToPutConstraints
Eannotation
source : nameOfTheFactory // or key
constraint : text interpreted by factory
name : the name of the constraint
message : the message to display if problems
severity : WARNING // or ERROR
inheritsConstraints : true // default to true or false
the only key neccessary is constraint
if others are not filled default messages are added
if you don't want to put constraints but stopping inheritance
you can just only pu inheritsConstraints to false
for each constraint add an eannotation with source setted to key or factoryName
Created attachment 126726 [details]
correct errors with many constraints on one Eclass
Created attachment 126727 [details]
a new version of the meta model tested
Created attachment 126729 [details]
problems during merge with remote sources this version is ok
Created attachment 146761 [details]
updated patch based on HEAD
Here's a more complete set of patches, based on HEAD, which includes support for both static and dynamic constraint/invariant delegation.
Created attachment 146762 [details]
sample delegate implementation for OCL
Created attachment 146778 [details]
updated test model
After discussing this with Ed, we felt it would be better to take a simplified (but not altogether different) approach. The patch I just attached makes use of the following annotations:
EPackage : ?
EAnnotation : Ecore
validationDelegates-><uri1> <uri2>
EClass : ?
EAnnotation : Ecore
constraints-><c1> <c2>
EAnnotation : <uri1>
<c1>-><body1>
<c2>-><body2>
EOperation : ?
EAnnotation : <uri2>
body-><body>
To keep support for invariants and constraints consistent, we decided not to support specification of severity or message at this time; if modelers want to change those (for static models) they can customize the generated code.
The issue of whether or not constraints are "inherited" is also not addressed, in the interest of simplicity.
Feedback is welcome and appreciated.
Created attachment 146787 [details]
another test model, which includes an invariant
Hi no problem for me with the new approach and for inheritance i just added this to add options for user. i have no trouble with removing it Created attachment 147341 [details]
updated patch
Updated based on feedback from Ed:
- capitalized loop label names
- added a parameter to validation methods to facilitate caching/lookup of compiled expressions by delegates
Hi Kenn, I have a couple of bigger comments for your consideration. Then, once you've addressed or dismissed them, I have a few little niggles to point out. 1. I notice that ValidationDelegateRegistryReader.readElement() is trying to remove a String from a URI-typed map. So, at the very least, I think that's an error. But it gets me wondering whether it should be a Map<URI, Object> in the first place. I don't see any point at which you're doing any kind of URI computation (e.g. resolve) or component access, so I wonder what the overhead of constructing URIs buys. I suspect a Map<String, Object> would be simpler and more appropriate. 2. It seems to me like we're generating a whole lot of code that doesn't really do much. It was one thing when we're expecting people to replace those methods in order to actually implement the constraint or invariant, but when the idea is to have it all work without touching the validator, it seems a bit much to me. I wonder if the validate and invariant methods could be generated as a call to a convenience method that invokes the validation delegate? I'd imagine the only thing that would be tricky to do there would be to create a nice diagnostic in the case where the delegate returns false. After all, a generic message for the non-found and exception cases would probably be suffice. And that leads me to wonder if the delegate should be creating the diagnostic when it returns false. It does have access to the diagnostic chain, and it might have better knowledge of how or why the expression failed to be satisfied. Perhaps passing it the source and code would be helpful then, too. I haven't convinced myself of this, but I'm interested in your reaction. As an alternative, maybe it would be good just to allow the user to control whether to generate the validator in the first place. Then, if they didn't want all the "useless" generated code, they could let the EObjectValidator handle the delegation generically. The more I think about this, the less I like my idea of having the delegate create the diagnostic, and the more I like the idea of having a method that implements the get delegate/delegate validation/create diagnostic logic as necessary. At least then it's only one thing to override. While I'm at it, here are my smaller comments: Should EcoreUtil have add a setValidationDelegates() for completeness? Tools might want to use this in creating a model. New public and protected methods should be tagged @since 2.6 in EcoreUtil, EObjectValidator, GenClassifier, and GenOperation. The warning in Diagnositician.validate() has been reintroduced (the annotation suppressing it has been removed). How about a method on GenOperation to form the expression variable name used in Class.javajet, line 1331? It strikes me as a really long, error-prone string to assemble in the template (especially with the conditional parameter types), but that's just my opinion. (In reply to comment #23) Thanks, Dave, for the insightful feedback. I agree with both #1 and #2, and will attach updates patches shortly (after which I'll move on to the "niggles"). > And that leads me to wonder if the delegate should be creating the diagnostic > when it returns false. It does have access to the diagnostic chain, and it > might have better knowledge of how or why the expression failed to be > satisfied. Perhaps passing it the source and code would be helpful then, too. I considered doing that (as the original patch did), but after looking at the implementation implications and discussing it in detail with Ed, decided against it. It seems better that the validation delegate focus on evaluating the expression (only) and that the validator has control over how failures are reported/handled, including the severity, etc.. > As an alternative, maybe it would be good just to allow the user to control > whether to generate the validator in the first place. Then, if they didn't want > all the "useless" generated code, they could let the EObjectValidator handle > the delegation generically. I think I prefer the first suggestion, i.e. reduce the amount of generated code. (In reply to comment #24) > The more I think about this, the less I like my idea of having the delegate > create the diagnostic, and the more I like the idea of having a method that > implements the get delegate/delegate validation/create diagnostic logic as > necessary. At least then it's only one thing to override. Agreed. Will we need a CQ for Tristan's original contribution? (In reply to comment #25) > Should EcoreUtil have add a setValidationDelegates() for completeness? Tools > might want to use this in creating a model. Yes. > New public and protected methods should be tagged @since 2.6 in EcoreUtil, > EObjectValidator, GenClassifier, and GenOperation. OK. > The warning in Diagnositician.validate() has been reintroduced (the annotation > suppressing it has been removed). Odd. I get a warning in my environment that the suppression is unnecessary. In any case, I'll add it back for now. > How about a method on GenOperation to form the expression variable name used in > Class.javajet, line 1331? It strikes me as a really long, error-prone string to > assemble in the template (especially with the conditional parameter types), but > that's just my opinion. I'd rather leave this until we address bug 255469, since we'll need to add constants for operation metadata anyway. (In reply to comment #28) > Will we need a CQ for Tristan's original contribution? I was thinking we wouldn't since I created my changes from scratch. Created attachment 147449 [details]
final (hopefully) patch
Dave, here's an updated patch which attempts to address both your bigger comments and your niggles. Please let me know what you think.
Created attachment 147455 [details]
updated sample OCL validation delegate
Here is an updated validation delegate implementation for OCL based on the modified EValidator.ValidationDelegate interface.
My final comments on your latest patch... I hope I didn't lead you astray before: I didn't mean to suggest that you should stop referring to the string identifying a validation delegate as a URI, just that the URI class isn't needed to represent it (this would be similar to how we represent an nsURI everywhere, including EPackage.Registry, as a string). Is your intent that people will use the more restricted Eclipse-style IDs, instead of the conventional URI form? My hunch is you still expect people to use URIs (since they're also used as the source of the other annotations), so I'd recommend going back to the name uri. Also, whatever name you do use, I'd suggest using the same (or a related) name in EcoreUtil.setValidationDelegates(). Right now, the parameter's name is constraints, but its not a list of constraint names (e.g. "nameIsToto"); it's a list of validation delegate URIs (e.g. "http://www.eclipse.org/ocl/3.0/OCL"). Finally, could the validate() implementation in EObjectValidator be non-static? Then it would be overrideable in generated validator classes. It might be nice to have a single place that the diagnostic creating could be customized for invariants, too, but I guess that would involve yet another level of indirection. Created attachment 147479 [details]
final patch
Here is a final set of updates that addresses the remaining concerns. Please confirm that the changes are OK.
Did you say final? ;-) I noticed just two things: Class.javajet has a //$NON-NLS-1$ instead of a <%=genModel.getNonNLS()%> on line 1394. ValidatorClass.javajet has unimported parameter types on line 115. Otherwise, it all looks really great. (In reply to comment #35) Great eyes, Dave! I'll make those small changes and commit the code soon. (In reply to comment #28) > Will we need a CQ for Tristan's original contribution? Hi I've seen this is not needed according to the discussion but what is CQ ? Hi Tristan, CQ stands for Contribution Questionnaire, and is part of the Eclipse intellectual property process. It's basically a form that needs to be filled out when a code contribution is accepted from a non-committer into an Eclipse project, to ensure that the contributor has the rights to that code and is willing to release it under the EPL. (In reply to comment #38) > Hi Tristan, > > CQ stands for Contribution Questionnaire, and is part of the Eclipse > intellectual property process. It's basically a form that needs to be filled > out when a code contribution is accepted from a non-committer into an Eclipse > project, to ensure that the contributor has the rights to that code and is > willing to release it under the EPL. Thank you ! The changes have been committed to CVS. Which CVS? This is an EMF core bug, but I see no new EMF core plugin corresponding to your example. I doubt that EMF core should have an OCL dependency. The attachment seemed to demonstrate a very simple addition that can be merged into the MDT/OCL code. I see no MDT/OCL CVS changes. (In reply to comment #41) > Which CVS? > > This is an EMF core bug, but I see no new EMF core plugin corresponding to your > example. I doubt that EMF core should have an OCL dependency. > > The attachment seemed to demonstrate a very simple addition that can be merged > into the MDT/OCL code. I see no MDT/OCL CVS changes. No changes have been committed for MDT OCL; this would have to be done against a separate (OCL-specific) bug. Note that, while the example works, a more robust implementation which included caching of previously compiled expressions should probably be provided for OCL. Fix available in HEAD: 2.6.0M2 (S200909211047). |
Build ID: I20081030-1917 new feature to permit plugins to implement their constraints language More information: as we discussed in newsgroup The feature adds the possibility for a developer to add constraint language if he adds an extension point referencing a class wiwh implements the factory : ConstraintsDelegator.Factory. The factories are loaded As the developer povides a factory (no default factory is provided) The user can add a constraint in its metamodel (currently only in EClass we can discuss if it's enough) for example EClass : A EAnnotation : Ecore -> constraint1 constraint2 factory -> factoryID EAnnotation : factoryID constraint1 -> String interpreted by constraint delegator EAnnotation : factoryID constraint2 -> String interpreted by constraint delegator if the id is too long and unusable in the first EAnnotation the user can use this syntax : EClass : A EAnnotation : Ecore -> constraint1 constraint2 factory -> factoryID#Key EAnnotation : Key constraint1 -> String interpreted by constraint delegator EAnnotation : Key constraint2 -> String interpreted by constraint delegator As i test, the solution works with dynamic instance and generated code because the constraints are checked in EObjectValidator I already test for example in a constraint to put a qualified class name which is instanciated in the factory, this kind of behaviour can be useful. but as the class needs to be integrated to environment i think with this behaviour (class interpretation)dynamic instances will not check the constraint. I don't provide OCL implementation i know it's possible but I prefer offers the possibility to another developer which will check the feature If somebody can check my work and discuss with me about what is good or bad it will be a great pleasure