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

feat: add idempotency guidance (#456) #479

Merged
merged 10 commits into from
Jan 21, 2019
Merged

Conversation

tkrop
Copy link
Member

@tkrop tkrop commented Jan 15, 2019

Fixes #456.

Copy link
Contributor

@duergner duergner left a comment

Choose a reason for hiding this comment

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

Looks fine in general.

Having actual changes mixed with pure reformatting makes review not that easy :-)

chapters/common-headers.adoc Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Show resolved Hide resolved
@ePaul
Copy link
Member

ePaul commented Jan 17, 2019

👍 for the actual rule changes, I didn't review the formatting stuff in detail.

@tkrop
Copy link
Member Author

tkrop commented Jan 17, 2019

@duergner @maxim-tschumak @ePaul Thanks for reviewing so far. Has one had the time to although have a look at spelling mistakes and grammar so far?

@tfrauenstein
Copy link
Member

👍 Nice work, thx.

|====================================================

[#229]
== {SHOULD} Consider To Design `POST` and `PATCH` Idempotent
Copy link
Contributor

@vadeg vadeg Jan 17, 2019

Choose a reason for hiding this comment

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

Should it be MAY instead? According to RFC-2119 for SHOULD

mean that there
   may exist valid reasons in particular circumstances to ignore a
   particular item, but the full implications must be understood and
   carefully weighed before choosing a different course.

As far as I understand this in context of API guideline it means that I should provide a valid reason NOT to not implement POST and PATCH idempotent. In other words if POST implemented, then it should be idempotent, otherwise provide a valid reason not doing this. I think it is too "strong".

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, nice discussion start.

I personally would like it to be understood this way. There are to many drawbacks when having non-idempotent behavior, e.g. for create zombie or duplicate resources, for update incorrect values, etc.

I would vote clearly for SHOULD.

chapters/common-headers.adoc Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
@tkrop
Copy link
Member Author

tkrop commented Jan 18, 2019

@vadeg @maxim-tschumak @tfrauenstein @duergner @whiskeysierra @ePaul @zeitlinger @gewald Thanks for all the great feedback and discussions. I have tried to incorporate all of them. Lets go into the last round for approvals, prove reading, and or voting for #479 (comment).

I would really like to merge this work today 😸!

@maxim-tschumak
Copy link
Contributor

👍

@tfrauenstein
Copy link
Member

👍

Copy link
Contributor

@meshcalero meshcalero left a comment

Choose a reason for hiding this comment

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

Typo in GET with Body:
s/that may conflicts/that may conflict/

@vadeg
Copy link
Contributor

vadeg commented Jan 18, 2019

👍

request body. The _secondary key_ is stored permanently in the resource. It
allows to ensure <<idempotent>> behavior by looking up the resource in case
of multiple independent resource creations from different clients (see
<<231>>).
Copy link

@cjbooms cjbooms Jan 18, 2019

Choose a reason for hiding this comment

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

Can you explain this a bit better, what is a a "resource specific secondary key" ?
Is it a property that has a uniqueness constraint enforced server-side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. Do you think I should add "(property with uniqueness constraint)" here? Or would it be enough to add this to rule 231, that is linked here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjbooms tried to be clearer about uniqueness requirements. Please have a look.

Copy link

@cjbooms cjbooms Jan 21, 2019

Choose a reason for hiding this comment

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

I think a property with uniqueness constraint enforced server-side is a more concise definition. It covers the wide variety of ways this could be achieved, and is not at all ambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

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

@maxim-tschumak
Copy link
Contributor

👍

1 similar comment
@tfrauenstein
Copy link
Member

👍

@tkrop tkrop merged commit 3f861d6 into master Jan 21, 2019
@tkrop tkrop deleted the 456-add-idempotency-guidance branch January 21, 2019 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants