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

Fix Partition All Docs Query With Keys #4152

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zachlankton
Copy link
Contributor

Overview

This PR addresses a bug that is described in issue #2496 where posting an all docs query to a partition endpoint with keys that are not a part of the partition also gets returned.

Testing recommendations

Consider the following documents, each in their own partition on a database named test.

"rows": [
        {
            "id": "part1:doc1",
            "key": "part1:doc1",
            "value": {
                "rev": "5-aa9d2b82de6db1e61fba8e4057299d21"
            }
        },
        {
            "id": "part2:doc1",
            "key": "part2:doc1",
            "value": {
                "rev": "3-a707bb62c86bd430389e4d596aeb6084"
            }
        }
]

When we run the following query against the part2 partition:

curl -X POST http://127.0.0.1:15984/test/_partition/part2/_all_docs \
    -H 'Content-Type: application/json' \
    -d '{"keys":["part1:doc1", "part2:doc1"]}'

We get both documents from both partitions, but what we expect should look like this

"rows": [
        {
            "key": "part1:doc1",
            "error": "not_found"
        },
        {
        "id": "part2:doc1",
            "key": "part2:doc1",
            "value": {
                "rev": "3-a707bb62c86bd430389e4d596aeb6084"
            }
       }
    ]

The code in this PR fixes this issue and returns the results we expect.

Related Issues or Pull Requests

Issue #2496

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • [n/a] Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • [n/a] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

@janl
Copy link
Member

janl commented Aug 20, 2022

Thanks for this, this looks decent overall, especially for a first contribution :)

I left a few minor notes on the code. I just have one question about the approach taken here.

The way I read it: passing keys to match into the query will return result rows we do not want. So this PR starts preventing those result rows from being returned.

Would another option to solve this be making it so that we don’t read those rows in the first place? That would make this a performance improvement too.

We might even be able to do this at the request validation stage: if keys includes a partition that is not the partition of the URL being queried, we return a 400 error with a message that all keys must match the partition provided in the URL.

Just spitballing here tho. Nice work!

@zachlankton
Copy link
Contributor Author

Thanks Jan,

I had considered something similar in the beginning and am able to refactor if this is truly the desired result.

However, this being a sort of bulk endpoint, I do believe it is consistent with the current behavior of the _all_docs endpoint on the root database (and on unpartitioned databases), where CouchDB returns a list of results that may include "error":"not found" results per key passed in.

src/fabric/src/fabric_view_all_docs.erl Outdated Show resolved Hide resolved
src/fabric/src/fabric_view_all_docs.erl Outdated Show resolved Hide resolved
@zachlankton zachlankton force-pushed the fix-partition-all-docs-with-keys branch 4 times, most recently from cbf2b11 to 0f9df2e Compare August 30, 2022 03:25
@zachlankton zachlankton force-pushed the fix-partition-all-docs-with-keys branch from 0f9df2e to 1cc0079 Compare September 2, 2022 02:32
@zachlankton zachlankton force-pushed the fix-partition-all-docs-with-keys branch from 1cc0079 to 195b56e Compare September 13, 2022 11:12
@zachlankton zachlankton force-pushed the fix-partition-all-docs-with-keys branch from 195b56e to eea086d Compare September 15, 2022 14:49
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.

None yet

3 participants