Skip to content

feat: AIP-154 – Resource freshness validation#4

Merged
lukesneeringer merged 15 commits intomainfrom
aip-154
Feb 22, 2021
Merged

feat: AIP-154 – Resource freshness validation#4
lukesneeringer merged 15 commits intomainfrom
aip-154

Conversation

@lukesneeringer
Copy link
Copy Markdown
Contributor

No description provided.

@lukesneeringer lukesneeringer requested a review from a team as a code owner September 3, 2020 21:44
@lukesneeringer lukesneeringer changed the title feat: AIP-154 (Resource freshness validation) feat: AIP-154 – Resource freshness validation Sep 3, 2020
Copy link
Copy Markdown

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

I left a few minor comments I think it would be good to address before merging.

Also, the IBM API Handbook has several additional requirements for Etags / If-Match you might want to consider.

  • An Etag MUST be supplied [edit: by the service] for any resources which support the If-Match or If-None-Match headers for any methods.

  • If supported, the ETag SHOULD be a quoted lowercase base-36 string, at least 16 characters in length (e.g., "md9weho39cn2302n") and MUST be based on a checksum or hash of the resource that guarantees it will change if the resource changes.

  • Even if W/-prefixed, an ETag MUST be guaranteed to change if any properties are changed that are directly mutable by a client.

  • If any conditional headers are supported for any operation within a service, the same conditional headers MUST be supported for all methods of any path that supports them and SHOULD be supported uniformly for all operations across the service.

  • If a client sends an unsupported conditional header with a request, the server SHOULD return a 400 error with an error model indicating that the header is unsupported.

  • If any validator or conditional headers are supported for any operations in the service, such an error MUST be returned for all unsupported conditional headers across all operations. (In other words, once a service gives the client reason to believe it understands conditional headers, it MUST NOT ever ignore them.)

Comment thread aip/general/0154/aip.md Outdated
Comment thread aip/general/0154/aip.md Outdated
ETag: "55cc0347-66fc-46c3-a26f-98a9a7d61d0e"
```

The etag field **must** be provided by the server on output, and values
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here we say the etag must be provided but on line 15 we way that that it may be provided. Maybe there's some subtle distinction or qualification but if so it isn't clear to me.

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

Discussed in VC...

I left a few minor comments I think it would be good to address before merging.

Also, the IBM API Handbook has several additional requirements for Etags / If-Match you might want to consider.

  • An Etag MUST be supplied for any resources which support the If-Match or If-None-Match headers for any methods.

Clarification: supplied by the service (rather than by the user)

  • If supported, the ETag SHOULD be a quoted lowercase base-36 string, at least 16 characters in length (e.g., "md9weho39cn2302n") and MUST be based on a checksum or hash of the resource that guarantees it will change if the resource changes.

@dhudlow Would appreciate a justification for the first half of this. Everyone agreed on the second half.

  • Even if W/-prefixed, an ETag MUST be guaranteed to change if any properties are changed that are directly mutable by a client.

@dhudlow Maybe this should be should? We have had some exception cases.

  • If any conditional headers are supported for any operation within a service, the same conditional headers MUST be supported for all methods of any path that supports them and SHOULD be supported uniformly for all operations across the service.

Seems like this should be POST/PUT/PATCH/DELETE only?

  • If a client sends an unsupported conditional header with a request, the server SHOULD return a 400 error with an error model indicating that the header is unsupported.

Seems like this requires services that do not know about etags to actively reject them. This seems unlikely to ever be done in practice no matter what the law says.

  • If any validator or conditional headers are supported for any operations in the service, such an error MUST be returned for all unsupported conditional headers across all operations. (In other words, once a service gives the client reason to believe it understands conditional headers, it MUST NOT ever ignore them.)

Seems like this should be POST/PUT/PATCH/DELETE only?

@hudlow
Copy link
Copy Markdown

hudlow commented Jan 19, 2021

If supported, the ETag SHOULD be a quoted lowercase base-36 string, at least 16 characters in length (e.g., "md9weho39cn2302n") and MUST be based on a checksum or hash of the resource that guarantees it will change if the resource changes.

@dhudlow Would appreciate a justification for the first half of this. Everyone agreed on the second half.

I cannot justify it. I think the first half is overly prescriptive.

Even if W/-prefixed, an ETag MUST be guaranteed to change if any properties are changed that are directly mutable by a client.

@dhudlow Maybe this should be should? We have had some exception cases.

Would be interested to hear them.

Seems like this should be POST/PUT/PATCH/DELETE only? (x2)

I think I can get behind that.

If a client sends an unsupported conditional header with a request, the server SHOULD return a 400 error with an error model indicating that the header is unsupported.

Seems like this requires services that do not know about etags to actively reject them. This seems unlikely to ever be done in practice no matter what the law says.

I stand by it. :-)

Comment thread aip/general/0154/aip.md Outdated
@google-cla google-cla Bot added the cla: yes label Feb 2, 2021
Comment thread aip/general/0154/aip.md Outdated
@lukesneeringer
Copy link
Copy Markdown
Contributor Author

TODO: Add discussion about GET and the conditional headers.

@hudlow
Copy link
Copy Markdown

hudlow commented Feb 2, 2021

@lukesneeringer Nitpick for your next pass: inconsistent capitalization of "ETag" :-)

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

TODO: Add discussion about GET and the conditional headers.

Added.

@lukesneeringer Nitpick for your next pass: inconsistent capitalization of "ETag" :-)

Fixed.

Comment thread aip/general/0154/aip.md Outdated
Luke Sneeringer and others added 2 commits February 2, 2021 12:55
Co-authored-by: Mike Kistler <mkistler@us.ibm.com>
Comment thread aip/general/0154/aip.md Outdated
Comment thread aip/general/0154/aip.md Outdated
Comment thread aip/general/0154/aip.md
ETag: W/"55cc0347-66fc-46c3-a26f-98a9a7d61d0e"
```

Strong ETags **must** and weak ETags **should** be guaranteed to change if any
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Decision thread: We decided that weak etags only should be guaranteed to change based on mutable properties, because there are exception cases where a mutable property is unimportant.

Copy link
Copy Markdown
Contributor Author

@lukesneeringer lukesneeringer Feb 9, 2021

Choose a reason for hiding this comment

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

Discussion:

  • Should we say that strong etags must change if the resource representation changes? (Group leaning toward yes.)
    • Discussion about "byte for byte": we do not want this. JSON whitespace is insignificant, for example.
    • What about if you delete and recreate? That may be byte for byte identical but the etag would probably change.
  • Equivalence vs. equality: etags probably should indicate equivalence.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some very tricky equivalence issues for JSON:

  1. Object field order, which technically can be meaningful in JavaScript and JSON. Bonus: duplicate property fields. (We disallow returning duplicate property fields because of ambiguity but many JSON deserializers silently choose a value from one of them.)
  2. Variations in precision of numbers such as between 1, 1.0, 1.0000000000000001 which may not (and, indeed, probably will not) be distinguishable to deserializers.

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

@mkistler Ping! :-)

Base automatically changed from master to main February 10, 2021 22:07
Copy link
Copy Markdown

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

What's here looks fine to me. 👍

But I wonder why we don't have an interface definitions section in this AIP like we do in many others. I think it would be worthwhile showing examples of documenting the ETag response header, the If-Match header parameter, noting that it should be required if the service does require it. We could further illustrate a nice OpenAPI feature called "links" that makes explicit the flow of information from the ETag response header to the If-Match header parameter. Should we make this the subject of a follow-on PR?

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

But I wonder why we don't have an interface definitions section in this AIP like we do in many others. I think it would be worthwhile showing examples of documenting the ETag response header, the If-Match header parameter, noting that it should be required if the service does require it. We could further illustrate a nice OpenAPI feature called "links" that makes explicit the flow of information from the ETag response header to the If-Match header parameter. Should we make this the subject of a follow-on PR?

This is a great point. I think I also agree with not blocking this PR on it -- I will open a follow-up PR that adds the If-Match example, and you can fill in the OpenAPI links.

@lukesneeringer lukesneeringer merged commit 353e9af into main Feb 22, 2021
@lukesneeringer lukesneeringer deleted the aip-154 branch February 22, 2021 20:19
lukesneeringer pushed a commit that referenced this pull request Feb 22, 2021
Based on a comment from mkistler@ in #4.
lukesneeringer pushed a commit that referenced this pull request Apr 20, 2021
Based on a comment from mkistler@ in #4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants