Bug 416833

Comment 1 Kenn Hussey 2013-09-09 15:05:19 EDT

Unless I'm missing something, the UML specification doesn't actually define the expected package merge behavior for stereotype property values; all it says is that stereotypes applied to a merged or receiving element will also be applied to the resulting element. Which part of the specification led you to expect a specific behavior in your scenario?

The current behavior is inherited from (the) EMF (copier) and probably isn't something we'd want to change. One way to achieve your desired behavior would be to make the stereotype property in question 'unsettable' (via the EStructuralFeature stereotype from the Ecore profile); doing so would allow you to explicitly set the value of the property to its default and have that preserved when merged into a receiving property.

Comment 2 Ronan Bar 2013-09-11 08:01:57 EDT

Hi,
Nope your not missing something. There is no explicit statement on the stereotype merge behaviour with respect to enumerations/defaults. That is why I assumed the "general semantic" should apply here also. That being the value from the merged package is the one one I will get in my resulting package. So I guess I expected my scenario to work like any other, which it does not seem to do because of the way Ecore doesn't "set" the value if the default is selected.

From what you say it makes sense that the copy works as it does. But is it really correct when considered outside of the Ecore context? Is this what a user would expect?

Your suggested work-around sounds good. I'll test that later on this week and reply then.

Thanks!
Ronan

Comment 3 Ronan Barrett 2013-09-16 12:46:57 EDT

Hmm I have done some more testing. It would seem that stereotype values from the receiving package (target) are the ones that are set by the merge() in the resulting package. This happens for all the datatypes I have tried i.e. String and Enumeration.

The only exception to this is if the receiving package (target) has no value set, or is the default of an enumeration, then the value will be taken from the merged (source) package. As you say the default can be forced to be set by toggling the "isUnsettable" ecore stereotype property.

So to summarise the behaviour is the opposite to what I expected.

Another related issue is that I tested setting the visibility of a class in the merged (source) package to "private" and the visibility of the matching class in the receiving package (target) to "protected". The resulting package has the visibility set to "protected" which seems to violate rule #5 on p178 of the spec. So again the values in the resulting package are from the receiving package (target) so long as a value is set i.e. not the default.

What do you think?

Comment 4 Ronan Barrett 2013-09-17 03:37:02 EDT

Note: When I say there is a violation of #5 I mean my interpretation of it. There is no specific mention of "protected" there as they only discuss the public/private visibility literals. To test this without hitting the isSettable issue I had to use a different set of literals.

Comment 5 Kenn Hussey 2013-09-17 21:07:35 EDT

(In reply to comment #3)
> Hmm I have done some more testing. It would seem that stereotype values from the
> receiving package (target) are the ones that are set by the merge() in the
> resulting package. This happens for all the datatypes I have tried i.e. String
> and Enumeration.

Strange, I just tried this myself and saw the behavior I expected - stereotype property values were taken from the merged package iff the feature was considered "set" by EMF. I'll attach my example profile and models for you to take a look.

> Another related issue is that I tested setting the visibility of a class in the
> merged (source) package to "private" and the visibility of the matching class in
> the receiving package (target) to "protected". The resulting package has the
> visibility set to "protected" which seems to violate rule #5 on p178 of the
> spec. So again the values in the resulting package are from the receiving
> package (target) so long as a value is set i.e. not the default.

I believe you're referring to the following rule, which in 11-08-05 (UML 2.4.1 Infrastructure) is on page 166:

5.	For all matching elements: if both matching elements have private visibility, the resulting element will have private visibility; otherwise, the resulting element will have public visibility.

This is actually implemented in the UML2 package merger:

		protected void mergeNamedElement_Visibility(
				NamedElement receivingNamedElement,
				NamedElement mergedNamedElement) {

			if (receivingNamedElement.getVisibility() == VisibilityKind.PRIVATE_LITERAL
				&& mergedNamedElement.getVisibility() == VisibilityKind.PRIVATE_LITERAL) {

				receivingNamedElement
					.setVisibility(VisibilityKind.PRIVATE_LITERAL);
			} else if (receivingNamedElement.isSetVisibility()) {
				receivingNamedElement
					.setVisibility(VisibilityKind.PUBLIC_LITERAL);
			}
		}

but for some reason it isn't being invoked properly. I'll do some more investigation and report my findings ASAP.

Comment 6 Kenn Hussey 2013-09-17 21:11:35 EDT

Created attachment 235582 [details]
sample profile and models

Here are the profile and models I used to verify that stereotype property values are being merged as I expected.

Comment 7 Ronan Bar 2013-09-18 14:31:01 EDT

Hi Kenn,
Thanks for the test models. I think I have made an error in my reading of the specification. I thought the receiving model was at the far end of the package merge relationship i.e. The package I will receive onto my merged model. I see from your example I have mixed up the names and thus my understanding of the semantic! I'm sorry for that. Hopefully the finding of the other issue means this hasn't been a total waste of time.

Also thanks for the tip on setting the isUnsettable that fixes my original problem.

Comment 8 Kenn Hussey 2013-09-19 21:56:16 EDT

Glad to hear that your original problem is fixed.

I've done some more investigation into the other problem and determined that none of the attribute transformations are being applied (!); indeed, I don't know how this ever worked. Unfortunately, this isn't something I can easily fix in a maintenance release, because it would represent a non-trivial change in behavior (for example, making the change and running the package merger on the UML2 source model itself resulted in a number of differences). The best I can offer is a fix in the next major release (Luna), where a new option would be introduced to process the attribute transformations (or not).

So, this definitely hasn't been a waste of time. Thanks for helping make UML2 better!

Comment 9 Ronan Bar 2013-09-20 08:31:35 EDT

Ok great, thanks Kenn. I can confirm at least transformations 5 and 6 are not running according to spec. From my perspective Luna is fine as I don't use these features for the merge today. Of course this will be a backwards incompatible change as people who rely on these merge transformations will get different results in Luna.

Comment 10 Kenn Hussey 2013-12-06 11:33:30 EST

I've pushed a new 'bugs/416833' branch with a proposed fix.

Comment 11 Christian Damus 2013-12-06 15:43:50 EST

Why is the merging of LiteralInteger and LiteralUnlimitedNatural values now deprecated? (I notice that these methods are no longer called)  Is it simply that package merge was never supposed to merge them, or is that a change in UML 2.5?

Comment 12 Kenn Hussey 2013-12-06 15:49:30 EST

I only deprecated those methods as an alternative to removing them. The package merge rules actually apply to the lower and upper bounds of multiplicity elements (not specifically to the values of embedded literal integers and unlimited naturals). As it turned out, fixing the bug so that this code was actually executed exposed the fact the the original implementation didn't work - no matching is done for literal integers or unlimited naturals and even if it were, it wouldn't help in applying the transformations because there isn't always a lower or upper value (e.g., in cases where the lower or upper bound is the default, 1).

Comment 13 Kenn Hussey 2013-12-06 15:51:12 EST

FYI, I am working on tests for this but wanted to commit the changes I had so that I could switch to another branch for code reviews. ;)

Comment 14 Christian Damus 2013-12-06 16:12:40 EST

I've pushed some non-functional tweaks to the branch in commit c1850fe.  Looks good!  I see that merging the UML metamodel now results in quite a number of visibilities changed.  Are you planning to commit a new merge?

I haven't merged this to master because the tests aren't yet in place.  But I could do that if you would like.

Comment 15 Kenn Hussey 2013-12-06 16:28:36 EST

Thanks. This option needs to be set to 'ignore' for the UML2 metamodel; the source models are just not structured to handle all of the package merge goodness. ;)

Yes, please hold off on mergin to master until I've added the tests.

Comment 16 Kenn Hussey 2013-12-07 13:58:50 EST

I've added tests and merged everything into the 'master' branch. The changes will be availble in an integration build soon.

Comment 17 Kenn Hussey 2013-12-07 15:33:41 EST

An integration build containing the changes is now available.