Bug 316591

Comment 1 Antoine Toulmé 2010-06-14 20:20:27 EDT

Created attachment 171884 [details]
SAP metamodel contribution

This comes from SAP and is a contribution to our model based on the final XSD.

The author of the contribution is alas on vacations. He will confirm the contribution when he comes back. In the mean time, I will open a CQ and apply the parallel IP process to it if possible.

Comment 2 Antoine Toulmé 2010-06-14 20:21:33 EDT

Opened CQ:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=4263

Comment 3 Henning Heitkoetter 2010-06-17 07:45:46 EDT

Very nice!
I had a look through the contribution; this will save us much work regarding renaming (types, associations) and opposite references.

Some questions/comments:
1 - As I understand from the CQ, we will use the model files, but generate and customize everything else on our own? (Just to make sure, I'm happy with that.)
2 - Do we want to use the ability to generate XML and XMI alike, that is - a I understand it - intended by the models?
IMO the different namespaces (.../MODEL vs. .../MODEL-XMI) will raise difficulties. For example, at the moment, MODEL-XMI is used in the serialized XML document (because it is the namespace of the package), leading to errors on load. If you change it to MODEL, XML will work, but a serialization as XMI will have the wrong namespace.
Additionally, some data is to be stored differently in XML vs. XMI, for example the text of a Documentation element or the body of a formal expression.
Note that we always have the option of using the XSL transformation provided by the spec (after some tuning), if we want to serialize with XMI.
3 - The classes in the ecore model of the contribution are not sorted by name (they appear in the same order as in the cmof files). As the generated editor apparently uses the sort order from this model, the "New Child" menu (as an example) is hardly usable. We should sort the class names from A to Z.

Further observations/notes, not necessarily related to this contribution:
4 - While testing the serialization, I noticed the following inconsistency between the XMI and XSD from the specification: Definitions is a subtype of BaseElement in XMI, but not in the XSD.
5 - I like the idea of handling QNames during (de)serialization. However, extending the QNameURIHandler from the contribution to support cross-document references could prove cumbersome (at least from my point of view), because you have no direct access to EObjects (notably Import elements) and cannot create them due to possible infinite recursion.

That's it for now :), I'm looking forward to implementing the final metamodel.

Comment 4 Henning Heitkoetter 2010-06-17 08:57:13 EDT

Created attachment 172112 [details]
Sort classifiers of EPackage by name

Re item #3 of my comment above:
Attached is a small tool (Eclipse project) that reads an ecore model and sorts the classifiers of the root package by name (except DocumentRoot, which is placed first).

Comment 5 Antoine Toulmé 2010-06-17 12:01:17 EDT

Henning, thanks for the comments. I am concentrating on having this pass the IP review before we play with it. The original contributor can probably shed some light on this when he's available next week.

Comment 6 Reiner Hille 2010-06-21 10:16:07 EDT

Hi Henning,
I'm Reiner Hille from SAP and I created the ecore that Antoine has uploaded.
I have a slidely newer version here and I will attach it today or tomorrow, but let me first try to answer your questions.
The basic IDE for what I did is to avoid using the XSLT, thus using the same ECore metamodel to read/write XMI (according to CMOF) and XML (according to schema). 
I did this by merging two ECores, one generated from CMOF and one generated from XSD. The merge is done with a little Java program that I would also contribute (because this program is usefull when the standard evolves). 
BTW: The official XSLTs are also generated from this program. It uses the same mapping rules.
Now about your questions:
1. I don't care. I tried to avoid chnages in the generated code and put almost everything into the ecore, extra files and so forth. However, I did some little changes on generated files (that now have a "generated NOT", that need to be considered.
2. Yes, this is the idea. I want to avoid the XSLTs because they are not very reliable.
The heard for having this working are all the "ExtendedMetadata" annotations that are merged into the ECore everywhere and the ensure that the XML element/namespace and so on are used. This requires of cause that the XMLResourceImpl is used (not XMIResourceImpl) and that is is configured to use the ExtendedMetadata. 
The new XMI namespace where introtuced late and caused some troubles, but I though I could solve them. Could you please describe me the situation where you see the wrong namespace?
And if you find other places where the mapping based on the ExendedMetadata is not complete, please let me know.
3. Yes, the class order comes from the order in the original CMOF model. I didn't do anything so far, because I didn't care. However, in the latest version of my merge tool, I sort the members (because this is critical for the XML serialization). I will update the tool to also sort the classes alphabetically (it is really a simple change).
4. There are MANY MANY inconsistencies, but I try to consider all of them in my merge tool - and believe that in the end I will have compliant serialization for both, XML and XMI.
5. Yes, I think we also need something more, maybe also an URIConverter. But I'm sure it is not too complicated.

Generally I'm willing to be involved in the subsequent steps coming to a fairly usable metamodel.

Regards,
 Reiner.

Comment 7 Antoine Toulmé 2010-06-21 11:03:55 EDT

Reiner, I'm discussing with the IP team to get your contribution included as soon as possible. Right now your contribution misses some legal information: 

You should go ahead and arrange the copyright headers and reattach to this CQ. 
Please see http://www.eclipse.org/legal/copyrightandlicensenotice.php.  It's
important that the initial author (under Contributors Section) contain the
initial author, organization and description.

Eventually, you could just attach the models, without the java files, to minimize the work of the IP team. You might want to make sure that the copyright fields of the genmodel contains the appropriate mention of the Eclipse license.

I will have more questions from the IP team to clear out provenance. As a rule of thumb, I know they ask the contributor to state that they have authorization to contribute this work under the Eclipse license, and they ask who worked on it for copyright tracking purposes.

Comment 8 Henning Heitkoetter 2010-06-22 04:01:04 EDT

(In reply to comment #6)

Hi Reiner,
thank you very much for your explanations.

1. I already did a search for "@generated NOT" and found the following two occurrences: BPMN20ResourceFactoryImpl.createResource; BPMN20Editor.doSaveAs. Additionally, the contents from the package o.e.bpmn2.ecore (Activator, QNameUriHandler) and SaveAsXMIAction are custom. Could you check if I'm missing something?

2. Steps to reproduce the wrong namespace:
- start new Eclipse instance including generated Editor
- create new BPMN20 model (Test.bpmn20; root element Definitions)
- this results in the following XML file:
  <?xml version="1.0" encoding="UTF-8"?>
  <bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL-XMI"/>
-- Note the bpmn namespace declaration, IMO, it should be xmlns:bpmn=".../MODEL" for a serialization to XML
- (force)open Test.bpmn20 with BPMN Editor - this results in an exception (FeatureNotFoundException: Feature 'definitions' not found.)

If you manually remove "-XMI" from the namespace in Test.bpmn20, the BPMN editor opens the file correctly (however, if you make changes and save, the old namespace is restored).

This behaviour results from two conflicting namespaces: the ExtendedMetadata are registered (in Activator) under their correct namespace, but the namespace URI of the BPMN package is the one for XMI. One could change the namespace of the package to the one for XML (the manual registration in Activator is not required in that case); but then we'll have trouble later if we want to support XMI. We would need a mechanism to change the namespace on the fly depending on whether the model is serialized as XML or XMI.

I hope we can work something out to solve this problem, as I do like the idea of being able to serialize to XML and XMI at the same time.

Comment 9 Reiner Hille 2010-06-22 04:09:17 EDT

Hi Henning,
thanks.
1. Yes, this is also my current list. Of cause it could change over time. Today I will attach a new ZIP that only contains .ecores, .genmodel, project stuff and the mentioned files, all with Copyright comments.
2. I see. I never tested to create models from scratch, but usually used an existing bpmn20 XML file. This usually opens correctly. I also tried the "Save as XMI" to see if it correctly converts to XMI.
It seems that soemthing in EMF ignores the ExtendedMetada information. I will debug and see how we can fix it.

Reiner.

Comment 10 Henning Heitkoetter 2010-06-22 06:00:17 EDT

(In reply to comment #9)

I think I found a solution for the namespace problem. We can provide a custom implementation of ExtendedMetadata (subclassing BasicExtendedMetadata) to the load and save options of BPMN20Resources (XMLResource.OPTION_EXTENDED_META_DATA). This implementation should override getNamespace(EPackage) and remove the "-XMI".

XMLSaveImpl.addNamespaceDeclarations calls this method to obtain the namespace uri.

Comment 11 Reiner Hille 2010-06-22 08:18:11 EDT

> I think I found a solution for the namespace problem. We can provide a custom
> implementation of ExtendedMetadata (subclassing BasicExtendedMetadata) to the
> load and save options of BPMN20Resources
> (XMLResource.OPTION_EXTENDED_META_DATA). This implementation should override
> getNamespace(EPackage) and remove the "-XMI".
> 
> XMLSaveImpl.addNamespaceDeclarations calls this method to obtain the namespace
> uri.

Yes, I was also debugging there. This would work and the cool thing is that we can pass in our ExtendedMetadata as option. Hopefully there is not too much other places where the BasicExendedMetadata singleton it hardcoded. 
However, I ask myself if the XMLSaveImpl.addNamespaceDeclarations implementation is really correct: Is is perfectly valid to specify different namespaces for any class or feature in ExtendedMetadata. Such namespace does not need to match any EPackage's namespace, so it looks wrong to me just to loop over all EPackages and wrting prefixes for there namespace.

Comment 12 Reiner Hille 2010-06-22 11:41:58 EDT

Created attachment 172434 [details]
SAP Metamodel Contribution Version 2

Comment 13 Reiner Hille 2010-06-22 11:45:58 EDT

I have uploaded a new version of the contribution with the following change:
- Copyright text in all relevant files
- Only non-generated files or files with changes are included
- Classes sorted as suggested by Henning.
- Correct sorting of features to get correct XML.
- Implementation of custom ExtendedMetadata to create new files with correct XML namespaces - using Henning's idea.
- Correct saving of IDs for references without type.

Comment 14 Antoine Toulmé 2010-06-22 23:37:22 EDT

Reiner, I'm attaching your new contribution to the CQ.

Comment 15 Antoine Toulmé 2010-06-22 23:43:04 EDT

Reiner, our IP team needs those confirmations:

2.  Please arrange for the committer to make the following confirmations on the
referenced bug:

a.  He authored 100% of the content 
b.  He has the rights to donate the content to Eclipse
c.  He contributes the content under the EPL

3.  Regarding the non-normative content to be included, I'm going to ask a
couple of other questions to ensure I'm understanding correctly and to
determine if we need a non epl CQ to track it's use from OMG:

a.  Is the non normative content authored solely by SAP as well?
b.  Is the non normative content contained in this contribution?

I think it's a., unless there are bits from the specification as well in your contribution ?

Comment 16 Reiner Hille 2010-06-23 04:18:11 EDT

(In reply to comment #15)
> Reiner, our IP team needs those confirmations:
> 
> 2.  Please arrange for the committer to make the following confirmations on the
> referenced bug:
> 
> a.  He authored 100% of the content 
Yes.
> b.  He has the rights to donate the content to Eclipse
Yes, SAP contribution approval process is finished successfully.
> c.  He contributes the content under the EPL
Yes.


> 3.  Regarding the non-normative content to be included, I'm going to ask a
> couple of other questions to ensure I'm understanding correctly and to
> determine if we need a non epl CQ to track it's use from OMG:
> 
> a.  Is the non normative content authored solely by SAP as well?
> b.  Is the non normative content contained in this contribution?

I would not use the terms normative and non-normative content here. Normative OMG content are spec text, XSD and CMOF files. They are input to what I contributed. Thus the contribution in an implementation of the normative OMG standard.
And, yes, the implementation was done completely by SAP and is completely contained in the contribution.

Comment 17 Henning Heitkoetter 2010-06-30 08:07:18 EDT

(In reply to comment #13)
> I have uploaded a new version of the contribution with the following change:
> ...
> - Correct saving of IDs for references without type.

I assume the last point concerns the OnlyContainmentTypeInfo class?
I'm afraid I don't understand its purpose - why do we not want so save type info for non-containment references?

Moreover, the use of this XMLTypeInfo leads to a bug. Consider the following steps:
1) create a new BPMN model
2) insert a MessageEventDefinition as child to the Definitions object
3) create a Process (@Definitions)
4) insert an EndEvent into the Process
5) add the MessageEventDefinition to the eventDefinitionRef collection of the end event
6) save the model.

Opening the model again with the BPMN 2 model editor leads to an exception ("tEventDefinition is abstract"). If I disable the save option XMLResource.OPTION_SAVE_TYPE_INFORMATION, the same procedure does not lead to an error.

Comment 18 Reiner Hille 2010-06-30 08:37:11 EDT

Yes, thanks. I had already fixed this, but during checkin I lost the current version and on redo I forgot the change. Indeed SAVE_TYPE_INFO is not wanted.

Comment 19 Reiner Hille 2010-06-30 09:48:50 EDT

After reading more carefully what you wrote I need to debug this again. I'm sure that I also tried the pure "turn save type info" off, but it didn't work.
Let me debug it once more.

Comment 20 Henning Heitkoetter 2010-06-30 09:54:29 EDT

(In reply to comment #19)
> After reading more carefully what you wrote I need to debug this again. I'm
> sure that I also tried the pure "turn save type info" off, but it didn't work.
> Let me debug it once more.

By "disabling XMLResource.OPTION_SAVE_TYPE_INFORMATION" I meant "do not supply an OnlyContainmentTypeInfo object" - so disabling was the wrong term. Sorry for the confusion. Just removing the corresponding line in BPMN20ResourceFactoryImpl should be enough.

Comment 21 Reiner Hille 2010-06-30 10:23:18 EDT

So, back to zero, please ignore my last two comments.
The OnlyContainmentTypeInfo is needed to prevent that the XMLResource writes "xsi:type" stuff into the file. Writing type would be acceptable for the case that a non-containment reference is written in element form:
e.g. 
   <bpmn:endEvent id="id3">
      <bpmn:eventDefinitionRef xsi:type = "tMessageEventDefintion">id1</bpmn:eventDefinitionRef>
    </bpmn:endEvent>

The BPMN 2.0 schema does not condider this, but it would not really harm. It is worse in cases where the reference is stored in an attribute. In such kind the XMLResource would try to encode the type in the string that our schmema requires to contain the QName only.

The problem you described is cause by a "resolveProxies" flag being still turned on for the reference from Event to EventDefintions. I think I should change my merger to turn proxy resolving off completely. As we never can rely having type information on references, we never can resolve proxies.
You can fix the issue locally going to ThrowEvent in BPMN20.ecore, select errorEventDefintionRefs and turn the "Resolve Proxies" to false.

Comment 22 Henning Heitkoetter 2010-06-30 11:36:54 EDT

(In reply to comment #21)
> The BPMN 2.0 schema does not condider this, but it would not really harm. It is
> worse in cases where the reference is stored in an attribute. In such kind the
> XMLResource would try to encode the type in the string that our schmema
> requires to contain the QName only.

Thanks for the explanation.
Note that I could reproduce this behavior (encoded type in attribute value, without OnlyContainmentTypeInfo) with the calledElement reference of CallActivity, but not with attachedToRef of BoundaryEvent. The difference between these two features is mainly the resolveProxies attribute (true for calledElement, false for attachedToRef) - so perhaps OnlyContainmentTypeInfo won't be necessary after you turned off proxy resolution completely, as you've indicated above.

Comment 23 Reiner Hille 2010-07-01 03:35:04 EDT

> Note that I could reproduce this behavior (encoded type in attribute value,
> without OnlyContainmentTypeInfo) with the calledElement reference of
> CallActivity, but not with attachedToRef of BoundaryEvent. The difference
> between these two features is mainly the resolveProxies attribute (true for
> calledElement, false for attachedToRef) - so perhaps OnlyContainmentTypeInfo
> won't be necessary after you turned off proxy resolution completely, as you've
> indicated above.

Good point. I will check this. BTW: I'm a little bit afraid how I implement multipe-file BPMNs without proxy resolution, because proxies are the normal EMF means to support cross-file references. But as the XML does to have type information and EMF requires typed proxies, we must find a different way to resolved the cross-file URIs. E.g. I could load all files from beginning on - when I see an <import> and thus resolve all references during load.

Comment 24 Reiner Hille 2010-07-01 08:02:20 EDT

Hi all,
I plan to commit my BPMN Ecore metamodel contribution to Git.
However, compared to the last ZIP, I changed the package names to be more compliant to current version and also to what is in the Wiki. Here is what will be commited:
Plugin 
org.eclipse.bpmn2
  model
    BPMN20.ecore
    BPMN20.genmodel
    BPMNDI.ecore
    DI.ecore
    DC.ecore
 src
   org.eclipse.bpmn2
     <all generated model classes>
   org.eclipse.bpmn2.util
     <generated, modified and handwritten classes, including Activator, XmlExtendedMetadata,…
   org.eclipse.bpmn2.bpmn2di
    <generated BpmnDi stuff>
…
   org.eclipse.di
     <generated DI stuff>


and so forth.
You see that I changed the packages to lowercase and made it more compliant to the “old” version.

 template
   <same customized templates as in “old” project

Plugin 
org.eclipse.bpmn2.edit
 
<generated plugin with the icons from “old” project merged in

Plugin 
org.eclipse.bpmn2.editor
 <generated plugin with 1 modified file (BPMN2Editor.java)>

All files have copyright note in.

Any complaints or things to consider?

Regards,
  Reiner.

Comment 25 Henning Heitkoetter 2010-07-01 08:51:18 EDT

(In reply to comment #24)
Hi Reiner,

thanks for the notice. For the most part, it looks good to me. Some questions and comments inline below:

>    org.eclipse.bpmn2
>      <all generated model classes>

I assume that implementation classes for the BPMN model elements (...Impl) reside in o.e.bpmn2.impl? (Analog for model elements in other packages)

>    org.eclipse.bpmn2.bpmn2di
>     <generated BpmnDi stuff>

The duplicate "bpmn2" looks a little bit awkward to me. I think I understand your reasons to name the package like that (to differentiate from the general DI package), but wouldn't o.e.bpmn2.di as package name make it equally clear that this package contains the special DI elements for BPMN2?

>    org.eclipse.di
>      <generated DI stuff>

I'm not sure wether we're "allowed" to use that package name? (see http://wiki.eclipse.org/Naming_Conventions#Java_Packages)

If not, we could keep o.e.bpmn2.bpmn2di for the BPMN specific DI stuff and use o.e.bpmn2.di for the general DI (contrary to my statement above), but that's not an ideal solution, given that the general DI elements are independent of BPMN.


We should agree on code formatting guidelines and use them for the generation (property Code Formatting of genmodel), otherwise the code could change with every regeneration. So far, we've used 4 spaces for indentation and a line width of 80. I'll attach my code formatting preferences as an example.

Thanks,
Henning

Comment 26 Henning Heitkoetter 2010-07-01 08:53:25 EDT

Created attachment 173212 [details]
Code formatting preferences

Eclipse Built-in code formatting profile, but with spaces instead of tabs for indentation.
Exported from the eclipse preference page, where it also can be imported.

Comment 27 Reiner Hille 2010-07-01 09:33:52 EDT

> I assume that implementation classes for the BPMN model elements (...Impl)
> reside in o.e.bpmn2.impl? (Analog for model elements in other packages)
Yes.

> The duplicate "bpmn2" looks a little bit awkward to me. I think I understand
> your reasons to name the package like that (to differentiate from the general
> DI package), but wouldn't o.e.bpmn2.di as package name make it equally clear
> that this package contains the special DI elements for BPMN2?
I also had this in mind, which would make somehow sense, as the XML NS "http://www.omg.org/spec/BPMN/20100524/DI-XMI" has only DI at the end. Problem is that the nsprefix must be unquite in BPMN2.0 files, so "di" as prefix is reserverd to "normal DI" and convention was to use "bpmndi" for the BPMN specifics. 
To summarize, I see the following possiblities:
1. org.eclipse.bpmn2.di (inconsistency with NS prefix)
2. org.eclipse.bpmn2.bpmn2di (duplicate bpmn2 in the name)
3. org.eclipse.bpmn2.bpmndi (bpmn/bpmn2 inconsistency)
4. org.eclipse.bpmn2di (not perfect, as bpmndi is not a separate standard)

What do you like best?

> 
> >    org.eclipse.di
> >      <generated DI stuff>
> 
> I'm not sure wether we're "allowed" to use that package name? (see

I spoke with Bern Kolb, our collegue with most Eclipse background, and he suggested "eclipse.org.di" and "eclipse.org.dc", because it would make migration to a final DI/DC standard easier, even if it will later live in an own plugin. Not the we simply "carry" a pre-production version of OMG DI/DC inside of BPMN because the final spec is not yet out.
 
> We should agree on code formatting guidelines and use them for the generation
> (property Code Formatting of genmodel), otherwise the code could change with
> every regeneration. So far, we've used 4 spaces for indentation and a line
> width of 80. I'll attach my code formatting preferences as an example.

Thanks, I will use your template.

Comment 28 Henning Heitkoetter 2010-07-01 12:02:09 EDT

(In reply to comment #27)

> To summarize, I see the following possiblities:
> 1. org.eclipse.bpmn2.di (inconsistency with NS prefix)
> 2. org.eclipse.bpmn2.bpmn2di (duplicate bpmn2 in the name)
> 3. org.eclipse.bpmn2.bpmndi (bpmn/bpmn2 inconsistency)
> 4. org.eclipse.bpmn2di (not perfect, as bpmndi is not a separate standard)
> 
> What do you like best?

I could live with different NS versus Package Prefix, that is, version 1 (provided we use o.e.di for the general DI package). I don't think that this would be a major inconsistency causing trouble later on. The namespace prefix used for serialization and the subpackage prefix should be independent.

If you fear that this could confuse users, I'll prefer (2), but I myself don't expect this to be the case.

> I spoke with Bern Kolb, our collegue with most Eclipse background, and he
> suggested "eclipse.org.di" and "eclipse.org.dc", because it would make
> migration to a final DI/DC standard easier, even if it will later live in an
> own plugin.

I also think that it would make sense to organize DI and DC java packages separately from BPMN and would prefer to do so - perhaps under a common part org.eclipse.dd with corresponding subpackages (as I understand it, both are part of the proposed Diagram Definition spec?).

I'm just not sure wether it conflicts with some eclipse guidelines, maybe Antoine as a more seasoned committer can shed some light on this matter?

Comment 29 Reiner Hille 2010-07-02 04:10:10 EDT

(In reply to comment #28)
> I could live with different NS versus Package Prefix, that is, version 1
> (provided we use o.e.di for the general DI package). I don't think that this
> would be a major inconsistency causing trouble later on. The namespace prefix
> used for serialization and the subpackage prefix should be independent.

Yes, I like this also best and implemented it already locally:
BPMNDI appears in package org.eclipse.bpmn2.di, NS prefix is "bpmndi" and prefix for package classes is "BpmnDi".

> I also think that it would make sense to organize DI and DC java packages
> separately from BPMN and would prefer to do so - perhaps under a common part
> org.eclipse.dd with corresponding subpackages (as I understand it, both are
> part of the proposed Diagram Definition spec?).
> 
> I'm just not sure wether it conflicts with some eclipse guidelines, maybe
> Antoine as a more seasoned committer can shed some light on this matter?

Good idea. I will move "dc" and "di" packages as subpackage of "dd".

Comment 30 Antoine Toulmé 2010-07-02 12:21:00 EDT

(In reply to comment #28)
> I'm just not sure wether it conflicts with some eclipse guidelines, maybe
> Antoine as a more seasoned committer can shed some light on this matter?

In terms of code organization, you can look at UML for ideas. There are no guidelines for NS I think - Kenn would know best.

Comment 31 Antoine Toulmé 2010-07-21 19:02:27 EDT

Marking as fixed. We have all the code in now.