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

Limit query param works incorrectly for _all_dbs #3786

Closed
mojito317 opened this issue Oct 14, 2021 · 6 comments
Closed

Limit query param works incorrectly for _all_dbs #3786

mojito317 opened this issue Oct 14, 2021 · 6 comments
Labels
Milestone

Comments

@mojito317
Copy link

Description

all_dbs with limit query is not working well. The response always has an array with a limit-1 length.

Steps to Reproduce

$ curl localhost:5984/_all_dbs | jq .
[
  "_replicator",
  "_users"
]

$ curl localhost:5984/_all_dbs?limit=1 | jq .
[]

$ curl localhost:5984/_all_dbs?limit=2 | jq .
[
 "_replicator"
]

Expected Behaviour

Get back as many elements in the array as the limit tells.

Your Environment

  • CouchDB version used: 3.2.0
  • Browser name and version: curl
  • Operating system and version: macOS Big Sur 11.6

Additional Context

@jaydoane
Copy link
Contributor

@nickva bisected commit history and located the commit where this fault was introduced. Further analysis revealed that by changing the code to use the actual shards db, it caused custodian's design doc to be injected into that db:

http $DB/_node/_local/_dbs/_all_docs
HTTP/1.1 200 OK
Cache-Control: must-revalidate
Content-Type: application/json
Date: Wed, 13 Oct 2021 18:07:26 GMT
Server: CouchDB/3.1.1-8ac1978 (Erlang OTP/20)
Transfer-Encoding: chunked
X-Couch-Request-ID: be32a659e4
X-CouchDB-Body-Time: 0
{
    "offset": 0,
    "rows": [
        {
            "id": "_design/custodian",
            "key": "_design/custodian",
            "value": {
                "rev": "1-51e56733e48a1b629dbc15e2c2ed6d7d"
            }
        },
        {
            "id": "_replicator",
            "key": "_replicator",
            "value": {
                "rev": "1-d52224ea9c7c3bba18d02e9a0c2c4410"
            }
        },
        {
            "id": "_users",
            "key": "_users",
            "value": {
                "rev": "1-48f825a6cc14fd694c3db04794d69a5b"
            }
        }
    ],
    "total_rows": 3
}

Evidently _design/custodian is (correctly) filtered out of the _all_dbs result, but after the limit is applied, which essentially results in an off-by-one result now for _all_dbs with limit applied, e.g.

$ curl -s -u adm:pass localhost:15984/_all_dbs | jq .
[
  "_replicator",
  "_users"
]

$ curl -s -u adm:pass localhost:15984/_all_dbs?limit=1 | jq .
[]

$ curl -s -u adm:pass localhost:15984/_all_dbs?limit=2 | jq .
[
  "_replicator"
]

I haven't looked at the code yet, but perhaps the limit could be applied after the ddoc has been filtered out?

@nickva
Copy link
Contributor

nickva commented Oct 14, 2021

An alternative could be to move to not use a VDU and instead rely on a before_doc_update/3 validation function since this is a system DB.

We did a similar thing for _replicator auto-injected VDU in main branch with the idea of eventually porting to 3.x as well. Then, if we had that we can auto-remove the injected design doc.

Additionally, the VDU / validation probably shouldn't happen in custodian but rather in the mem3 module, so we should move the logic there if we plan on keeping it for the future.

@jaydoane
Copy link
Contributor

Using a before_doc_update/3 validation function and avoiding JS makes sense, especially for the shards db.

We did a similar thing for _replicator auto-injected VDU in main branch with the idea of eventually porting to 3.x as well

Is this to what you are referring?

Also agree that mem3 is a better location for the VDU, if it were to stay.

@nickva
Copy link
Contributor

nickva commented Oct 14, 2021

Yeah that's the before_doc_update/3 handler I was referring to. That callback works, I believe, on system databases in a fairly straightforward manner and if we translate the doc body to a map before passing it to the validator, we can do succinct pattern matching that might look more elegant than the JS version.

@nickva
Copy link
Contributor

nickva commented Oct 21, 2021

Tentative (draft) PR #3796

nickva added a commit that referenced this issue Oct 22, 2021
This fixes issue: #3786

In addition, add few _all_dbs limit tests since we didn't seem to have
any previously to catch such issues. Plus, test some of the corner
cases which should be caught by the BDU and should return a 403 error
code.
nickva added a commit that referenced this issue Oct 24, 2021
This fixes issue: #3786

In addition, add few _all_dbs limit tests since we didn't seem to have
any previously to catch such issues. Plus, test some of the corner
cases which should be caught by the BDU and should return a 403 error
code.
nickva added a commit that referenced this issue Oct 24, 2021
This fixes issue: #3786

In addition, add few _all_dbs limit tests since we didn't seem to have
any previously to catch such issues. Plus, test some of the corner
cases which should be caught by the BDU and should return a 403 error
code.
@janl janl added this to the 3.2.1 milestone Oct 25, 2021
@nickva
Copy link
Contributor

nickva commented Oct 26, 2021

This is fixed and should be ready for 3.2.1

@nickva nickva closed this as completed Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants