Bugzilla – Full Text Bug Listing
| Summary: | [environment] Optional lookup of EPackages in Registry by URI key | ||
|---|---|---|---|
| Product: | [Modeling] OCL | Reporter: | Axel Uhl <eclipse> |
| Component: | Core | Assignee: | Axel Uhl <eclipse> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | adolfosbh, eclipse, ed |
| Version: | 3.1.0 | ||
| Target Milestone: | M6 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Axel Uhl
Created attachment 190521 [details]
Introduces ParsingOptions.LOOKUP_PACKAGE_BY_ALIAS
The LOOKUP_PACKAGE_BY_ALIAS option is considered in lookupPackage and lookupClassifier and if set, branches out to the new private findPackageWithAlias method. If the package isn't found by looking up its name in the registry's keys, lookup is delegated to the existing findPackage operation.
Added test case BasicOCLTest.testAliasPackageName that asserts the new functionality.
Created attachment 190525 [details]
Introduces ParsingOptions.LOOKUP_PACKAGE_BY_ALIAS
Excluded patch to launch which accidentally sneaked into the now obsolete patch
Wow. That's nice and simple. I think aliases can only be for the first element of the pathname, so there is no need for the alias builder. The existing findPackage has a special lookup for the first name and then nested package lookups for children. findPackageWithAlias can be similar. For instance if OclEcore aliases to ocl::ecore, then an access is OclEcore::BagType, but if Ocl:: aliases to ocl, then Ocl::ecore::BagType is the same access. ParsingOptions is a pragmatic better idea than a new EnvironmentOptions. However perhaps an enumeration rather than Boolean value would be more mnemonic and extensible. I can't decide whether: lookup packages else keys lookup keys else packages would be useful further algorithms. An enumeration makes them much easier to add later if we find them to be necessary. And/Or. It may be a good idea to put the findPackage algorithm selection logic in a non-static protected findRegisteredPackage method. (Trivia. We already have findPackageByNsPrefix, so perhaps findPackageByAlias is more consistent.) Created attachment 190540 [details]
Using enum, renamed method, factored common behavior
As suggested, I now use an enumeration to encode the lookup strategy and factored the common strategy selection piece into a new private method. Renamed findPackageWithAlias into findPackageByAlias.
Created attachment 190543 [details]
Simpler lookup, @since's doc
Attached
provides 2 missing @since's (you need to enable API checking)
provides documentation for the enum cases
simplifies the by alias lookup to just use the first name directly
---
It's unfortunate that the ParsingOption is in OCL not OCL Ecore. A quick glance at UMLEnvironment suggests that it is not easy to do the same thing since UML does not have an equivalent registry. May happen one day; may be obsoleted by the pivot model.
We need a couple of paragrpahs of tutorial/wiki documentation, which may require rather more to provide an appropriate context for the paragraphs.
---
If you're happy with the simplification, please wait a couple of days so that Adolfo can comment, and to allow Hudson slave 1 to stabilize. Thursday/Friday may be a good commit time.
Created attachment 190546 [details]
Added one more test case
Great, we seem to have convergence. I added one more test case that ensures that regular package name look-ups still work if the alias strategy is chosen. As you suggest, I'll wait until Thursday or Friday with committing this.
Created attachment 190549 [details]
Added documentation
In addition to the changes of the previous patches I added some paragraphs of documentation in customization.html.
Ed, there is a problem with the current patch. You removed the fallback code that performs a regular findPackage if nothing is found by alias lookup. This makes all regular packages unfindable which I don't think is useful. I'll re-add the fallback code. What was again the reason you removed it? -- Axel Aggravatingly, EPackageRegistryImpl seems inconsistent regarding containsKey(...) and values(). There is also an interesting Javadoc for EPackageRegistryImpl.containsKey that sheds some light on this inconsistency. While containsKey(...) delegates, all other Map methods do not. This makes it hard for people to use a delegating registry because calling values() on it as we do in EcoreEnvironment.findPackage(...) does not deliver the packages contained in the delegate. I'll update the documentation accordingly as this seems to be the best we can do currently. Created attachment 190567 [details]
Do regular fallback lookup if nothing found in aliases
This is necessary in order not to force clients to register *all* other packages with regular names again.
(In reply to comment #10) > Created attachment 190567 [details] > Do regular fallback lookup if nothing found in aliases > > This is necessary in order not to force clients to register *all* other > packages with regular names again. Omission of te fallback wasn't intentional. I was intending just to remove the alias builder. But maybe it should have been. In Java we expect to import almost everything. There may be dangerous ambiguities between my import of XXX and an XXX provided by a newly installed modeling component. OK it's a fallback so the import should always win, but I think it is safer to require the import/alias. If nothing else it forces the user to reference XXXPackage.eINSTANCE, which ensures that the MANIFEST references it. Without the import there is the uncertainty, why doesn't it find XXX? (In reply to comment #11) > Omission of te fallback wasn't intentional. I was intending just to remove the > alias builder. > > But maybe it should have been. > > In Java we expect to import almost everything. There may be dangerous > ambiguities between my import of XXX and an XXX provided by a newly installed > modeling component. OK it's a fallback so the import should always win, but I > think it is safer to require the import/alias. If nothing else it forces the > user to reference XXXPackage.eINSTANCE, which ensures that the MANIFEST > references it. Without the import there is the uncertainty, why doesn't it find > XXX? It would require users to import each and every package if the alias lookup strategy is employed whereas with the default lookup strategy all packages in the registry are found without further explicit alias registration ("import"). This is really tedious for those who happen to run into an ambiguity. I really would like it much better if we could just give precedence to aliases ("URIs") if the alias lookup strategy is selected and resort to regular lookup if nothing is found under the alias. Can you live with that? (In reply to comment #12) > It would require users to import each and every package if the alias lookup > strategy is employed whereas with the default lookup strategy all packages in > the registry are found without further explicit alias registration ("import"). It only requires root packages to be aliased, nested packages can be accessed relative to the alias. > This is really tedious for those who happen to run into an ambiguity. Perhaps you've mistyped. This makes no sense to me. > I really > would like it much better if we could just give precedence to aliases ("URIs") > if the alias lookup strategy is selected and resort to regular lookup if > nothing is found under the alias. Can you live with that? It sounds remarkably like the "lookup keys else packages" strategy that motivated an enum rather than a Boolean. Go for it. Let the users choose a strict/lazy behavior. (In reply to comment #13) > (In reply to comment #12) > It only requires root packages to be aliased, nested packages can be accessed > relative to the alias. True. Those can still be many. > > This is really tedious for those who happen to run into an ambiguity. > > Perhaps you've mistyped. This makes no sense to me. I meant what I think I wrote :-). Let me try again: If someone happens to encounter an ambiguity while using the default lookup strategy and that user's expression (or set of expressions, if many expressions need to be parsed) require access to other packages than the ones causing the ambiguity, then all those other packages need to be added to the dedicated registry explicitly as well or else the lookups for those types will fail when changing the strategy from default to alias. This causes extra effort which is what I called tedious. Makes sense? > > I really > > would like it much better if we could just give precedence to aliases ("URIs") > > if the alias lookup strategy is selected and resort to regular lookup if > > nothing is found under the alias. Can you live with that? > > It sounds remarkably like the "lookup keys else packages" strategy that > motivated an enum rather than a Boolean. Go for it. Let the users choose a > strict/lazy behavior. That's true. Turns out the enum really is the better choice. I'll look at it later tonight. Created attachment 190592 [details]
Added a third enumeration option to first look for aliases, then regular names
As suggested, added a third option that first looks up by alias, then by regular package name. The previous alias-only option now only looks for aliases, not regular names.
Created attachment 190621 [details]
Documentation tidy up
No intentional functional change.
One redundant import removed.
Documentation moved from non-API private static method to the API enums, and obsolete descriptions updated.
------------
I'm not happy with this non-delegating values() problem.
The aliases-then-names strategy recommends a delegating registry and then has to document that it doesn't work so you have to cross-populate.
Fixing non-delegation for the standard strategy is dangerous. There may be clients who have a private registry delegating to the global registry who benefit from not getting any resolution from the possibly huge global registry. Also the search of the global registry will cause all package delegates to be resolved loading every model in the system!
Ahah! Delegating registries deliberately do not work.
Sorry. I've run out of time. Please ensure that the documentation makes clear in the EcoreEnvironment, Factory, ... constructors that the EPackage.Registry does not delegate by design, in order to avoid loading all models in all plugins. Users are therefore recommended to selectively populate a registry rather than use the gloabl one, or do putAll from a gl;obal one.
(In reply to comment #16) > Created attachment 190621 [details] > Documentation tidy up > > No intentional functional change. > > One redundant import removed. > > Documentation moved from non-API private static method to the API enums, and > obsolete descriptions updated. Looks good. > I'm not happy with this non-delegating values() problem. I think you may have missed the "if (next instanceof EPackage)" in EcoreEnvironment.findPackage, line 655. If a package is unresolved, it doesn't get resolved. It will simply show up as an EPackageDescriptor. The cruel thing about this is that now I realize that whether or not a package is found depends on whether it has been resolved in the registry before. I think this should be noted in the EnvironmentFactory constructor where the registry can be passed in. > The aliases-then-names strategy recommends a delegating registry and then has > to document that it doesn't work so you have to cross-populate. > > Fixing non-delegation for the standard strategy is dangerous. There may be > clients who have a private registry delegating to the global registry who > benefit from not getting any resolution from the possibly huge global registry. > Also the search of the global registry will cause all package delegates to be > resolved loading every model in the system! > > Ahah! Delegating registries deliberately do not work. > > Sorry. I've run out of time. Please ensure that the documentation makes clear > in the EcoreEnvironment, Factory, ... constructors that the EPackage.Registry > does not delegate by design, in order to avoid loading all models in all > plugins. Users are therefore recommended to selectively populate a registry > rather than use the gloabl one, or do putAll from a gl;obal one. I'll adjust the documentation accordingly. Created attachment 190648 [details]
Updated documentation regarding delegating registries and package resolution
I updated the documentation as discussed. Package resolution is not worse with the patch that it was before. Registry.values() does not resolve package descriptors. I added the consequences for package lookup to the documentation.
The hint to Registry / Map.putAll is OK IMHO because no resolution of yet unresolved package descriptors takes place in this case either.
Different issues here ;P To shake a little bit more the cocktail, does anyone noticed the following issue Bug 212120 ?. I've not spent any time in analyzing the proposed algorythm to see if it could solve this problem, but anyway just a reminder for the future ;) The good news are that the patch doesn't change the present behaviour so I think we are painless with it. Besides, it adds some useful information/documentation concerning the delagating registry (which I didn't know, BTW). +1 Two minor comments: - What about to include any kind of test to demonstrate the problem of looking up packages using the global epackage registry as delegate ? - What about if we avoid creating a delegating registry with the global epackage registry ?. If it really doesn't work let's teach users to use in the correct way. (testAliasPackageName and testRegularPackageNameInContextWithAliasLookupEnabled tests). Regards, Adolfo. I replaced the use of the delegating constructor by the default constructor which is possible because the two tests don't depend on the contents of the default registry. Committed to CVS HEAD. Mid-air collision ... (In reply to comment #19) > To shake a little bit more the cocktail, does anyone noticed the following > issue Bug 212120 ?. I've not spent any time in analyzing the proposed algorythm > to see if it could solve this problem, but anyway just a reminder for the > future ;) Well spotted, we can RESOLVE that with this fix too. The suggested fix is not appropriate; it breaks OMG specs. The by-aliases strategy allows the users to choose to take control and arguably is a top-down strategy as requested. > > The good news are that the patch doesn't change the present behaviour so I > think we are painless with it. Besides, it adds some useful > information/documentation concerning the delagating registry (which I didn't > know, BTW). +1 Ok to commit now. But I think Hudson slave-1 is misbehaving again, so no need to start a build immediately. > Two minor comments: > - What about to include any kind of test to demonstrate the problem of looking > up packages using the global epackage registry as delegate ? It may be interesting to prove out theories of defective behaviour, but that can only be a private since we don't specigfically need to make the failure a regular test. > - What about if we avoid creating a delegating registry with the global > epackage registry ?. If it really doesn't work let's teach users to use in the > correct way. (testAliasPackageName and > testRegularPackageNameInContextWithAliasLookupEnabled tests). Anything we do to the global registry to make things 'better' risks fully populating a partially populated registry for some users. Perhaps we need to augment the lookup-by-name strategy with a new lookup-by-names that is deterministic. Sorry Guys, I thought that just commenting on the bug I were added to CC. Well, it looks like I'm not automatically added when replying via mylyn. I hope we don't have to mix bugzillas mdt-ocl.dev newsgroup again... Nothing else to add, excepting that I'm currently struggling with our build again... I don't know why buckminster is stupidly failing while importing/building source code. I'll keep you informed. Cheers, Adolfo. (In reply to comment #21) > Mid-air collision ... > > (In reply to comment #19) > > To shake a little bit more the cocktail, does anyone noticed the following > > issue Bug 212120 ?. I've not spent any time in analyzing the proposed algorythm > > to see if it could solve this problem, but anyway just a reminder for the > > future ;) > Well spotted, we can RESOLVE that with this fix too. The suggested fix is not > appropriate; it breaks OMG specs. The by-aliases strategy allows the users to > choose to take control and arguably is a top-down strategy as requested. > > > > > The good news are that the patch doesn't change the present behaviour so I > > think we are painless with it. Besides, it adds some useful > > information/documentation concerning the delagating registry (which I didn't > > know, BTW). +1 > Ok to commit now. But I think Hudson slave-1 is misbehaving again, so no need > to start a build immediately. > > > Two minor comments: > > - What about to include any kind of test to demonstrate the problem of looking > > up packages using the global epackage registry as delegate ? > It may be interesting to prove out theories of defective behaviour, but that > can only be a private since we don't specigfically need to make the failure a > regular test. > > > - What about if we avoid creating a delegating registry with the global > > epackage registry ?. If it really doesn't work let's teach users to use in the > > correct way. (testAliasPackageName and > > testRegularPackageNameInContextWithAliasLookupEnabled tests). > Anything we do to the global registry to make things 'better' risks fully > populating a partially populated registry for some users. > > Perhaps we need to augment the lookup-by-name strategy with a new > lookup-by-names that is deterministic. Closing |