Bugzilla – Full Text Bug Listing
| Summary: | [validator] CompleteOCLEObjectValidator ignores inherited constraints | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Modeling] OCL | Reporter: | Ed Willink <ed> | ||||
| Component: | Core | Assignee: | OCL Inbox <mdt-ocl-inbox> | ||||
| Status: | CLOSED FIXED | QA Contact: | Ed Willink <ed> | ||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | eclipse | ||||
| Version: | 3.2.0 | ||||||
| Target Milestone: | SR1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows Vista | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Ed Willink
(In reply to comment #0) > CompleteOCLEObjectValidator invokes typeManager.getLocalConstraints(type) and > so ignores inherited constraints. No. The supertype search is orchestrated by EObjectValidator (although only for the first superclass). The bug seems to be that typeManager.getAllClasses(type) returns null for a primary class without secondaries. (In reply to comment #1) > (In reply to comment #0) > > CompleteOCLEObjectValidator invokes typeManager.getLocalConstraints(type) and > > so ignores inherited constraints. > > No. The supertype search is orchestrated by EObjectValidator (although only for > the first superclass). Bug 355184 raised against EObjectValidator that affects OCLinEcoreValidator. > The bug seems to be that typeManager.getAllClasses(type) returns null for a > primary class without secondaries. This is a bug that shows up with addition of a missing OCLinEcoreValidator test. A test for CompleteOCLEObjectValidator shows that the original bug report was right. Fixes for both pushed to master for M2. Created attachment 201881 [details]
Rework to align with maintenance/R3_1.
Also pushed upstream as bug/351578sr1.
Please review for SR1.
I currently have no working / compiling xtext environment set up. Adolfo, can you review? The source code changes look ok to me, but I would have liked to at least confirm that the new tests fail before and succeed after the patch. (In reply to comment #4) > I currently have no working / compiling xtext environment set up. Adolfo, can > you review? The source code changes look ok to me, but I would have liked to at > least confirm that the new tests fail before and succeed after the patch. Checked... Tests Cases fail before the the fix, they are OK after it. I'm afraid I'm so novice with this code that I can only add potentially naive comments: - I'm not sure if it's related, but If bug Bug 355184 is sorted out, will this code be revised again ?. It looks like you are manually visiting every super class to gather constraints. - Just noticed that every time you try to obtain a pivto2ecore adapter with no TypeManager..... a new TypeManager is created..... suspicious: 1. What about trying to look in the resourceSet of the resource to obtain an already created TypeManager ? ... I've found a TypeManager.getAdapter utility which may help 2. TypeManager.getAdapter has an extra "eAdapters.add(adapter);" when the TypeManager is instantiated... The adapter is already attached in the TypeManager constructor 3. Ecore2Pivot constructor again tries to create a new TypeManager... Should not it try to obtain it from the resource's resourceSet ? 4. Ecore2Pivot.getAdapter tries to add the new adapter to the resource's adapter list... Should not this be done in the Ecore2Pivot constructor ? - return allOk || (diagnostics != null) ?. If diagnostic may be null, it might potentially throw NPE when !allOk BTW, I was not able to apply the patch. I think it was not correctly created (maybe a no workspace patch)... I reviewed by synchronizing against bug/351578SR1 (In reply to comment #5) > I'm afraid I'm so novice with this code that I can only add potentially naive > comments: Thanks for the review. > - I'm not sure if it's related, but If bug Bug 355184 is sorted out, will this > code be revised again ?. It looks like you are manually visiting every super > class to gather constraints. Bug 355184 is an EMF 'feature' that could be workedaround if OCLinEcoreEObjectValidator totally overrode validate in the same way as CompleOCLEObjectValidator does. This bug just fixes CompleteOCL. > - Just noticed that every time you try to obtain a pivto2ecore adapter with no > TypeManager..... a new TypeManager is created..... suspicious: > 1. What about trying to look in the resourceSet of the resource to obtain > an already created TypeManager ? ... I've found a TypeManager.getAdapter > utility which may help The TypeManager (Meta-Model Manager) manages the complete meta-model; i.e. with CompleteOCL modifications. The same resource may be shared by applications that have different CompleteOCL complements, so there may be many distinct TypeManagers. It is only for CS Resources and ResourceSets that you can lookup the TypeManager in this way. For the typical no-CompleteOCL case the TypeManager overheads are reduced and I'm trying to reduce them further. > 2. TypeManager.getAdapter has an extra "eAdapters.add(adapter);" when the > TypeManager is instantiated... The adapter is already attached in the > TypeManager constructor Yes. Inelegant and inefficient, but not this review. Bug 355790 raised. > 3. Ecore2Pivot constructor again tries to create a new TypeManager... > Should not it try to obtain it from the resource's resourceSet ? Application code should make every effort to reuse the TypeManager. Generally the first model load can allow a default creation, then e.g. Ecore2Pivot.getTypeManager() allows the reuse. Multiple TypeManagers is one of my 'favourite' application bugs. > 4. Ecore2Pivot.getAdapter tries to add the new adapter to the resource's > adapter list... Should not this be done in the Ecore2Pivot constructor ? In general an Adapter is constructed then added to a list which invokes setTarget(). This potentially allows reuse on multiple targets. When I really want the target to be final, I install in the constructor, which means that setTarget() doesn't fully support its 'API'. When I'm less demanding on a final I use the normal external addition. > - return allOk || (diagnostics != null) ?. If diagnostic may be null, it might > potentially throw NPE when !allOk The new (diagnostics != null) is a stronger compliance with apparent EValidator philosophy. If diagnostics == null, validation terminates on the first anomally. If diagnostics != null, validation accumulates all problems therein. > BTW, > I was not able to apply the patch. I think it was not correctly created (maybe > a no workspace patch)... I reviewed by synchronizing against bug/351578SR1 GIT patches seem to be a Work In Progress: Bug 343276. > > - Just noticed that every time you try to obtain a pivto2ecore adapter with no > > TypeManager..... a new TypeManager is created..... suspicious: > > 1. What about trying to look in the resourceSet of the resource to obtain > > an already created TypeManager ? ... I've found a TypeManager.getAdapter > > utility which may help > > The TypeManager (Meta-Model Manager) manages the complete meta-model; i.e. with > CompleteOCL modifications. The same resource may be shared by applications that > have different CompleteOCL complements, so there may be many distinct > TypeManagers. It is only for CS Resources and ResourceSets that you can lookup > the TypeManager in this way. Too many questions.... I'm not sure If should question/discuss the design... ;P "The same resource may be shared by applications that have different CompleteOCL complements" - Do you mean the same "physical" resource, the same in-memory EMF resource ? - What does "application" mean ? In anycase I guess that different application should imply different ResourceSets and therefore different EMF Resources. - The same "resource" with different CompleteOCL complements, could imply different TypeManager attached to different ResourceSets?... Will we have different EMF Resources (corresponding to the same "physical" resource) in those ResourceSets ?.... Well, I think it's better to stop here, I should deeply analyse the design, and I don't currently have time to do that... I only make you think about it, if you feel all is fine, go ahead... > > 3. Ecore2Pivot constructor again tries to create a new TypeManager... > > Should not it try to obtain it from the resource's resourceSet ? > > Application code should make every effort to reuse the TypeManager. Generally > the first model load can allow a default creation, then e.g. > Ecore2Pivot.getTypeManager() allows the reuse. > > Multiple TypeManagers is one of my 'favourite' application bugs. I think that facade methods like Ecore2Pivot.get/findAdapter TypeManager.get/findAdapter is the only code which applications should know... Depending on the context in which these adapters make sense (ResourceSet ?, Resource? extra parameters like CompleteOCL complements ?), the facade methods will expect different parameters (those one corresponding the context)... but they should be the only one which should create/instantiate, add and find the corresponding adapters depending on the that context (ResourceSet, resource, CompleteOCL complements). > > > 4. Ecore2Pivot.getAdapter tries to add the new adapter to the resource's > > adapter list... Should not this be done in the Ecore2Pivot constructor ? > > In general an Adapter is constructed then added to a list which invokes > setTarget(). This potentially allows reuse on multiple targets. When I really > want the target to be final, I install in the constructor, which means that > setTarget() doesn't fully support its 'API'. When I'm less demanding on a final > I use the normal external addition. As commented, as far as possible, I prefer to prevent users to do such external addition.... Again, if you feel this is necessary, I can't say the contrary without deeply reviewing the API design and use of it.... I only want to make you think about it. > > > - return allOk || (diagnostics != null) ?. If diagnostic may be null, it might > > potentially throw NPE when !allOk > > The new (diagnostics != null) is a stronger compliance with apparent EValidator > philosophy. If diagnostics == null, validation terminates on the first anomally. > If diagnostics != null, validation accumulates all problems therein. Ed if diagnostic == null and you encounter an error, you will have an NPE in line 234 From your words I understand that you want to do the following ?: if (message != null) { allOk = false; if (diagnostic == null) { break; } diagnostics.add(new BasicDiagnostic(severity, DIAGNOSTIC_SOURCE, 0, message, new Object [] { object })); if (severity == Diagnostic.ERROR) { break; // Generate many warnings but only one error } } ... return allOk; (In reply to comment #7) > > If diagnostics != null, validation accumulates all problems therein. > > Ed if diagnostic == null and you encounter an error, you will have an NPE in > line 234 Oops. Finally found the API statement in EValidator. "@param diagnostics a place to accumulate diagnostics; if it's <code>null</code>, no diagnostics should be produced." so diagnostics == null, supports early return false and should bypass message String building. bug/351578sr1 updated and extra tests added. Also spotted a missing else. If the constraint is non-Boolean, no need to evaluate it and so overwrite the non-Boolean message. Pushed to maintenance/R3_1 for SR1 RC2 CLOSED after a year in the RESOLVED state. |