Bugzilla – Full Text Bug Listing
| Summary: | MetaTypeInformation.getFactoryPids() returns non-factory pids | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Jarek Gawor <jgawor> | ||||||||||||
| Component: | Compendium | Assignee: | John Ross <jwross> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | jwross, tjwatson | ||||||||||||
| Version: | unspecified | ||||||||||||||
| Target Milestone: | 3.7 M5 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 186528 [details]
Patch for this issue.
The attached patch ensures that the ObjectClassDefinitionImpl object is cloned if the same OCD is shared between a factory and non-factory pid.
The current code associates a type with an ObjectClassDefinitionImpl, either PID or FPID, based on whether or not the factoryPid attribute is declared on the Designate element. The MetaTypeProviderImpl uses this type to determine in which table the PID (or PIDs) should be placed, either allPids or allFactoryPids. The XML ordering makes a difference because the type is ultimately based on the last encountered Designate associated with the OCD.
It's not clear this imposed type is justified. It seems to be based on a comment in 105.6.2 which states "For factories, the related pid attribute is ignored because all instances of a factory must share the same meta type." However, this seems to only refer to the case where both pid and factoryPid attributes are specified on the same Designate element. For example,
<Designate pid="singleton" factoryPid="factory">
<Object ocdref="test.config.general"/>
</Designate>
would result in "factory" being delivered as part of getFactoryPids() but not in "singleton" being delivered as part of getPids(). I don't think it applies to the XML snippet provided previously in this report. In fact, there's a very similar example in 105.6.2.
Regardless, the result of having "factory" returned by getPids() or "singleton" returned by getFactoryPids() seems clearly unacceptable.
Created attachment 186584 [details]
Proposed Alternate Patch 1
I'm attaching a proposed alternate patch. This avoids cloning and (I believe) makes the original code clearer in terms of what's going on. I added a new Designate class that tracks the Designate related information as well as the OCD associated with it. A collection of designates is now returned by the parser rather than a hashtable of pids to ocds. The MetaTypeProviderImpl then iterates over the collection and asks each designate if it's a singleton configuration (i.e. if a factoryPid was not specified). If so, the pid and ocd go into allPids. If not, the factoryPid and ocd go into allFactoryPids.
The initial patch might work; however, I am unable to apply it, presumably because it's in the Standard format. If we prefer to go with that one instead, we'll need to have it reattached in either Context or Unified (preferred) format so it can be applied and tested.
Created attachment 186586 [details]
Test Patch 1
I'm also attaching a patch to org.eclipse.equinox.compendium.tests that tests for this.
Created attachment 186646 [details]
Proposed Alternate Patch 2
Updated alternate patch. Removed unused package private field from DataParser and added copyright to Designate.
Thanks. I went ahead and released John's latest patch. After some more discussion post release, we decided the Designate method should be isFactory() rather than isSingleton() because it's a bit clearer. I'll attach an updated patch with the latest from HEAD soon. Created attachment 186725 [details]
Updated alternate patch with isFactory and copyrights
This patch changes isSingleton() to isFactory() and updates the copyrights in all modified files to 2011.
Thanks for catching and reporting this, Jarek. Please let us know if this doesn't fix your issue.
Tested with latest code from trunk. Works great for me. Thanks! I released the last clean up patch from John. Thanks John. |
Build Identifier: When OCD definition in metatype.xml is shared between a factoryPid and a pid, depending on ordering of <Designate> elements in XML, the MetaTypeInformation.getFactoryPids() can return non-factory pids or MetaTypeInformation.getPids() can return factory pids. For example: <OCD name="general" id="test.config.general"> <AD id="present" type="Boolean" default="true" /> </OCD> <Designate pid="singleton"> <Object ocdref="test.config.general" /> </Designate> <Designate pid="factory" factoryPid="factory"> <Object ocdref="test.config.general" /> </Designate> In this case getFactoryPids() would return 'factory' and 'singleton' pids. Reproducible: Always