WIP: resolve property references to other namespace #39

fra-pa · 2021-05-13T08:00:02Z

This fixes the issue with equally named properties defined in inherited types as long as these types have different namespaces.
DescriptorBuilder to lookup properties in other namespaces if the name is explicitly prefixed (i.e. namespace:propertyName).

Naming conflicts resulting from multiple inheritance within the same model are still NOT resolved. E.g.:

If the scope for namespaces would be expanded to types, the properties could be seperated and mapped accordingly. But I doubt that this is necessary (or even useful) as long as moddle has no explicit namespacing (e.g. with a new keyword; which could be a useful extension). IMO, a proper model design should avoid this kind of conflicts and it seems to be quite some effort to implement.

Closes #36.

Enable DescriptorBuilder to lookup properties in other namespaces if the name is explicitly prefixed (i.e. namespace:propertyName). This allows a type to inherit properties with the same name defined in super classes from different namespaces. A name conflict, caused by equally named properties from different base types defined inside the same namespace, can NOT be resolved and still throws an exception. Closes bpmn-io#36.

test/spec/properties.js

lib/properties.js

test/spec/properties.js

lib/descriptor-builder.js

nikku · 2021-05-17T08:59:33Z

Thanks for opening this PR @fra-pa.

I was able to have a look and added some comments how to improve the test coverage. The overall code changes look simple enough at the moment. We'd need to improve the test coverage though and test some more edge cases.

fra-pa · 2021-05-17T12:45:46Z

Thanks for review and feedback @nikku. I'll improve testing as soon as I find time.

One question regarding your process with commits to the PR branch. Should I just add them or squash into a single one (with forced push or a new branch)?

nikku · 2021-05-17T12:56:06Z

You can just add additional commits, as you amend your PR. Using some git magic you or myself can always do the cleanup later. Adding commits on top makes it easier for me as a reviewer to follow along the progress of your contribution.

- add properties 'many' and 'single' to models - restructure and rename test cases - use 'eql' for deep-equal for comparing 'many' properties 'many' property replaces 'any' to use a fundamental type. Even if there's no type checking, this improves conformance for assignments and corresponding tests to the definition.

- add a non-many property with a default to the model - test for existance and the correct value

Add properties with default value from a different namespace as 'this'.

Instead of expecting the thrown exception to be of type 'Error', compare the whole string.

Instead of expecting to not throw 'Error', expect to not throw any exception for 'getType()' and 'getDescriptor()'.

nikku · 2021-05-20T17:47:16Z

lib/factory.js

+      if (p.ns.prefix === descriptor.ns.prefix) {
+
+        // use local name if property already in local ns
+        name = p.name;


Interesting question here: What I'd expect is to have local name and ns name attribute present.

I.e. I can always access the value via foo["a:id"] regardless if it has a local name set or not. On the other hand I'd like to have a#id always available as the shorthand, if it is the only id property defined for an element.

Likely this is only possible to accomplish via proxies, am I right about that?

Interesting question here: What I'd expect is to have local name and ns name attribute present.

They are present, but ...

I.e. I can always access the value via foo["a:id"] regardless if it has a local name set or not. On the other hand I'd like to have a#id always available as the shorthand, if it is the only id property defined for an element.

Likely this is only possible to accomplish via proxies, am I right about that?

For isMany-properties this is possible without a proxy, as we reference an object. But fundamental types (i.e. BUILTINS) are only accessible through get() when referenced with their localName. a#id on the other hand is always available through a.id (and a.get('id')of course), as long it was previously initialized (see my remarks below) - so yes, you're right.

IMO this is a compromise that we should accept. As you say, the alternative would be the introduction of proxy objects.

I hope we understand each other (please don't hesitate to keep raising questions). The test cases should hold the truth to specify desired behaviour. So we need to make sure to get them right and fully cover this topic as well as update the documentation before merging.

Btw, the current implementation also fails to access foo['a:id'] if not initialized during creation or has a default value or was previously accessed with get(). I've not tested this out yet, but this seems to be the case for all properties except foo.id. I.e. you need to use get() first before foo['a:id'] is mapped into the object.

This is somehow inconsistent and it might be a nice improvement to map property instances early (i.e. during type creation) even if they are not explicitly initialized or have a default value. I'd more like to get undefined instead of an unexpected exception. If you like, I'll open another issue for this.

One question in this context: should it be possible to define a default value for isMany-properties (e.g. "default": [ 1, 2 ])? I tried this, received no exception but the value was not applied. Maybe I miss it, but there seems to be no documentation or spec for this. Should I address this in a seperate issue or am I just wrong here?

nikku · 2021-05-20T17:49:46Z

Overall I believe we make good progress on this PR.

What I'd love to see is whether our existing serialization utility is actually able to consume multiple-inherited diagrams, modify them and properly serialize them. Only then we're talking about an actual meaningful roundtrip that would justify adding the complexity to this library.

Not only test properties in 'propertiesByName' to exist, but also test e.g. 'many' and 'mh:many' for equality (i.e. properties from the same ns). In case of 'isMany' properties, use 'eql' for deep-equality testing.

Let 'propertiesByName' point to equal descriptors for property names with the same namespace prefix. Take care, that properties which are only defined in another ns are also mapped with their 'localName'.

This chunk should not have been included in this commit, as it's WIP for future commit.

- add property 'defaultSingle' with a different type to 'dt:Root' - inherit also from 'dt:root' - test property descriptors for existance and correct type definition

Add property to 'MultipleInherited' to test descriptor and type for a non-inherited property.

Defining a property that is already defined in a base type from the same namespace without a 'redefines' or 'replaces' trait is not allowed and should throw an exception when retrieving type information (i.e. calling 'Moddle.getType()').

Add function 'defineProperty()' back, with an additional parameter for the property name.

test/fixtures/model/properties.json

nikku · 2021-05-26T19:09:04Z

test/fixtures/model/datatype.json

      "properties": [
        { "name": "bounds", "serialize": "xsi:type", "type": "Rect" },
-        { "name": "otherBounds", "type": "do:Rect", "serialize": "xsi:type", "isMany": true }
+        { "name": "otherBounds",


Proposal: Revert to original formatting (single line) like most of the other props.

I added another property and was not sure about the formatting. If you take a look at fixtures/properties.json, most of the attributes are in a seperate line, but others are defined in one line (top of the file).

It's not consistant and even though I'm not an ideologist about 80 character line limit, I apply this rule for readability and consistancy if reasonable.

This commit changes the formatting back to one-liners and if you prefere this, we keep it. If you like, I'll change the formatting of all fixtures to one style.

nikku · 2021-05-26T19:09:38Z

lib/descriptor-builder.js

      'property <' + propertyName + '> already defined; ' +
-      'override of <' + definedProperty.definedBy.ns.name + '#' + definedProperty.ns.name + '> by ' +
-      '<' + p.definedBy.ns.name + '#' + p.ns.name + '> not allowed without redefines');
+        'override of <' + definedProperty.definedBy.ns.name + '#' +


Am I blind or is this exactly equivalent to the original version, just with a different formatting?

Sorry, to make the review harder than necessary with this reformatting. In the course of modifying the function, I used code, dismissed it later and forgot to also revert the formatting.

Your remark brought the function back to my mind. After taking another look, I refactored the variables and changed the error message. I hope this makes the namespace aspect more explicit.

nikku · 2021-05-26T19:10:55Z

test/spec/moddle.js

      expect(descriptor.ns).to.jsonEqual(expectedDescriptorNs);
      expect(descriptor.properties).to.jsonEqual(expectedDescriptorProperties);
-      expect(descriptor.propertiesByName).to.jsonEqual(expectedDescriptorPropertiesByName);
+      expect(descriptor.propertiesByName.id).to


Why remove the original check here?

I'd say we could add an additional test case, if we want:

Suggested change

expect(descriptor.propertiesByName.id).to

expect(descriptor.propertiesByName.id).to.equal(descriptor.propertiesByName['props:id']);

Why remove the original check here?

Because I added a property and comparing the whole structure depends on the order. That's why I created two expects for this.

I'd say we could add an additional test case, if we want:

👍 added

I applied the suggestion and CI action failed (with an empty error message, why I missed to fix it immediately - and forgot to run tests again). Fixed it here.

test/spec/multiple-inherited-properties.js

nikku

Some findings inlined.

Did you already check if serialization continues to work / works with the multi-namespace support?

Cf. #39 (comment)

fra-pa · 2021-05-26T20:18:08Z

Some findings inlined.

Thanks for another deep review.

Did you already check if serialization continues to work.

I created a branch to get moddle from repository. Existing tests still green.

works with the multi-namespace support?

Also added a first test for the writer, which failed. As with moddle, the different namespaces need to be differentiated. Had no time to dig deeper and push something of substance yet. Soon come ...

In test case 'should provide type descriptor' also expect that 'propertiesByName' point to the same property when accessed with and without a namespace prefix. Co-authored-by: Nico Rehwaldt <[email protected]>

Refactor variables and error message in assertNotDefined() to enhance importance of the namespace aspect for defined properties.

CLAassistant · 2022-03-18T08:06:35Z

All committers have signed the CLA.

nikku · 2022-12-17T15:11:27Z

Closing in favor of #45, which is still a major breaking change.

However it keeps interoperability up as much as possible (local access is still possible), while allowing extensibility through extension packages and explicit namespacing.

In this sense the moddle solution is not to support stock UML packageing 1:1, but rather workaround the brokeness of that thing using a virtual glue package (8a65475).

This was referenced May 13, 2021

Multiple inherited properties #37

Closed

Exception in DescriptorBuilder when inherited properties have the same name #36

Open