Bugzilla – Full Text Bug Listing
| Summary: | Abusive IllegalStateException on Notification.getNewXXXValue with dynamic feature delegation | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Alex Lagarde <alex.lagarde> | ||||
| Component: | Core | Assignee: | Ed Merks <Ed.Merks> | ||||
| Status: | CLOSED FIXED | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | matthieu.helleboid, pierre-charles.david | ||||
| Version: | 2.6.0 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Alex Lagarde
Yes, this seems wrong looking at the code. I'm not sure how best to fix it yet. I'd be concerned about letting Boolean features look the same as boolean features; after all, what does one do for null which can't be represented as a primitive. But the bottom line is that the notifications should look the same regardless of whether you have a generated model or how feature delegation is done. I'd appreciate a small test case that doesn't depend on CDO, but I think it won't be hard to make one. I'm not sure if I'll be able to address this in the 2.8 maintenance stream. I'll try my best. Created attachment 220113 [details]
Proposed fix + Junit test case (git patch)
Here is a proposed fix and the associated Junit Test Case.
I also made the modification for all the primitive types.
Alex or Ed may have another fix, but the junit test case should stay usable.
The test case use two different DynamicDelegationFactoryImpl, one generated with feature delegation = none, and the other with feature delegation = Dynamic.
Yes, that's a possible solution, though, as I mentioned, I'm not as comfortable with that. A Boolean allows null and such, a possible value in the notification can and will throw exception, so it's not safe to use the methods unless you're sure the feature is really of a primitive type or you're sure null won't be a value. Taking a step back, it's clear that the setting delegate creating the notification instances can know exactly which primitive type (type feature's type) its producing notifications for and could create the "right type" of notification based on that knowledge. That seems more consistent. I.e., strive to produce exactly the same notification. As you say, the JUnit tests will be useful no matter the approach we use. Is it important for you guys that this be fixed in the 2.8 maintenance stream? You're right but like you suggested, I think that (null instanceof Boolean|OtherType) will return false and then methods will also throw an exception. This way the contract between boolean and Boolean is safe. As the dynamic feature delegation is less used, it's more a compatibility and coherence fix without a lot of performance loss. I can update the patch to test this if you want. About the 2.8 maintenance stream, I prefer to let the reporter (Alex) choose. Hi Ed, first of all, thanks for the fast reaction (as usual). Yes, it would be nice to see this fix integrated in the 2.8 maintenance stream. We are also using older versions of EMF (2.6 & 2.7), but I suppose it is too late to fix this issue in those? Thanks again ! Alex A different set of fixes are committed to 2.8 and master. I took the approach of producing exactly the same notification as you get for generated features of primitive type, not changing the notification implementation to yield non-primitive values as primitives. This involved adding new constructors for ENotificationImpl and an internal API for creation notifications for the data setting delegates. For 2.8 these are all package protected so as not to change API in a maintenance stream. For 2.9 they are public. I provide maintenance support only for in the current release version, so 2.6 and 2.7 won't ever be changed. The changes are available in the latest builds. |