Common rules for inclusion and querying #210

merkys · 2019-11-25T12:07:24Z

In discussions with @rartino and @CasperWA over #184 the following ideas over property inclusion have been crystallized:

There should be a small set of properties that MUST be included in the response by default;
response_fields may be used to include optional properties.

Similarly,

There should be a small set of properties that MUST support querying;
Other properties support querying OPTIONALLY.

This PR implements these ideas by adding general notices and cleaning up the 'Entry List' section to explicitly mark only those properties that either included by default or must support querying. (I know, this goes against @CasperWA's idea of having a property reference there, but I have little ideas how to do it otherwise. Machine-readable specs maybe? 😕). Thus fixes #204.

Also this PR:

Makes coordinate-based properties included upon request only (fixes Include properties in response upon requests #184, closes Extend response_fields to support field exclusion #181, thus includes Change of inclusion status for coordinate-related properties #203)
Makes 'lattice_vectors' mandatory in the response (affects Format of 'lattice_vectors' is unclear for 2D, 1D and 0D systems #196)
Reduces query support requirement for nsites from MUST to OPTIONAL (includes Relieve the requirement level of query support for 'nsites' from MUST to OPTIONAL #205)
Forbids exclusion of id and type (fixes Exclusion of 'id' and 'type' properties upon request #209)

…uests.

…try Listing Endpoints' covers this now.

…excluded' to '**Response**: REQUIRED in the response' as paragraph in 'Entry Listing Endpoints' covers this now.

… on #206.

… queryable.

…s is now covered in 'API Filtering Format Specification'.

…205).

rartino · 2019-11-25T22:57:35Z

Thank you for doing the job with this PR! This will greatly help to avoid a situation with all implementations having slightly incompatible interpretations of the rules of when to, and when not to, include output fields.

However, with the edits you've made so far in this PR, don't you have to also update the definition of response_fields in 4.1.1, i.e.,from

response_fields: a comma-delimited set of fields to be provided in the output. If provided, only these fields MUST be returned and no others. Example: http://example.com/optimade/v0.9/structures?response_fields=id,url

into something like

response_fields: a comma-delimited set of optional fields to be provided in the output. If provided, the implementation MUST include in its response both the indicated fields and the fields marked as REQUIRED for the endpoint. Fields for which no meaningful value can be given MUST still be included with a null value. The implementation MUST NOT return any other fields. Example: http://example.com/optimade/v0.9/structures?response_fields=immutable_id,chemical_formula_hill

merkys · 2019-11-26T07:19:54Z

Right, the suggested change to response_fields would fix #209 as well. However, I am a little hesitant about bundling that many changes in a single PR, as requirements of other implementations might conflict. Maybe let's wait for today's Web meeting to discuss it with wider audience, OK?

CasperWA

Thanks @merkys!

I think you can definitely include the changes to response_fields here as well, as was suggested by @rartino. While it is a lot of changes in one PR, they are deeply connected. In the end, we do not squash and commit, so the commits will still be separate no matter what. So for the purposes of the PR, it is nice to have it done all-in-one :)

optimade.rst

CasperWA · 2019-12-02T13:26:08Z

optimade.rst

@@ -1656,8 +1656,7 @@ lattice\_vectors
 - **Type**: list of list of floats.
 - **Requirements/Conventions**:

-  - **Response**: REQUIRED in the response unless explicitly excluded, except when property `dimension_types`_ is equal to :val:`[0, 0, 0]` (in this case it is OPTIONAL).
-  - **Query**: Support for queries on this property is OPTIONAL. If supported, filters MAY support only a subset of comparison operators.
+  - **Response**: REQUIRED in the response.


I presume it is intentional to leave out the conditional here?

Yes. The intent was to simplify the rules for inclusion/exclusion of properties. Also I had PR #206 in mind while developing the current PR.

optimade.rst

CasperWA · 2019-12-02T13:29:53Z

optimade.rst

@@ -1977,7 +1965,6 @@ Database-provider-specific entry types MUST have all properties described above

 - **Requirements/Conventions for properties in database-provider-specific entry types**:

-  - **Response**: OPTIONAL in the response.
  - **Query**: Support for queries on these properties are OPTIONAL.


You've deleted this for some that are OPTIONAL in the response, but not all. How do you justify this? Or rather, what is the implication?

You are right. I was a little hesitant to cleanup the requirements, but I think with the addition of general clarification in 502bd44 the rest of the optional clauses can be removed. Did so in 8a6a4b2 and 892f60a.

Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

rartino

In my opinion, every PR MUST leave the full specification internally consistent. Without editing the description of response_fields I don't think this PR fulfills that requirement.

We cannot simultaneously have:

The response_fields parameter described as "only these fields MUST be returned and no others."
A list of fields marked "REQUIRED in the response" with no indication that a valid response can (and should) exclude them if the response_fields parameter is used and they are not included.

Hence, I think the easiest solution to this problem by far is to also integrate the fix that was proposed for #209 into this PR, and simply say that REQUIRED properties are always REQUIRED in the response, and resonse_fields only indicates extra fields to include on top of those.

Otherwise, you need to revert the unless otherwise excluded caveats and look over the rest of the text for internal consistency with how response_fields is described to work in the present version.

optimade.rst

…ally queryable properties.

…e of properties.

…perties.

Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>

rartino · 2019-12-18T09:34:46Z

I had forgotten to merge the current state of develop. And, oh wow, that was a painful merge.

(I've tried to be careful, and my interpretation is that any bad mistake ought to show up in the diff for the present PR, since it would be reverting changes in the present state of develop. But, I'm honestly not sure on that; I remember a case earlier in the history of optimade.md where we managed to revert some changes in an unclean merge. We couldn't even trace back in which merge it had happened, there were 3 or so candidates...)

merkys · 2019-12-18T13:12:15Z

I had forgotten to merge the current state of develop. And, oh wow, that was a painful merge.

I trust you did the merge right :) OK, so we need yet one more approve.

rartino · 2019-12-18T16:19:56Z

Pinging @CasperWA, @sauliusg or @giovannipizzi for a review!

CasperWA

Finally got around to going through this - sorry for the delay.

I really like the new "Supported".
Thanks for all the hard work that has been put into this PR!

Some things I noticed:

"Default properties" don't seem to be defined anywhere, but maybe that was just me missing it?
In the Entry list section, there are intermxed uses of either filter features or filter operators. I like both, and both have merit, I just wanted to point it out, to make sure this is a desired change by all - since they do have slightly different meanings/connotations.
In a similar vein, comparison operators vs. filter operators are used for some properties and not others. These make a lot of sense, but we should make sure they are used correctly when changed (or used).

optimade.rst

CasperWA · 2019-12-19T20:53:36Z

optimade.rst

@@ -310,16 +310,13 @@ The text in this section describes how the API handles properties with the value
 The use of :val:`null` values inside nested property values (such as, e.g., lists or dictionaries) are described in the definitions of those data structures elsewhere in the specification, see section `Entry List`_.
 For these properties, :val:`null` MAY carry a special meaning.

-REQUIRED properties with an unknown value MUST be returned in the response, unless explicitly left out (e.g., by using :query-param:`response_fields`, see section `Entry Listing URL Query Parameters`_).
+Responses that include entries where REQUIRED properties have an unknown value MUST include these properties with the value :val:`null`.


This sentence is a bit difficult to read, I think. How about:

Suggested change

Responses that include entries where REQUIRED properties have an unknown value MUST include these properties with the value :val:`null`.

REQUIRED properties with an unknown value MUST be included and returned in the response.

I've edited into:

REQUIRED properties with an unknown value MUST be included and returned in the response with the value :val:null.

CasperWA · 2019-12-19T21:31:37Z

optimade.rst


-OPTIONAL properties with an unknown value MAY be returned in the response.
-If an OPTIONAL property is *not* returned in a *full* response (i.e., not using :query-param:`response_fields`), the client MUST assume the property has an unknown value, i.e., :val:`null`.
+Responses to requests where OPTIONAL properties have been explicitly requested via the :query-param:`response_fields` query parameter, and which include entries where those properties have an unknown value, MUST include these properties with the value :val:`null`.


This may then also be rewritten.

Suggested change

Responses to requests where OPTIONAL properties have been explicitly requested via the :query-param:`response_fields` query parameter, and which include entries where those properties have an unknown value, MUST include these properties with the value :val:`null`.

OPTIONAL properties, if requested explicitly via the :query-param:`response_fields` query parameter, MUST be included and returned in the response.

Your version misses "with an unknown value". Sure, arguably you can say that the statement applies generally--- unknown value or not, but the point of this section is to clarify that these properties need to be in the response even if the value is unknown.

I've edited into:

OPTIONAL properties with an unknown value, if requested explicitly via the :query-param:response_fields query parameter, MUST be included and returned in the response with the value :val:null.

optimade.rst

rartino · 2019-12-20T00:01:06Z

@CasperWA Thanks for the careful review! Hopefully, this version is better.

…ilter operators'.

CasperWA

@CasperWA Thanks for the careful review! Hopefully, this version is better.

This is looking splendid. Thanks for being so fast and taking care of my late-night review and suggested changes - here are a handful more 😅

I found some typing mistakes and have tried to rephrase a couple of sentences, not much this time.

I think with these last comments taken care of, this should be ready to be merged.

optimade.rst

rartino · 2019-12-20T09:33:37Z

@CasperWA done!

CasperWA

Woot woot! I approve this hard work started by @merkys and finished by @rartino !
Thank you guys!

This is a somewhat major change, so we should definitely announce this change at the meeting today (but merge before, i.e., now).

So please merge @rartino, when you have the time.

merkys added 9 commits November 25, 2019 12:41

Describing which properties are returned by default in entry list req…

aef2e4d

…uests.

Adding a callback to 'Single Entry Endpoints'.

39b78d6

Dropping '**Response**: OPTIONAL in the response' as paragraph in 'En…

65de03c

…try Listing Endpoints' covers this now.

Shortening '**Response**: REQUIRED in the response unless explicitly …

746a4d8

…excluded' to '**Response**: REQUIRED in the response' as paragraph in 'Entry Listing Endpoints' covers this now.

Requiring 'lattice_vectors' in the response as per @rartino's comment…

2a393f0

… on #206.

Making coordinate-related properties OPTIONAL (includes #203).

724f6f7

Describing the general treatment of properties that are not marked as…

2b61396

… queryable.

Removing notices about optional usage of properties in filters as thi…

ade5532

…s is now covered in 'API Filtering Format Specification'.

Property 'nsites' is no longer required to support querying (includes #…

39d3b85

…205).

merkys requested review from rartino and CasperWA November 25, 2019 12:07

merkys self-assigned this Nov 25, 2019

merkys 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 Nov 25, 2019

merkys mentioned this pull request Nov 26, 2019

Property 'fractional_site_positions' #206

Open

Adjusting the definition of Queryable property.

f29f48b

merkys added this to the 1.0 release milestone Nov 28, 2019

CasperWA reviewed Dec 2, 2019

View reviewed changes

merkys and others added 2 commits December 2, 2019 17:31

Fixing vocabulary: 'entry description' -> 'response'.

c69d5f0

Splitting one sentence per line.

947a0b6

Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

rartino requested changes Dec 2, 2019

View reviewed changes

optimade.rst Show resolved Hide resolved

optimade.rst Outdated Show resolved Hide resolved

merkys mentioned this pull request Dec 3, 2019

Relieve the requirement level of query support for 'nsites' from MUST to OPTIONAL #205

Closed

merkys added 3 commits December 4, 2019 09:56

Adding a sentence on support of subset of filter operators for option…

502bd44

…ally queryable properties.

Simplifying requirements for the presence in the response for a coupl…

892f60a

…e of properties.

Simplifying requirements for the filter operators for a couple of pro…

8a6a4b2

…perties.

rartino added PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR and removed 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 labels Dec 4, 2019

Converting 'response_fields' to a list of additional optional fields.

55f542e

Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>

rartino mentioned this pull request Dec 4, 2019

Change of inclusion status for coordinate-related properties #203

Closed

rartino dismissed their stale review via 543b0fc December 18, 2019 09:30

Merge upstream develop

543b0fc

rartino previously approved these changes Dec 18, 2019

View reviewed changes

rartino requested a review from giovannipizzi December 18, 2019 16:19

CasperWA requested changes Dec 19, 2019

View reviewed changes

Further text fixes based on review

1d6a210

rartino dismissed their stale review via 1d6a210 December 19, 2019 23:55

Fix minor edit mistakes.

ad01ba2

rartino requested a review from CasperWA December 20, 2019 00:00

rartino previously approved these changes Dec 20, 2019

View reviewed changes

Improve inconsistency between usage of terms 'filter features' and 'f…

3f1c5e0

…ilter operators'.

rartino dismissed their stale review via 3f1c5e0 December 20, 2019 02:14

rartino previously approved these changes Dec 20, 2019

View reviewed changes

merkys assigned rartino Dec 20, 2019

CasperWA requested changes Dec 20, 2019

View reviewed changes

Further minor text fixes suggested in review.

fe86cd4

rartino dismissed their stale review via fe86cd4 December 20, 2019 09:30

rartino approved these changes Dec 20, 2019

View reviewed changes

rartino requested a review from CasperWA December 20, 2019 09:33

CasperWA approved these changes Dec 20, 2019

View reviewed changes

rartino merged commit b9d849a into Materials-Consortia:develop Dec 20, 2019

CasperWA mentioned this pull request Dec 20, 2019

Update models with new levels of REQUIRED response properties Materials-Consortia/optimade-python-tools#114

Closed

This was referenced Feb 3, 2020

New REQUIRED level properties Materials-Consortia/optimade-python-tools#153

Merged

Clarify required fields in a response #250

Open

merkys mentioned this pull request Sep 29, 2020

Change query support requirement from MUST to SHOULD for properties with support level SHOULD #319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common rules for inclusion and querying #210

Common rules for inclusion and querying #210

merkys commented Nov 25, 2019 •

edited

Loading

rartino commented Nov 25, 2019 •

edited

Loading

merkys commented Nov 26, 2019

CasperWA left a comment

CasperWA Dec 2, 2019

merkys Dec 2, 2019

CasperWA Dec 2, 2019

merkys Dec 4, 2019

rartino left a comment

rartino commented Dec 18, 2019

merkys commented Dec 18, 2019

rartino commented Dec 18, 2019

CasperWA left a comment

CasperWA Dec 19, 2019

rartino Dec 19, 2019

CasperWA Dec 19, 2019

rartino Dec 19, 2019 •

edited

Loading

rartino Dec 19, 2019

rartino commented Dec 20, 2019

CasperWA left a comment •

edited

Loading

rartino commented Dec 20, 2019

CasperWA left a comment •

edited

Loading

	Responses that include entries where REQUIRED properties have an unknown value MUST include these properties with the value :val:`null`.
	REQUIRED properties with an unknown value MUST be included and returned in the response.

	Responses to requests where OPTIONAL properties have been explicitly requested via the :query-param:`response_fields` query parameter, and which include entries where those properties have an unknown value, MUST include these properties with the value :val:`null`.
	OPTIONAL properties, if requested explicitly via the :query-param:`response_fields` query parameter, MUST be included and returned in the response.

Common rules for inclusion and querying #210

Common rules for inclusion and querying #210

Conversation

merkys commented Nov 25, 2019 • edited Loading

rartino commented Nov 25, 2019 • edited Loading

merkys commented Nov 26, 2019

CasperWA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rartino left a comment

Choose a reason for hiding this comment

rartino commented Dec 18, 2019

merkys commented Dec 18, 2019

rartino commented Dec 18, 2019

CasperWA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rartino Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rartino commented Dec 20, 2019

CasperWA left a comment • edited Loading

Choose a reason for hiding this comment

rartino commented Dec 20, 2019

CasperWA left a comment • edited Loading

Choose a reason for hiding this comment

merkys commented Nov 25, 2019 •

edited

Loading

rartino commented Nov 25, 2019 •

edited

Loading

rartino Dec 19, 2019 •

edited

Loading

CasperWA left a comment •

edited

Loading

CasperWA left a comment •

edited

Loading