search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[validator] CompleteOCLEObjectValidator ignores inherited constraints #742

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 351578 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Jul 08, 2011 10:20 EDT | | Modified | May 20, 2013 11:37 EDT | | Version | 3.2.0 | | Reporter | Ed Willink |

Description

CompleteOCLEObjectValidator invokes typeManager.getLocalConstraints(type) and so ignores inherited constraints.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 17, 2011 17:23

(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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 21, 2011 11:01

(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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 21, 2011 16:53

Created attachment 201881 Rework to align with maintenance/R3_1.

Also pushed upstream as bug/351578sr1.

Please review for SR1.

:notepad_spiral: %5B351578%5D+SR1+rework.+Test+and+support+CompleteOCL%2FOCLinEcore+validation.patch

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Aug 23, 2011 05:36

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.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Aug 24, 2011 06:39

(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:

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

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 25, 2011 01:35

(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.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Aug 25, 2011 08:34

  • 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"

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;

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 26, 2011 02:49

(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 null, 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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 29, 2011 03:44

Pushed to maintenance/R3_1 for SR1 RC2

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 20, 2013 11:37

CLOSED after a year in the RESOLVED state.