Dexels / navajo

Navajo Service-oriented Applications
GNU Affero General Public License v3.0
9 stars 5 forks source link

(Document) Taking message type into account when merging messages #577

Closed roelofkemp closed 3 years ago

roelofkemp commented 3 years ago

The merge operation on Navajo message currently does not correctly take the message type into account. Consider this example:

<message name="Source">
  <message name="M1" type="array">
    <message name="M1" type="definition">
       <property name="P1" />
    </message>
  </message>
  <message name="M2" type="simple">
    <property name="P2"/>
  </message>
</message>

<message name="Target">
  <message name="M1" type="simple">
    <property name="P1"/>
  </message>
  <message name="M2" type="array">
    <message name="M2" type="definition">
       <property name="P2" />
    </message>
  </message>
</message>

If these two messages will be merged: source.merge(target), the current result will be:

<message name="Source">
  <message name="M1" type="simple">
    <property name="P1"/>
  </message>
  <message name="M2" type="array">
    <message name="M2" type="definition">
       <property name="P2" />
    </message>
  </message>
</message>

Note that the message type for M1 and M2 have changed. This should not happen, especially in combination with entities where an entity definition may define a message as simple, and the output of the response can change its type to array (or the other way around), and yet the client relies on the entity definition.

Therefore incompatible message types should not be considered mergeable and an exception should be thrown

kharybdys commented 3 years ago

Shouldn't the output be "crash" instead? Your expected output is to just silently ignore the merge but that can cause problems much further on the line. Fail fast applies here I think

roelofkemp commented 3 years ago

agreed. I've updated the original description

ghost commented 3 years ago

Released as bundle com.dexels.navajo.document version 3.6.38.

jposthumus commented 3 years ago

found an error when using the following code:

The fix was to rename the message Unions to whatever