Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
NiklasPor opened this issue Sep 14, 2024 · 6 comments
Open
Assignees
Labels
bug decide To vote on during next meeting schema Issues that impact the schema file
Milestone

Comments

@NiklasPor
Copy link

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
Copy link
Contributor

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
Copy link
Author

  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 Is xs:sequence intended? #175 (comment) where this topic was already discussed. (Although I'd still favor xs:any over xs:sequence)

@andyward
Copy link
Contributor

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
Copy link
Member

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
Copy link
Author

NiklasPor commented Sep 16, 2024

@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 atomczak added decide To vote on during next meeting schema Issues that impact the schema file labels Sep 16, 2024
@atomczak
Copy link
Contributor

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

@atomczak atomczak added the bug label Sep 16, 2024
@atomczak atomczak added this to the 1.1 milestone Sep 16, 2024
@atomczak atomczak self-assigned this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug decide To vote on during next meeting schema Issues that impact the schema file
Projects
None yet
Development

No branches or pull requests

4 participants