ZeligsoftDev / CX4CBDDS

CX4CBDDS component modeling and generation tool
Apache License 2.0
8 stars 5 forks source link

Investigate redundant xmi types in the uml file #461

Closed ysroh closed 7 months ago

ysroh commented 1 year ago

Issue and tracking information

Investigate if we can do something about the redundant xmi types in the uml files. e.g.,

https://www.eclipse.org/forums/index.php?t=msg&th=1102970&goto=1823300&

Developer's time Estimated effort to fix (hours):

Developer's Actual time spent on fix (hours)

Issue reporter to provide a detailed description of the issue in the space below

eposse commented 8 months ago

I know this is an issue when comparing file versions, with some models having the xmi:type attribute and some missing it, but I haven't been able to reproduce it yet, and I always get the xmi:type attribute. By any chance can you reproduce a case where you don't get the attribute?

emammoser commented 8 months ago

What I have found is that if I do any operation in Papyrus that does not involve CX when saving the file, I can generate the file. One way of doing this was going into the "uml" viewer of Papyrus for a model, making an innocuous change, and saving the file. Another one that has started to bite some of our developers is certain egit operations, which I have witnessed but cannot give good reproducer tips for.

Last year, when I developed a migration script for some of our migration models, I had the issue where all of the migration code that I wrote worked well, but would always save the model without the xmi:type attributes. In order to fix this, to save WITH the xmi:type attributes I had to save the resource with a specific XML Resource option using the following:

try {
    HashMap<String, Boolean> map = new HashMap<String, Boolean>();
    map.put(XMLResource.OPTION_SAVE_TYPE_INFORMATION, Boolean.TRUE);
    res.save(map);
} catch (IOException e) {
    e.printStackTrace();
}

I believe that map option is likely defaulted as false normally.

eposse commented 7 months ago

I've been able to reproduce part of it in one scenario: merging conflicting files. In my particular case, I had a very simple test model with a package, a plain UML Class inside the package, and created two branches, one where I added a UML Property with Integer type to the Class and the other where I added a UML Property with Real type to the same class. In both cases, the property had the same name. Interestingly, this was not the conflict, and the merged model just added the two properties. The conflict happened in a file called Test_en_US.properties that was generated when I added a label, and the conflict was in the timestamp comment. So this merge functionality might not be what is intended, but that's a different issue. The issue here is that after merging, some of the xmi:type annotations disappeared.

The result of the merge didn't remove all xmi:type annotations, but removed the following:

  1. In the eAnnotations of the top-level uml:Model element; both the eAnnotations element itself, and the "details" elements.
  2. In packageImport elements
  3. In ownedAttribute elements that changed
  4. In profileApplication elements
  5. In eAnnotations of profileApplications
  6. In appliedProfile elements

I'll have to do more tests, but I suspect that it may only affect changed elements and eAnnotations.

I haven't been able to reproduce it by making plain UML (non-CX) changes.

I am aware of the saving options, but looking at the Papyrus source code, there is no clear default. In some cases the default is true, and in others it's false. I'm trying to understand which is which.

But clearly, when EGit is used, the saving uses false. What is not clear to me is which route does it go: does it invoke the Papyrus save operation, or does it do directly through UML or EMF. So I'll have to investigate these.

eposse commented 7 months ago

More info.

Once I got a file with the xmi:type annotations removed from the UML Properties, I made a simple change to one of the properties in the UML tab of the Properties view, and set "isUnique" to false, and saved. After saving the model, this added back the xmi:type annotation to the changed property and nothing else, but after committing the change with EGit, it added the xmi:type annotation to all elements. So clearly there are two different kinds of "save" at play here.

eposse commented 7 months ago

A bit more: the problem has nothing to do with CX. It it purely a Papyrus + EMF Compare issue.

When Papyrus saves models normally, it saves them with the xmi:type info. When models are merged, (after resolving conflicts, if any) it saves the merged model, but here the defaults are different. In particular, the defaults for Papyrus are normally these:

XMIResource.OPTION_USE_XMI_TYPE
XMLResource.OPTION_SAVE_TYPE_INFORMATION

The first is a Boolean. The second one be either a Boolean or an XMLTypeInfo, which is an interface with two boolean shouldSaveType(...) methods.

In plain Papyrus, when saving a model (not during merge), It goes through a route where it checks if XMLResource.OPTION_SAVE_TYPE_INFORMATION is a Boolean or not, and if it is a Boolean and its value is true, it creates a new XMLTypeInfo like this:

new XMLTypeInfo()
{
    public boolean shouldSaveType(EClass objectType, EClassifier featureType, EStructuralFeature feature)
    {
        return objectType != anyType;
    }

    public boolean shouldSaveType(EClass objectType, EClass featureType, EStructuralFeature feature)
    {
        return true;
    }
};

So as long as the objectType is not the type Any, it saves the type info.

But when saving during merge, what happens is different. First, when creating a UMLResource using the primitive (non-Papyrus) UML Resource factory (org.eclipse.uml2.uml.internal.resource.UMLResourceFactoryImpl.createResource(URI), the default options for saving are initialized with the option XMLResource.OPTION_SAVE_TYPE_INFORMATION set to:

new XMLTypeInfo() {

    public boolean shouldSaveType(EClass objectType,
            EClassifier featureType, EStructuralFeature feature) {
        return objectType != featureType
            && objectType != XMLTypePackage.Literals.ANY_TYPE;
    }

    public boolean shouldSaveType(EClass objectType,
            EClass featureType, EStructuralFeature feature) {
        return objectType != featureType;
    }
});

And this is the one that is used when saving after the merge (and apparently after all saves after the merge, but I'll have to confirm this).

As can be seen, the criteria for saving is diffierent, namely that the objectType be different than the featureType and that the objectType is not the type Any. But for many elements, like eAnnotation, packageImport, ownedAttribute. profileApplication and appliedProfile (and surely many more), the objectType is the same as the feature type (in the UML meta-model), so these methods return false, and the type info is omitted.

Certainly we cannot modify the UML meta-model, so we need to override these options on save, but this will have to be the save operation of EMF Compare. I'll have to investigate if this can be overridden or if we need to patch it.

emammoser commented 7 months ago

Thank you for the information. Just to be clear, our goal here is to remove the redundant types completely if they serve no purpose. I am surprised you aren't able to reproduce the issue as easily as we are, or maybe it has just been a while and my memory is foggy. Either way, I appreciate the deep dive into the issue.

Since Papyrus appears to not care, or not enforce that they are written out, our goal would be to have the method that CX uses to save the file to set the options that omit those annotations. If this can also apply to all other options in Papyrus, that works too, although CX and eGit are the only ways that I know of that our users interact with models that would involve saving.

eposse commented 7 months ago

Hi, I'm not sure if there is a misunderstanding or if I expressed myself incorrectly, but I have been able to reproduce it, or at least part of the behaviour. See https://github.com/ZeligsoftDev/CX4CBDDS/issues/461#issuecomment-1977578534. Specifically I have been able to reproduce is the following:

  1. Normal saving of a Papyrus model in the workspace generates the xmi:type annotations (almost) always, including the redundant ones.
  2. When merging, the models are saved without redundant xmi:type annotations.

Is this not the same that you are experiencing?

From my investigation, it looks like the desired behaviour is the one that happens during merge, rather than during a normal Papyrus save, which is to use the second XMLTypeInfo instance shown in my previous comment, or perhaps even something very similar, but definitely different from the first instance shown in the comment, which saves the type info almost always.

So my idea is to try to tell Papyrus to use a new XMLTypeInfo instance with the desired logic. The only question is whether I can do it via an extension point, or with a patch. That's what I'm investigating now.

emammoser commented 7 months ago

Sorry, I believe we are on the same page. I agree your comment above and with your investigation path going forward.

eposse commented 7 months ago

See PR #497.

emammoser commented 7 months ago

Looks great. Thank you. Can we have this applied to the v3 stream as well? Thank you

eposse commented 7 months ago

Hmm... I thought it would be an easy cherry pick of the commit, but it turns out that the relevant Papyrus APIs changed (as it happens with jumps in major versions) so the exact solution does not work for v3. I'll have to investigate what's the equivalent way to do it in Papyrus 6.5.

This illustrates quite well the increased cost of having two main streams.

emammoser commented 7 months ago

I understand your frustration. We frequently maintain multiple streams of multiple products ourselves as well, it is a struggle, but a necessary evil at times.

eposse commented 7 months ago

Ok, the issue turned out to be missing dependencies. Anyway, it builds now. Are you ok if I merge the PRs?

emammoser commented 7 months ago

I would like to first test the v2 streams when I get a moment, hopefully later this week.

emammoser commented 7 months ago

When testing the v2 streams solution I see that many xmi:types are removed. I think this solution is good.

eposse commented 7 months ago

Yes. The functionality that I put in place is the same as the default UML2 one which Papyrus had overridden, and which is used by EMF Compare when merging. This means that while Papyrus will save the type info for all elements (except those of type 'Any'), UML2 will only save the ones that are necessary. For example, you'll see the xmi:type annotation in all 'packagedElement' inside any package, because 'packagedElement' itself is too generic and doesn't say whether it's, e.g., a 'Class' or a 'Package' or a 'Type' or a 'Constraint'.

So the result that you'll get now, by saving Papyrus models, should be the same as the result of a merge.

Should I merge the PRs and close it now, or would you like to do more testing (maybe on v3)?

emammoser commented 7 months ago

You can merge in the PRs now.

eposse commented 7 months ago

Closing.