metricq / aiocouch

πŸ›‹ An asynchronous client library for CouchDB 2.x and 3.x
https://aiocouch.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
29 stars 10 forks source link

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

Closed adrienverge closed 1 year ago

adrienverge commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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

bravier commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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)