You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2022/08/19 22:13:44 UTC

[GitHub] [couchdb] zachlankton opened a new pull request, #4152: Fix Partition All Docs Query With Keys

zachlankton opened a new pull request, #4152:
URL: https://github.com/apache/couchdb/pull/4152

   ## 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`.
   
   ```json
   "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:
   
   ```bash
   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
   
   ```json
   "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
   
   - [x] Code is written and works correctly
   - [x] 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r953035578


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,24 @@ all_docs_concurrency() ->
             10
     end.
 
+get_query_list(undefined) ->
+    [];
+get_query_list(List) when is_list(List) ->
+    List.
+validate_if_partition(Row, Acc) ->
+    QueryList = get_query_list(Acc#vacc.req#httpd.qs),
+    case couch_util:get_value("partition", QueryList) of

Review Comment:
   I just discovered `chttpd:qs_value/2` ... will be refactoring this code.



##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,24 @@ all_docs_concurrency() ->
             10
     end.
 
+get_query_list(undefined) ->
+    [];
+get_query_list(List) when is_list(List) ->
+    List.
+validate_if_partition(Row, Acc) ->
+    QueryList = get_query_list(Acc#vacc.req#httpd.qs),
+    case couch_util:get_value("partition", QueryList) of
+        undefined -> Row;
+        Id -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowKey = Row#view_row.key,
+    IsDocInPartition = binary:match(BinRowKey, list_to_binary(Id)) /= nomatch,

Review Comment:
   How do we feel about the way I am matching the `BinRowKey` with the requested partition `Id` ?
   
   This works, but wondering if it could be problematic in the future as a partition key of `test` could also match the following row key: `someOtherPartition:with-test-in-the-doc-id`
   
   What would be the recommended way to extract the partition from the `BinRowKey` and do an explicit match?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r952673935


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),

Review Comment:
   I realize now that I wasn't totally clear with my variable name as I am trying to get the key, not the id.  So I will update as follows:
   
   ```erlang
   BinRowKey = Row#view_row.key,
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] rnewson commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
rnewson commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r950700712


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),
+    IsDocInPartition = binary:match(BinRowId, list_to_binary(Id)) /= nomatch,
+    case IsDocInPartition of
+        true -> Row;
+        false -> {view_row, BinRowId, undefined, undefined, undefined, undefined}

Review Comment:
   and also it should be created as `#view_row{id = BinRowId}`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r952718093


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)

Review Comment:
   You are correct, I am newish to the pattern matching in erlang and here it really shows.  I can see how this will fail if there are any other query parameters.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] janl commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
janl commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r950668450


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)

Review Comment:
   this only matches if the `qs` is either `undefined` or exactly the list `[{"partition", Id}]` — is this always the case? Can there be other options in the qs? If yes, we’d probably need to get the partition tuple out with a `proplists:get()` or `couch_util:get_value()`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r952680366


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),
+    IsDocInPartition = binary:match(BinRowId, list_to_binary(Id)) /= nomatch,
+    case IsDocInPartition of
+        true -> Row;
+        false -> {view_row, BinRowId, undefined, undefined, undefined, undefined}

Review Comment:
   The current implementation passes this "dummy" row back into the call back on the `_all_docs` endpoint, so I do believe this is the correct way to handle missing documents.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r953054995


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,24 @@ all_docs_concurrency() ->
             10
     end.
 
+get_query_list(undefined) ->
+    [];
+get_query_list(List) when is_list(List) ->
+    List.
+validate_if_partition(Row, Acc) ->
+    QueryList = get_query_list(Acc#vacc.req#httpd.qs),
+    case couch_util:get_value("partition", QueryList) of
+        undefined -> Row;
+        Id -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowKey = Row#view_row.key,
+    IsDocInPartition = binary:match(BinRowKey, list_to_binary(Id)) /= nomatch,

Review Comment:
   just discovered `couch_partition:is_member(BinRowKey, Partition)`  will be refactoring this as well



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] janl commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
janl commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r950864793


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),

Review Comment:
   thanks, that’s what I was looking for



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#issuecomment-1224135235

   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. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r952641604


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),

Review Comment:
   This is so much better, thank you!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] janl commented on pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
janl commented on PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#issuecomment-1221259044

   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!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] rnewson commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
rnewson commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r950700821


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),

Review Comment:
   Neither of those are good approaches. `Row#view_row.id` extracts the required field by name.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] zachlankton commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
zachlankton commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r952675113


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),
+    IsDocInPartition = binary:match(BinRowId, list_to_binary(Id)) /= nomatch,
+    case IsDocInPartition of
+        true -> Row;
+        false -> {view_row, BinRowId, undefined, undefined, undefined, undefined}

Review Comment:
   👍 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] janl commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
janl commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r950668579


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),
+    IsDocInPartition = binary:match(BinRowId, list_to_binary(Id)) /= nomatch,
+    case IsDocInPartition of
+        true -> Row;
+        false -> {view_row, BinRowId, undefined, undefined, undefined, undefined}

Review Comment:
   this looks like you create a dummy row for entries that should not be in the result that presumably gets filtered out later. You also pass this into `Callback()` in line 318 below. Wouldn’t it make more sense to just not call `Callback()` with that row?
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] janl commented on a diff in pull request #4152: Fix Partition All Docs Query With Keys

Posted by GitBox <gi...@apache.org>.
janl commented on code in PR #4152:
URL: https://github.com/apache/couchdb/pull/4152#discussion_r950668273


##########
src/fabric/src/fabric_view_all_docs.erl:
##########
@@ -286,6 +286,20 @@ all_docs_concurrency() ->
             10
     end.
 
+validate_if_partition(Row, Acc) ->
+    QueryStr = Acc#vacc.req#httpd.qs,
+    case QueryStr of
+        undefined -> Row;
+        [{"partition", Id}] -> is_doc_in_partition(Row, Id)
+    end.
+is_doc_in_partition(Row, Id) ->
+    BinRowId = element(2, Row),

Review Comment:
   the more idiomatic way of doing this in Erlang would be:
   
   ```
    {_, BinRowId, _, _, _, _} = Row,
   ```
   
   but I’m not 100% that that’s actually better. I think maybe on revising the code it is nicer to know the explicit row structure rather than having to know the id is in location 2.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org