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

Handle unset optional properties #47

Merged
merged 15 commits into from
Jun 13, 2019
Merged

Conversation

ctoher
Copy link
Contributor

@ctoher ctoher commented Jul 12, 2018

Dear all,

I've inserted a first draft of the section on how to handle unset optional properties (i.e. properties with "null" values - #17 on the list of issues), as per the discussion during the meeting in Lausanne.

I've added this as a new subsection 3.3.4, but let me know if you think some of the information would be better suited in a different section, e.g. if the " IS KNOWN" filter option should be in the filter section instead. Please let me know if you have any other comments, questions or suggestions on this topic.

Best regards

Cormac.

Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

This is based on the OPTIMaDe meeting discussion. This looks very good. We have a few changes suggested to formalize SHOULD and MUST in a few places. And there is one pagragraph about multi-valued properties that we did not quite understand, where we suggest a replacement.

And, also, there is an checked in .DS_Store file that should be removed from the pull request.

optimade.md Outdated

Note that this only applies to properties that are described by a single value,
and not to properties with multiple attributes such as the structure.
In this case, attributes that have null value are simply omitted from the returned structure.
Copy link
Contributor

Choose a reason for hiding this comment

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

In an OPTIMaDe meeting discussion, we note that we have some trouble understanding exactly what this means. What we think is needed is to say that the text in this section only describes how the api handles properties that themselves are null, and does not regulate which values inside the data structure used by a properties can be null. That is rather to be described in relation to the definition of those data structures.

So:

Suggested change
In this case, attributes that have null value are simply omitted from the returned structure.
The text in this section describes how the api handles properties that are null. It does not regulate the handling of values inside property data structures that can be null. That use of null values is described in relation to the definition of those data structures.

(Note: I cannot get github to make a suggestion over several lines, but this is meant to replace the whole paragraph.)

optimade.md Outdated
Unset optional properties in a database are properties that exist and have a specific value within a database for some materials entries,
but are undefined for other entries, i.e. have the value 'null' within a JSON file or SQL database.

Unset properties are not returned in the response unless explicitly requested in the search query.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Unset properties are not returned in the response unless explicitly requested in the search query.
Unset properties MUST NOT be returned in the response unless explicitly requested in the search query.

optimade.md Outdated
but are undefined for other entries, i.e. have the value 'null' within a JSON file or SQL database.

Unset properties are not returned in the response unless explicitly requested in the search query.
Any comparisons involving unset properties should return a value of false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Any comparisons involving unset properties should return a value of false,
Any comparisons involving unset properties MUST be evaluated as false,

optimade.md Outdated

If a property is explicitly requested in a search query without value range filters,
then all entries otherwise satisfying the query should be returned, including those with null values for this property.
The value of these properties should be set to the JSON 'null' value in the response.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The value of these properties should be set to the JSON 'null' value in the response.
These properties MUST be set to the JSON 'null' value in the response.

optimade.md Outdated
i.e. by definition the value of 'null' is outside of any defined search range.

If a property is explicitly requested in a search query without value range filters,
then all entries otherwise satisfying the query should be returned, including those with null values for this property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
then all entries otherwise satisfying the query should be returned, including those with null values for this property.
then all entries otherwise satisfying the query SHOULD be returned, including those with null values for this property.

@ctoher
Copy link
Contributor Author

ctoher commented Feb 22, 2019

Dear all,

I've updated the draft of the section on how to handle unset optional properties (i.e. properties with "null" values - #17 on the list of issues), to incorporate Rickard's suggested changes.

I've slightly modified his suggestion for the last sentence of the paragraph about null values inside property data structures to try to make it clearer - let me know if this makes sense, or if you think something else would work better.

Best regards

Cormac.

@giovannipizzi
Copy link
Contributor

After discussion in the skype call, what is missing in this PR is an update of the grammar. This can be done at the same time (or right after having discussed) issue #58

@ctoher
Copy link
Contributor Author

ctoher commented Jun 12, 2019

Dear all,

I've updated the specification for the handling of "null" properties. In particular, I updated the "IS KNOWN" and "IS UNKNOWN" discussion, including a cross-reference to section 5.2 (the filter specification where IS KNOWN and IS UNKNOWN are described). Please let me know if you have any comments or suggestions.

Best regards

Cormac.

optimade.md Outdated
@@ -377,6 +378,30 @@ the next course of action SHOULD be to fetch the resource objects under the
to the corresponding database ID that was originally queried, using the object's
`base_url` value.

### <a name="h.3.3.4">3.3.4. Unset optional properties</a>

Unset optional properties in a database are properties that exist and have a specific value within a database for some materials entries, but are undefined for other entries, i.e. have the value 'null' within a JSON file or SQL database.
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent, I would always write null or 'null' as null instead, with backticks - both here and in a number of places below.

optimade.md Outdated
@@ -377,6 +378,30 @@ the next course of action SHOULD be to fetch the resource objects under the
to the corresponding database ID that was originally queried, using the object's
`base_url` value.

### <a name="h.3.3.4">3.3.4. Unset optional properties</a>

Unset optional properties in a database are properties that exist and have a specific value within a database for some materials entries, but are undefined for other entries, i.e. have the value 'null' within a JSON file or SQL database.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we want to mention SQL here. Maybe just say "i.e. have the value null or are missing in the data source (that can be a JSON file, a database, ...)" or something like this? (assuming I'm understanding what you want to say here)

optimade.md Outdated

Unset properties MUST NOT be returned in the response, unless explicitly requested in the search query.

Any comparisons involving unset properties MUST be evaluated as false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, I would use false instead of false?

@ctoher
Copy link
Contributor Author

ctoher commented Jun 13, 2019

I fixed the null and false formatting as per Giovanni's comment. Let me know if there are any other formatting changes that should be made.

I removed the mention of SQL; this part now reads: "e.g. have the value null within a JSON file." The idea behind this part is just to give a concrete example of the types of values we're talking about; let me know if you think we should write this in a different way.

@ltalirz ltalirz added the PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing label Jun 13, 2019
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Approved

@rartino rartino merged commit fc5a1b1 into Materials-Consortia:develop Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants