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

Update schema to not allow empty op array #1844

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Update schema to not allow empty op array #1844

merged 1 commit into from
Sep 27, 2023

Conversation

egekorkan
Copy link
Contributor

@egekorkan egekorkan commented Jul 5, 2023

fixes #1841
This is a bug fix in my opinion.

Note: I have mirrored the changes in Playground at eclipse-thingweb/playground#476 and verified that tests of valid and invalid TDs still pass.

@egekorkan egekorkan added Editorial Issues with no technical impact on implementations validation Topic related to Normative Parsing, Validation, Consumption labels Jul 5, 2023
@egekorkan
Copy link
Contributor Author

egekorkan commented Jul 5, 2023

Call of 05.07:

  • @relu91 We may have users who interpret the empty array as absence of op and fill in with defaults. We aren't clear in the spec about the array having no values at all
  • @lu-zero : This is repeated in other places, we should have the same behavior everywhere.

@egekorkan we should have a look to see where we allow empty arrays in other places.

The TF consensus is to not allow empty arrays and that defaults apply when a term is missing, not when its values are empty (empty objects or empty arrays or "" as a string).

Adding this clarification can be done in 2.0. We should not close the issue and reflect the changes to the text in 2.0

@egekorkan
Copy link
Contributor Author

  • @egekorkan will check the other places that have an array value, whether they allow 0 items in the array or not.
  • @egekorkan will check if there are any TDs submitted with empty op array (and other arrays if found in the point above)

@egekorkan
Copy link
Contributor Author

All the arrays (32 occurrences) in the current schema are listed below. The ones without minItems are also linked:

  • security: OK
  • scopes: does NOT have minItems:1
  • @context: has a bunch of rules to mandate minItems:1
  • @type: does NOT have minItems:1
  • oneOf: does NOT have minItems:1
  • enum: has minItems:1
  • items: does NOT have minItems:1
  • required: does NOT have minItems:1
  • additionalResponsesDefinition: does NOT have minItems:1
  • forms: OK
  • hreflang: does NOT have minItems:1
  • oneOf and allOf inside combosec: OK
  • scopes in oAuth2: does NOT have minItems:1
  • links: does NOT have minItems:1
  • profile: OK

Long story short, it is quite a mess. There are some that are obvious to me but JSON Schema itself does not mandate a string array to have at least one item. It even defaults to []. See line 32 here. This means we should not mandate minItems on required. BUT it mandates 1 item for schema arrays (array of data schema for us). See line 6 on the file above. This means that definition of oneOf and allOf should not have minItems.

links in OpenAPI has no restrictions. See here.

@lu-zero has raised the concern that these should be aligned but it may not be so simple to do so since the specs we follow are not aligned but they probably have a reason.

@egekorkan
Copy link
Contributor Author

egekorkan commented Sep 21, 2023

None of the TD inputs for testing have an empty op array. Regarding other empty arrays:

  • Ditto TMs have empty array for "tm:optional" and "tm:required". @thjaeckle is this part of the generator/parser and happens all the time?
  • @JKRhb sdf converter has one TM with the default value of a property being an empty array. See here. @JKRhb is this intentional?
  • One WebThing has @type with empty array. See here. This might make sense if all TDs are created with an empty @type and more are added if needed. @benfrancis any opinions on this? Noting that JSON-LD spec does not say anything about @type being empty is a problem.

@JKRhb
Copy link
Member

JKRhb commented Sep 21, 2023

Thank you for checking all the inputs, @egekorkan!

  • @JKRhb sdf converter has one TM with the default value of a property being an empty array. See here. @JKRhb is this intentional?

That is actually taken over from the original SDF model which served as the input for the conversion. I think in the context of the model, the empty array does make sense (to indicate an empty list of options as the default).

@lu-zero
Copy link
Contributor

lu-zero commented Sep 21, 2023

Thank you for tackling it, just few safety checks:

  • Do we have elements that have a non-empty default and a need to represent an empty value?
  • Do we want to support expressing this?

@egekorkan
Copy link
Contributor Author

Do we want to support expressing this?

For op, we don't need to or want to. For other values, we need to understand why they are used like that.

a need to represent an empty value?

I am not aware of it except for having an empty array as a value of an affordance input/output. E.g. one key of an object always returns [] and we express this via const:[] or similarly for default:[]. This is not an issue and not expressed in our schema anyways.

@relu91
Copy link
Member

relu91 commented Sep 21, 2023

I think the problem only arises when we allow [] and it is not a valid value for that property. For op it is problematic because implementations might interpret [] as undefined and fill in default values but others might reject the TD. Putting minitems in the dataschema ensures that we are clear and we a consistent behavior in different implementations. Maybe we should do the same for scopes and oneOf, but other properties seem ok to me.

@lu-zero
Copy link
Contributor

lu-zero commented Sep 21, 2023

Exactly, Ideally we could be extra explicit and use a notation like NonEmptyArray in 2.0 to not have this kind of misunderstanding in the spec

@thjaeckle
Copy link

  • Ditto TMs have empty array for "tm:optional" and "tm:required". @thjaeckle is this part of the generator/parser and happens all the time?

No, this is an information in the ThingModel. If the modeler chooses to put items in tm:optional then the array is non empty.
It all afforances are required, then this array stays empty.

The by Ditto generated TD does not contain tm:optional.
When creating a new thing in Ditto which references a ThingModel, Ditto will create a Json skeleton with default values for the properties of the TM and will not create those being marked as optional.

tm:required does no longer exist, or does it?

@danielpeintner
Copy link
Contributor

tm:required does no longer exist, or does it?

No, tm:optional is the replacement

see https://w3c.github.io/wot-thing-description/#changes-from-august-2022
image

@egekorkan
Copy link
Contributor Author

@thjaeckle quick question:

It all afforances are required, then this array stays empty.

Does it mean that you always start with an empty array and the modeler puts affordances as needed? Other option would be to start without the tm:optional and it is instantiated when the modeler makes an item optional

@thjaeckle
Copy link

@thjaeckle quick question:

It all afforances are required, then this array stays empty.

Does it mean that you always start with an empty array and the modeler puts affordances as needed? Other option would be to start without the tm:optional and it is instantiated when the modeler makes an item optional

I recently created a TM and I started with empty tm:optional, created first the properties and later checked which are optional and filled the array with those.
If the model already was published, I usually increase the micro version of the model when making formerly required properties optional.

Starting without tm:optional would also be good, to keep the model minimal.

@egekorkan
Copy link
Contributor Author

My opinion for today's meeting is to merge this PR and document all the other cases in an issue since I do not think that we have time to analyse where we should allow empty array and where not. Making them all aligned at the moment is not possible anyways since making tm:optional having minItems:1 would have an implementation impact, see above with Eclipse Ditto. Even if they are willing to make the changes asap, we should not require them to do so at this point.

@lu-zero
Copy link
Contributor

lu-zero commented Sep 27, 2023

I think it is enough to document the diverging cases either by defining OneOrMany, NoneOrMany, EmptyOrMany and using them where needed or just noting what's the expectation in case the field is present but empty.

@egekorkan
Copy link
Contributor Author

Call of 27.09: Merging and creating follow-up issue to analyze it in the next charter work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editorial Issues with no technical impact on implementations validation Topic related to Normative Parsing, Validation, Consumption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TD Schema allows empty op
6 participants