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

schemas: Changes to sync OGC API - Common review #953

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jerstlouis
Copy link
Member

@jerstlouis jerstlouis commented Sep 24, 2024

@jerstlouis
Copy link
Member Author

It seems like the correct thing should be "oneOf" rather than "anyOf" to reflect the mutual exclusion.

There's a discussion in the S/O post (which suggested anyOf) about this. I'm leaning towards "oneOf" and thinking of fixing Common to use "oneOf".

- The main change relates to the ability to describe meaning of enum values
  related to opengeospatial/ogcapi-coverages#173 (comment)
  using a combination of `anyOf` with `const` + `title` as described in
  https://stackoverflow.com/questions/64233370/in-json-schema-how-to-define-an-enum-with-description-of-each-elements-in-the-e
- Changes also introduce a new `x-ogc-nilValues` keyword for identifying nil values
- Other changes are gramamr fixes and minor clarifications
- Changing indirect dependency to QUDT in general as opposed to QUDT units since QUDT can also used for
  semantic definitions (not only QUDT units)
- Clarified that JSON Schema keywords can also include additional keywords in section below
Copy link
Member

@cportele cportele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the specific comments.

On "anyOf" vs "oneOf": If used for validation, both will deliver the same result, if all options are mutually exclusive. With "oneOf" validation will take longer, but one could argue that it expresses the semantics more clearly. Anyhow, I really think we should avoid recommending the use of "anyOf" or "oneOf".

@@ -9,7 +9,7 @@ The following normative documents contain provisions that, through reference in
* [[ogc06_103r4]] Open Geospatial Consortium (OGC). OGC 06-103r4: **OpenGIS® Implementation Standard for Geographic information - Simple feature access - Part 1: Common architecture**. Edited by J. Herring. 2011. Available at http://portal.opengeospatial.org/files/?artifact_id=25355
* [[qudtunits]] QUDT.org. **QUDT Units Vocabulary 2.1**. Edited by R. Hodgson. 2024. Available at https://www.qudt.org/doc/DOC_VOCAB-UNITS.html.
* [[qudt]] QUDT.org. **QUDT Ontologies, derived models and vocabularies 2.1**. Edited by R. Hodgson. 2024. Available at https://www.qudt.org/.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not correct. Only the QUDT Units Vocabulary is a normative reference. The QUDT Ontologies could be added to the bibliography.

Copy link
Member

@cportele cportele Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meeting 2024-10-07: This change should be reversed. QUDT can be added to the bibliography.

@@ -12,7 +12,7 @@ The Requirements Class "Schemas" specifies basic provisions for schemas of items
|Indirect Dependency |<<json-schema-validation,JSON Schema Validation: A Vocabulary for Structural Validation of JSON>>
|Indirect Dependency |<<ogc06_103r4,Simple feature access - Part 1: Common architecture>>
|Indirect Dependency |<<ucum,The Unified Code for Units of Measure>>
|Indirect Dependency |<<qudtunits,QUDT Units Vocabulary>>
|Indirect Dependency |<<qudt,QUDT Ontologies, derived models and vocabularies>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not correct. Only the QUDT Units Vocabulary is normatively referenced from provisions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meeting 2024-10-07: Same as above, this should be reversed.

^|J |Required properties SHOULD be included in "required".
^|K |The JSON Schema keywords SHOULD be constrained to the those mentioned in this recommendation and requirement `/req/{req-class}/properties`.
^|H |For integer properties that represent enumerated values without providing a description for each, "enum" SHOULD be provided.
^|I |For integer properties that represent enumerated values while providing a description for each, "oneOf" with a combination of "const" and "title" for each entry SHOULD be provided.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using "enum" for all lists of enumerated values is easier to parse and more predictable. "oneOf" is always more problematic as one never knows how complex the different options are. And "oneOf" is often not supported in tooling.

Having descriptions for each enum could also be handled as an orthogonal capability. In ldproxy, for example, we support references to a dictionary/object that maps codes/values to titles (we call them codelists). An example: schema and a codelist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meeting 2024-10-07: Both options are valid and they have their drawbacks. We need more time to think about this more and work this out. @cportele will document other options.

|===
^|*Requirement {counter:req-num}* |/req/{req-class}/{req}
^|A |The keyword "x-ogc-nilValues" SHALL be used to identify the values considered nil.
^|B |The value of the keyword "x-ogc-nilValues" SHALL be an array of objects, each including a mandatory "const" value and an optional "title" explanation of the meaning of that particular nil value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Using "enum" seems to be cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants