fix(admin) return HTTP 404 on PUT with non-existing PK #3007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Background ---------- Due to unfortunate legacy design decisions, our HTTP PUT handlers behave in the following way as of today: 1. if the payload does not have the entity's primary key values, we attempt an insert() 2. if the payload has primary key values, we attempt an update() Issue ----- While acknowledging this isn't the appropriate/desired behavior for a PUT handler, #2774 has raised the following concern: > When a PUT payload contains an non-existing primary key (attempt to create an entity), the endpoint responds HTTP 200 OK, but the entity is not created, and the response body is empty. Root cause & resolution ----------------------- This was due to a mis-usage of the `dao:update()` API. While this change will **not** allow #2774 to work the way it is expected of it to work, this will help alleviate confusion by at least returning the appropriate HTTP status code, at least in the way our Admin API implements PUT handlers... Appendix -------- In the future, and as we are working towards a new model comprised of a new Admin API, we promise ourselves to implement a more appropriate behavior for PUT endpoints. Fix #2774
|
Seems sane; do we have a doc update to go with it? |
On my TODO list! 😉 |
kikito
approved these changes
Nov 3, 2017
thibaultcha
added a commit
to Kong/docs.konghq.com
that referenced
this pull request
Nov 8, 2017
As a follow-up to: Kong/kong#3007 (PR) Kong/kong#2774 (issue) This commit adds clarifications as to the current behavior of PUT endpoints on the Admin API. As much as we acknowledge that their behavior is not compliant with the HTTP specifications, we do wish to clarify their behavior to avoid confusion as of today, until a different implementation sees the light of day.
|
@p0pr0ck5 Have a look at Kong/docs.konghq.com#548 for docs updates clarifying this behavior! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Due to unfortunate legacy design decisions, our HTTP PUT handlers behave
in the following way as of today:
we attempt an insert()
Issue
While acknowledging this isn't the appropriate/desired behavior for a
PUT handler, #2774 has raised the following concern:
Root cause & resolution
This was due to a mis-usage of the
dao:update()API.While this change will not allow #2774 to work the way it is
expected of it to work, this will help alleviate confusion by at least
returning the appropriate HTTP status code, at least in the way our
Admin API implements PUT handlers...
Appendix
In the future, and as we are working towards a new model comprised of a
new Admin API, we promise ourselves to implement a more appropriate
behavior for PUT endpoints.
Fix #2774