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

Return 201 or 202 on Document.save() #49

Closed
adrienverge opened this issue Oct 5, 2022 · 10 comments · Fixed by #53
Closed

Return 201 or 202 on Document.save() #49

adrienverge opened this issue Oct 5, 2022 · 10 comments · Fixed by #53
Labels
bug Something isn't working

Comments

@adrienverge
Copy link
Contributor

Hello @bmario,

We need to know whether the PUT /db/doc route returned a HTTP 201 or a HTTP 202. Knowing this info is part of the API and documented, so it's legitimate for an aiocouch user to be able to get this return code.

201 Created – Quorum met, document created and stored on disk
202 Accepted – Quorum not met, document data accepted but not yet stored on disk and no guarantee that it will be accepted my the majority of nodes

Would you be favorable to an improvement to Document.save() in order to return this info? Potentially using a parameter like return_http_code=True, which will be passed to RemoteDocument then RemoteServer._put()? E.g.

>>> await Document(db, 'id', {'a': 1}).save()  # or .save(return_http_code=True)
201
>>> await Document(db, 'id', {'a': 1}).save()  # or .save(return_http_code=True)
'HTTP 201 Created'
>>> await Document(db, 'id', {'a': 1}).save()  # or .save(return_http_code=True)
{'http_code': 201}

Note: this could also apply to the PUT /db route, which works similarly.

@bmario bmario added the bug Something isn't working label Oct 5, 2022
@bmario
Copy link
Member

bmario commented Oct 5, 2022

Darn it; this should apply to way more than these two. There's also the DELETE /db(/doc), COPY /db/doc, and POST /db. Though, I've never encountered those 202 codes yet. I guess they only pop up with a cluster? I should include a cluster setup in the tests, then.

The real question is, does this 202 code should affect the state of the Document/Datebase instance? If I understand this correctly, it should. The result doesn't contain a _rev. So I assume it would affect other endpoints as well.

@adrienverge
Copy link
Contributor Author

Darn it; this should apply to way more than these two. There's also the DELETE /db(/doc), COPY /db/doc, and POST /db.

True!

Though, I've never encountered those 202 codes yet. I guess they only pop up with a cluster?

Yes, I think they can only happen when a quorum fails, which means with a cluster. We see them from time to time, which is problematic because sometimes 2 concurrent calls on 2 CouchDB nodes end up on HTTP 202 Accepted, so both clients think their document version is saved for good, whereas there will be a conflict.

The real question is, does this 202 code should affect the state of the Document/Datebase instance? If I understand this correctly, it should. The result doesn't contain a _rev. So I assume it would affect other endpoints as well.

I'm not sure this return code affects the Document, because the document isn't changed. It could affect the Database, in a way, because it could have multiple conflicting versions of the same document. But personally I see it as an information independent from the state of docs and dbs.
I believe 201 vs. 202 return codes are independent from batch mode writes: they can happen even on regular PUT /db/doc calls, where the saved document has a _rev.

@bravier
Copy link
Contributor

bravier commented Dec 1, 2022

Hello @bmario, did you had any chance to look into it?
Would be nice for aiocouch to support this.

@bravier
Copy link
Contributor

bravier commented Dec 8, 2022

Can we help you getting this into aiocouch @bmario?
Do you think it's a good idea? If yes we can propose a PR maybe?

@bmario
Copy link
Member

bmario commented Dec 8, 2022

My problem right now is that I can't judge the impact of the 202 responses on the control flow. I still haven't fully grasped, what the response would look like if the status code is 202. So, I'd love to see a few examples, of what it looks like when directly using the CouchDB API. It would really help to have these.

@bravier
Copy link
Contributor

bravier commented Dec 9, 2022

CouchDB responses are exactly the same whether it responds with 201 or 202.
Only the status code changes to indicate if quorum was met (201) or not (202).

@bravier
Copy link
Contributor

bravier commented Jan 5, 2023

Happy new year @bmario!

In your last comment you said it would be helpful for you to play with a CouchDB cluster and see how it behaves.

I've setup a small 3 nodes cluster on my local machine so I can help you with the tests. Feel free to ask me to try things.
Otherwise maybe there's a chance you can setup such an environment on your side? That would be easier.

In my setup, to simulate a quorum not met, I'm freezing 2 out of 3 cluster nodes using pkill -STOP signal then I issue a PUT /<database>/<document> request on the node that is not frozen.
This PUT responds with a HTTP 202 status code instead of the regular HTTP 201 but the response content is the same as said in my previous comment.

It would be helpful for us to track these 202 when they happen so we can adapt our code behavior.

Thanks!

@H--o-l
Copy link
Contributor

H--o-l commented Jan 27, 2023

Hello @bmario!

I too would be interested in accessing more info from the CouchDB HTTP API, especially the status code which is part of the CouchDB API (and why not also headers, or the raw HTTP answer).

You can see for example that the official CouchDB nano library gives access to the error, header, and body of CouchDB answer: https://www.npmjs.com/package/nano#getting-started

If you don't have the time to do it yourself I'd be motivated for working on a PR, but before doing so I need to know if you will be willing to look at it once it's opened and willing to integrate it if it's well written.

What do you think?

@bmario
Copy link
Member

bmario commented Feb 28, 2023

I'm certainly willing to merge a change like that. However, I don't see how there is a way to keep the current interface. One obvious place would be to return the response headers from Document.save(). First, you'd have to extend the underlying calls to return the headers as well, for example, this one. HTTP error code will be propagated as exceptions. Where else would you need the status code?

@adrienverge
Copy link
Contributor Author

Where else would you need the status code?

At the Document level, only the PUT /{db}/{docid}, COPY /{db}/{docid} and DELETE /{db}/{docid} routes can return 2 types of successful HTTP code (201 and 202 (200 and 202 for DELETE)). HEAD and GET have only one valid code (200), according to the CouchDB documentation.
(For other instances (server, database, bulk) I did not check all the docs, but maybe let's focus on the Document interface only?)

I don't see how there is a way to keep the current interface

Right, Document.save() would have to return something. If needed, this return value could be present only if .save(return_http_code=True) is passed. About this return value: for extendibility, I would suggest returning {'http_code': 201} rather than just 201.

In my case I only need the HTTP code, but other users could want to read the HTTP headers or other things. In this case, other interfaces could be more future-proof:

>>> await Document(db, 'id', {'a': 1}).save()
{'http_code': 201}
>>> await Document(db, 'id', {'a': 1}).save(return_http_code=True)
{'http_code': 201}
>>> await Document(db, 'id', {'a': 1}).save(return_http_response=True)
HTTPResponse(status=201)

bmario added a commit that referenced this issue Aug 7, 2023
These methods are `Document.save()`, `Document.delete()`,
and `Document.copy()`.

Breaking change: `Document.copy()` does not return a `Document` anymore.
You can use the ETag and fetch the document yourself.

Fixes #49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants