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

ADD upsert operation #3217

Merged
merged 6 commits into from
Jun 26, 2018
Merged

ADD upsert operation #3217

merged 6 commits into from
Jun 26, 2018

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Jun 15, 2018

Implements #3215

Requested LGTMs: @kzangeli (mainly for the code and test parts) @jmcanterafonseca (mainly for the .apib parts)


+ Response 201
+ Response 201 204
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how this should be included in the .apib

  • Enumerating the possible 2xx codes? (as it is now)
  • Using two + Response blocks, one for 201 the other for 204?
  • Using only 201 as an example (as it was before)?

@jmcanterafonseca some advice on this would be great, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to add a new response code

Copy link
Member Author

Choose a reason for hiding this comment

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

Does .apib accept to have something like this?

+ Response 201
   ...
+ Response 204
  ...

(Maybe is a naïve question, but we haven't used that pattern in the .apib up to now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

New block added in 6566b8a... although I'm not sure if works this way (as asked in my previous comment).

Copy link
Member

@kzangeli kzangeli left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1074,7 +1074,8 @@ the JSON entity representation format (described in a "JSON Entity Representatio

Response:

* Successful operation uses 201 Created. Reponse includes a `Location` header with the URL of the
* Successful operation uses 201 Created (if upsert option is not used) or 204 No Content (if
upsert option is used). Reponse includes a `Location` header with the URL of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Only Location header will be present when there was actually an entity creation

Copy link
Member Author

Choose a reason for hiding this comment

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

I think is better to include always the Location heaader. It will be redudant in the case of update, but at the same time it offerst a less variable response interface for clients. It doesn't hurt.

@@ -1106,8 +1107,10 @@ Response:
+ Members
+ keyValues - when used, the request payload uses the `keyValues` simplified entity
representation. See "Simplified Entity Representation" section for details.
+ upsert - when used, entity is updated if already exits (otherwise this situation causes
a 422 Unprocessable Entity error).
Copy link
Contributor

Choose a reason for hiding this comment

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

clarify this better, perhaps putting that sentence separate

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to clarify in 6566b8a


+ Response 201
+ Response 201 204
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to add a new response code

Copy link
Contributor

@jmcanterafonseca jmcanterafonseca left a comment

Choose a reason for hiding this comment

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

LGTM with the minor change fixed

@@ -1106,13 +1107,21 @@ Response:
+ Members
+ keyValues - when used, the request payload uses the `keyValues` simplified entity
representation. See "Simplified Entity Representation" section for details.
+ upsert - when used, entity is updated if already exits. If upsert is not used and
Copy link
Contributor

Choose a reason for hiding this comment

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

use backtick for 422 Unprocessable Entity

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in f01b3c3

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants