buildingSMART / IDS

Computer interpretable (XML) standard to define Information Delivery Specifications for BIM (mainly used for IFC)
https://www.buildingsmart.org/standards/bsi-standards/information-delivery-specification-ids/
Other
209 stars 64 forks source link

Reasoning behind maxOccurs="unbounded" on sequence in requirementsType #344

Open NiklasPor opened 1 month ago

NiklasPor commented 1 month ago

As many of you probably know, the order in which facets appears in applicability is predefined. On the first glance this is the same for the requirements, but it's not as easy as it appears.

// Excerpt from ids.xsd
  <xs:sequence maxOccurs="unbounded">
      <xs:element name="entity" minOccurs="0">...</xs:element>
      <xs:element name="partOf" minOccurs="0" maxOccurs="unbounded"></xs:element>
      <xs:element name="classification" minOccurs="0" maxOccurs="unbounded"></xs:element>
      <xs:element name="attribute" minOccurs="0" maxOccurs="unbounded">...</xs:element>
      <xs:element name="property" minOccurs="0" maxOccurs="unbounded">...</xs:element>
      <xs:element name="material" minOccurs="0" maxOccurs="unbounded">...</xs:element>
  </xs:sequence>

Notice that on the xs:sequence we have maxOccurs="unbounded" this enables the sequence to be repeated as many times as we want to. Combining this with optional elements inside the sequence, it's now possible to have ids files with orders which look different, but still are compliant.

This makes it not only possible to repeat the entity facet more than once inside the requirements, but also enables the user to ignore the ordering of the xs:sequence:

<ids:requirements>
    <ids:property>...</ids:property>
    <ids:material>...</ids:material>
    <ids:partOf>...</ids:partOf>
    <ids:material>...</ids:material>
    <ids:partOf>...</ids:partOf>
</ids:requirements>

Audit runs fine:

=== ids-tool - utility tool for buildingSMART IDS files.
info: idsTool.Program[0] Auditing: Ids structure, Ids content.
info: idsTool.Program[0] Auditing file: `/Users/niklaspor/Documents/GitHub/IDS-Audit-tool/ordering-test.ids`.
info: idsTool.Program[0] The file schema version is: Ids1_0
info: idsTool.Program[0] Completed reading 69 xml elements.
info: idsTool.Program[0] Completed with status: Ok.

ordering-test.ids.zip


I think this is rather confusing for the implementer / user. Inside the applicabilityType the order is required and enforced. Inside the requirementsType it look on the first glance like it's supposed to be ordered (xs:sequence) while in reality you can use any order.

As it was an explicit decision to use xs:sequence to "produce the text version of the content in a reliable and consistent way" (comment from @CBenghi ) I think it would be favorable to remove the maxOccurs="unbounded" from the requirementsType once the next breaking & major version of the schema is released.

If I'm missing any reason for the maxOccurs="unbounded" please tell me, I could not find anything while searching through issues & docs. Thanks for taking the time to read through this 👋

andyward commented 1 month ago

Don't we need maxOccurs="unbounded" so we can have test multiple propeties, attributes etc in a single specification? For entityType is clearly makes no sense as an entity can only be one.

I personally feel we should be replacing all xs:sequences with xs:any. I can see no benefit in enforcing the sequencing of the nodes? My recollection is the decision related to 'implementor simplicity' but as this shows it's creating other issues. (not to mention folks who may edit IDS in an editor without XSD support)

NiklasPor commented 1 month ago
  1. I don't think we need maxOccurs="unbounded" on the sequence, because all elements inside the sequence have maxOccurs="unbounded" themselves. So they can already be repeated, but just have to be placed after each other. Check the excerpt from ids.xsd at the top. The only element without maxOccurs="unbounded" is the entity facet – so this seems intended. (This is also how it works inside the applicabilityType where the sequence does not have maxOccurs="unbounded".

  2. Yes I'd also favor xs:any. Take a look at https://github.com/buildingSMART/IDS/issues/175#issuecomment-1744634900 where this topic was already discussed. (Although I'd still favor xs:any over xs:sequence)

andyward commented 1 month ago

Ah yes - I mis-read and thought you meant the maxOcurs on the elements. I agree the maxOccurs on the xs:sequence doesn't make sense - unless I'm missing something.

berlotti commented 1 month ago

We had this discussion before. My XSD validator (xmlspy) says 'any' is not valid XSD in that situation. Happy to take suggestions to change this, as long as XSD stays valid XSD

NiklasPor commented 1 month ago

@berlotti The xs:any is just a side note, what about the maxOccurs=unbounded on the requirementsType ?

// Update, I think if we wanted to replace the xs:sequence we should replace it with xs:all not xs:any

atomczak commented 1 month ago

The xs:all (and xs:any) is not suited, I think, because it only allows for one element of a kind (the maxOccurs can be only 0 or 1 and we need 'unbounded').

On the main topic, I think you're right, @NiklasPor. I don't see any reason for having maxOccurs=unbounded on sequence in requirementsType (L113). We want to be able to have for example: E-C-P-P, but not E-P-C-P. Right now, maxOccurs makes it possible.

Attaching sample file: SequenceTest.zip