Closed eclipse-ocl-bot closed 1 month ago
By Axel Uhl on Dec 08, 2010 08:17
(In reply to comment #100)
The com.google stuff is in Orbit, so you can install Orbit, but it should appear automatically if you install Xtext or MDT/OCL Examples.
Thanks for the hint. Now I can build the patch.
First a few minor issues:
Is there any specific reason you didn't apply the DI pattern to the extent map, e.g., in EcoreEvaluationEnvironment.createExtentMap(Object)?
Why does EcoreEvaluationEnvironment.createOppositeEndFinder still use "new DefaultOppositeEndFinder(...)? I assume it should be the same as in Ecoreenvironment.createOppositeEndFinder(), right?
createOppositeEndFinder() can probably be private as all constructors will have to invoke it due to the final declaration of oppositeEndFinder.
My major concern, however, is this: the injector is centrally configured for the single instance of the OCLPlugin, first come, first serve. This works for the two classes and their single instances OCLPlugin and OCLEcorePlugin. I don't (yet?) see how this would work if different clients of the OCLEcorePlugin wanted to use it with varying behavior. For example, some downstream bundles may want to use it with the DefaultOppositeEndProvider, some may want to use a Query2OppositeEndProvider. Similarly, some may want to use the LazyExtentMap, others the OppositeEndFinder-consistent ExtentMap.
With a single injector fixed by the first client to execute functionality that requires an injector (see your assertion "injector==null" in OCLEcorePlugin.createInjector) there can only be one system-wide configuration, not controllable by any contexts. Once created, an Injector can't be changed.
So, while the approach you chose to DI may work to manage the two different configurations required for Ecore vs UML, it most certainly doesn't work for different (potentially concurrent) runtime configurations of the single OCLEcorePlugin. This is why I really would like to have my overlladed constructors back.
By Axel Uhl on Dec 08, 2010 08:33
(In reply to comment #97)
Created an attachment (id=184753) [details] Using Dependency Injection
Other things I suggested in my latest patch went away, too, such as the additional o.e.o.ecore.utilities.AbstractVisitor constructor taking a default result.
I suggest to leave the entire DI approach to 4.0.0, do it with reasonable scoping and @Inject annotations there, make sure an injector is used throughout for instance creation and allowing for different concurrent configurations along different call stacks. Without this, constructors are needed which allow for different configurations of the o.e.o.[ecore|uml].OCL instances used.
By Axel Uhl on Dec 08, 2010 10:59
(In reply to comment #97)\ Another issue with the way DI is being used: OCLEcorePlugin is in an internal package, so I can't easily get access to its createInjector(Module...) operation so as to configure it specifically.
By Axel Uhl on Dec 08, 2010 11:05
(In reply to comment #97)\ If I try to specialize OppositeEndFinder.IProvider in our Query2OppositeEndFinder, I'm facing more difficulties because the Query2OppositeEndFinder constructor takes more arguments, particularly a QueryContextProvider object that is query2-specific. With the createOppositeEndFinder(Registry) interface operation and without further injection capabilities due to the lack of consistent @Inject usage, I don't see how I could instantiate the class appropriately.
Can we please go back to no-DI for the moment?
By Ed Willink on Dec 09, 2010 01:40
(In reply to comment #102)
Other things I suggested in my latest patch went away, too, such as the additional o.e.o.ecore.utilities.AbstractVisitor constructor taking a default result.
Indeed. Addition of the constructor was not part of the contribution and does not appear to be necessary for it. I did not see any comment that explained why it had been added. Please do not add further functionality to reviewed code. It costs me a lot of time to check where we are and investigate what else is changing. If an additional constructor is useful, please raise a separate Bugzilla.
I suggest to leave the entire DI approach to 4.0.0, do it with reasonable scoping and @Inject annotations there, make sure an injector is used throughout for instance creation and allowing for different concurrent configurations along different call stacks. Without this, constructors are needed which allow for different configurations of the o.e.o.[ecore|uml].OCL instances used.
Yes. The DI approach is interesting, in particular the elimination of the need for an EnvironmentExtension, but as you pointed out my attempts to make the injector environment-specific are incomplete and do not extend to the EvaluationEnvironment. (OCL.createInjector delegates to OCLECorePlugin.createInjector).
I investigated the DI approach now as a possible solution to your keenness for OppositeFinder variability.
Clearly considerable work is needed to make DI enhance existing functionality while maintaining compatibility for applications that do non-DI environment construction.
I asked if there is a specific use case for an alternate DefaultOppositeEndFinder. Unless there is such a use case then I think we can leave out the extra constructors and bundle the flexibility with the general problem of making Environment a flexible configurable service provider.
In order to make progress can you comment on what should be different to Attachment 184519 rather than what could be different.
I misunderstood your comment on #70 as a change to use LazyExtentMap. I'll look at this area again, since OCL.setExtentMap seems undesirable too.
By Axel Uhl on Dec 09, 2010 02:19
(In reply to comment #105)
Ed,
(In reply to comment #102)
Other things I suggested in my latest patch went away, too, such as the additional o.e.o.ecore.utilities.AbstractVisitor constructor taking a default result.
Indeed. Addition of the constructor was not part of the contribution and does not appear to be necessary for it. I did not see any comment that explained why it had been added. Please do not add further functionality to reviewed code.
in https://bugs.eclipse.org/bugs/show_bug.cgi?id=251621#c94 I wrote "Furthermore, I added constructors to o.e.o.ecore.AbstractVisitor so as to\ enable passing on a result to the base class constructor." It is required to pass a default result to the superclass constructor.
It costs me a lot of time to check where we are and investigate what else is changing. If an additional constructor is useful, please raise a separate Bugzilla.
No, this one is a simple, tiny change that belongs there. I can't see why this should be raised in another Bugzilla, particularly since it's in a new class anyway where there is not even backward compatibility issues. And please don't forget that it also costs a lot of time to prepare the patches.
I asked if there is a specific use case for an alternate DefaultOppositeEndFinder. Unless there is such a use case then I think we can leave out the extra constructors and bundle the flexibility with the general problem of making Environment a flexible configurable service provider.
\ I described several times that query2 is the use case and that the current DefaultOppositeEndFinder can only navigate what's loaded in a ResourceSet which is a random collection of Resources so far navigated by a client.
In order to make progress can you comment on what should be different to Attachment 184519 [details] rather than what could be different.
I did that in my comment 96 on https://bugs.eclipse.org/bugs/attachment.cgi?id=184716
By Ed Willink on Dec 09, 2010 05:43
AbstractVisitor; sorry my mistake. The constructors are obviously good. I thought they were being added to old rather than new code.
OCL.setExtentMap(). No problem. This seems to be the standard way of doing things. Your alternative HashMap usefully demonstrates how unnecessarily wide this interface. We can certainly think in terms of something much narrower.
I'm learning a lot about the OCL code here. I think I now understand the construction design and why we have a conflict between your specific solution and my desire for a scalable solution. EnvironmentFactory is not just a factory for an Environment but also for things that an Environment may need such as an EvaluationEnvironment. In effect EnvironmentFactory is a poor man's dependency injector.
The problem is that changes in the last few years ignored this design and so createOCLAnalyzer and createTypeResolver made Environment rather than its factory the creation focus. You have emulated the recent practice.
Fortunately EnvironmentFactory is clearly Javadoc'd as do not implement. This can now be strengthened with an explicit @noimplement, and we can add the missing createValidationVisitor etc to EnvironmentFactory without API violation. No need for DI or EnvironmentExtension or getAdapter.
Unfortunately the EnvironmentFactory policy does not extend to EcoreEnvironmentFactory which is an implementation not interface. I'm therefore introducing the EcoreEnvironmentFactoryInterface for createOppositeFinder, which makes OppositeEndFinder configurable by derived EnvironmentFactory passed to OCL.createEnvironment().
Unfortunately again the EnvironmentFactory policy does not extend to the EvaluationEnvironment which is why you need an extra constructor to coordinate the oppositeEndFinder. So yes we do need an extra constructor, but with an EnvironmentFactory argument so that it scales to solve a variety of problems rather than just opposite ends.
I think that this extra constructor should take an Environment rather than EnvironmentFactory argument, since an EvaluationEnvironment seems to be always created from an Environment via its factory, so losing traceability from the EvaluationEnvironment back to its Environment is sometimes awkward. So the new EvaluationEnvirionment(Environment) constructor can be accompanied by deprecation of the old.
I hope to post a patch with this in this evening. (By resurrecting EnvironmentFactory, we provide a focus for DI if we get round to it.)
Thanks for helping to expose the design drift; sorry it's been so painful (for both of us).
By Ed Willink on Dec 09, 2010 14:52
Created attachment 184896 EnvironmentFactory-based creation
Attached
a) uses latest CVS and so resolves Bug 331143 clashes b) locates new create functionality in EnvironmentFactory
EcoreEvaluationEnvironment now behaves more like UMLEvaluationEnvironment in taking a factory argument that allows the eval env to getOppositeEndFinder from the factory where the same instance can be shared by both env and evalEnv.
I hope that this is now ready for submission for IP approval.
:notepad_spiral: Bug251621.patch
By Axel Uhl on Dec 10, 2010 11:30
(In reply to comment #108)
Created an attachment (id=184896) [details] EnvironmentFactory-based creation
Attached
a) uses latest CVS and so resolves Bug 331143 clashes b) locates new create functionality in EnvironmentFactory
EcoreEvaluationEnvironment now behaves more like UMLEvaluationEnvironment in taking a factory argument that allows the eval env to getOppositeEndFinder from the factory where the same instance can be shared by both env and evalEnv.
I hope that this is now ready for submission for IP approval.
Look very good. I was able to produce a specialized factory in a separate bundle that allows us to parameterize conveniently with OppositeEndFinder instances and which by default uses ExtentMap instead of LazyExtentMap. (If anyone is interested, I can post this as another attachment here, or later commit it to the examples section.)
All our impact analysis stuff works again, based on patch 184896. Let's move this on.
Thanks for all your efforts.
By Ed Willink on Dec 12, 2010 06:49
Submitted for IP approval as http://dev.eclipse.org/ipzilla/show_bug.cgi?id=4693.
By Ed Willink on Dec 12, 2010 06:52
Bug 330045 has been marked as a duplicate of this bug.
By Axel Uhl on Dec 14, 2010 17:12
For IP Review:\ I authored 100% of the original patch. Ed Willink revised a few areas, leading up to the final patch https://bugs.eclipse.org/bugs/attachment.cgi?id=184896. I make the code available under the EPL. SAP AG as my employer permits this contribution.
By Adolfo Sanchez-Barbudo Herrera on Dec 15, 2010 04:47
Ed,
Since it's not the first time that occurs, what about if Axel re-attatch the last patch so that you will only have to ip-log+ the contribution rather than the bugzilla entry. I think that it's better for the automatic ip-log gatherer. Remember that when being elected for the MDT/OCL team extra people was candide because they only add some comments in the bugzilla.
Regards,\ Adolfo.
By Ed Willink on Dec 15, 2010 11:47
Comment on attachment 182910 Icon for OppositePropertyCallExp
See if it's the GIF that inhibits IP log appearance
OppositePropertyCallExp.gif
By Ed Willink on Dec 15, 2010 11:48
Comment on attachment 184716 (attachment deleted)\
In addition to Ed's simplification allows for consistent passing of OppositeEndFinder
See if this shows Axel's contribution
By Ed Willink on Dec 15, 2010 11:49
See if the whole bug as iplog will show Axel in the IP log.
By Ed Willink on Dec 15, 2010 12:02
IP: I'm baffled, putting iplog+ on kenns obsolete Buig 331143 cleans that bug up and shows Kennas the contributor.
Nothing seems to be preapred to let Axel appear in the log! Maybe there's a delay. I'll try again tomorrow.
By Ed Willink on Dec 15, 2010 12:35
Thanks. Committed to HEAD for 3.1.0M5.
Axel: If http://www.eclipse.org/projects/ip_log.php?projectid=modeling.mdt.ocl does not credit you with a contribution on this bug tomorrow, please re-attach the final patch, and mark the other non-GIF patches as obsolete.
By Ed Willink on Dec 15, 2010 13:09
(In reply to comment #117)
IP: I'm baffled, putting iplog+ on kenns obsolete Buig 331143 cleans that bug up and shows Kennas the contributor.
Nothing seems to be preapred to let Axel appear in the log! Maybe there's a delay. I'll try again tomorrow.
Duh! Of course nothing will appear until the Bugzilla is RESOLVED!
By Ed Willink on May 29, 2012 13:22
Closing all bugs resolved in Indigo.
By Ed Willink on Sep 29, 2013 18:26
Finally in Bug 418282 we have a killer case for OppositePropertyCallExp.
| --- | --- | | Bugzilla Link | 251621 | | Status | CLOSED FIXED | | Importance | P3 enhancement | | Reported | Oct 21, 2008 17:08 EDT | | Modified | Sep 29, 2013 18:26 EDT | | Blocks | 318248 | | Reporter | Ed Willink |
Description
Bug #245586 started with a discussion of whether OCL should or should not assist QVT in using unnamed opposites in EMOF; the discussion was inconclusive and diverted to refactoring simpleNameCS to allow a derived simplePropertyNameCS which solves the problem within QVTr.
I have just started writing the missing validation constraints for QVTr using my IMP-based MDT OCL text editor. So I'm writing pure MDT OCL expressions referring to the QVT meta-model which is defined in Ecore/EMOF.
I find that I cannot use the implicit toLeadingLower name to navigate a default-named relationship, because AbstractEnvironment.findNonNavigableAssociationEnds is never called.
It is difficult to resolve this in a derived implementation because of a number of private methods.
I'll submit a patch that at least makes derivation possible once bug #242236 has settled. You can choose whether to accept a larger patch that performs the resolution as well.