hide shard-sync and purge documents from _local_docs

[ckruse]

It hides the shard-sync and purge documents from the response unless a ?include_system=true is given.

Overview

Currently the system docs are included in _local_docs. This has shown to confuse users, and they try to delete them, which might fail and/or lead to problems.

Testing recommendations

curl -X PUT http:https://admin:[email protected]:15984/test-db
curl -X PUT http:https://admin:[email protected]:15984/test-db/doc -d '{"a":1}'
curl -X POST http:https://admin:[email protected]:15984/test-db/_purge -d '{"doc":["1-23202479633c2b380f79507a776743d5"]}' -Hcontent-type:application/json

sleep 5

# this shouldn't include any system documents
curl http:https://admin:[email protected]:15984/test-db/_local_docs

# this should include the system documents
curl http:https://admin:[email protected]:15984/test-db/_local_docs?include_system=true

Related Issues or Pull Requests

• Hide internal checkpoint docs from $db/_local_docs response #3930

Checklist

I think most of this is not applicable. Regarding tests, I'm not sure if there should be added tests for this. It would mean a sleep in a test, wouldn't it?

• Code is written and works correctly
• Changes are covered by tests
• Any new configurable parameters are documented in rel/overlay/etc/default.ini
• Documentation changes were made in the src/docs folder
• Documentation changes were backported (separated PR) to affected branches

[nickva]

Thanks for your contribution @ckruse! That looks much simpler and more elegant than expected.

One worry I had was that there is a tiny chance some users might actually have _local/purge-something... docs. But it seems we can filter those out by inspecting the doc bodies, I suggested a way to do it, what do you think about it?

It would be nice to also cover these changes by a test. Testing is tricky because we don't really have multi-node tests. One approach we could take is to just start with a single node test and insert a few local docs that look like those system docs and see how they are filtered out.

We could copy-paste and modify an existing chttpd test like: https://github.com/apache/couchdb/blob/main/src/chttpd/test/eunit/chttpd_revs_diff_tests.erl

Then insert a few docs like :

 {
   "_id": "_local/shard-sync-sUYbrDRqi_Mfo8dD6-OGow-At83GdaY-DBmWWzFL89R1g",
   "_rev": "0-1",
   "seq": 1,
   "target_uuid": "4f3eb3181db904d62176fb81626f84d0"
    }

{
 "_id": "_local/purge-mrview-0e22fbf4544d9ee72b82a82161d4bbc6",
 "_rev": "0-1",
  "ddoc_id": "_design/ddoc1",
   "purge_seq": 0,
   "signature": [1,2,3], 
   "type": "mrview",
   "updated_on": 1676995502
 }

And some regular and non-system local docs and see how include_system= behaves.

[ckruse]

In my tests there is no <<"signature">> field in purge docs. But besides that, I like your approach! 👍 what do you think of this variant?

All other fields don't seem to be particularly suitable to me:

[{<<"type">>,<<"internal_replication">>},{<<"updated_on">>,1677008682},{<<"purge_seq">>,12},{<<"source">>,<<"[email protected]">>},{<<"target">>,<<"[email protected]">>},{<<"range">>,undefined}]

Regarding tests: will add them tomorrow according to your suggestions. 👍 Thank you very much!

[nickva]

In my tests there is no <<"signature">> field in purge docs. But besides that, I like your approach! 👍 what do you think of this variant?

Looks great, I hadn't considered internal replication purge checkpoints. "purge_seq" should be enough to avoid accidents, I think.

Also you named the function better than I did. Your version is more descriptive.

[rnewson]

I understand the intention but I worry about how incomplete it is. Those local docs can still be fetched, updated, deleted, if you know the ids. Just scrubbing them from the /dbname/_local_docs response is a partial fix.

The PR is careful to do its best to confirm that the docs are of the type we want to skip, by looking into the doc properties. This reminded me that actually nothing stops a user with sufficient privilege from writing a doc of these two forms (not that it would be wise to do so).

It's regrettable we used local docs for truly internal things that users shouldn't see, the fix might require us to address that properly (another btree for 'sysdocs', say) or to accept the messy status quo.

Needs more thought, but it's a good start and thank you for not just raising the issue but having a good stab at fixing it.

[ckruse]

Currently I am still working on the tests (I don‘t yet understand why my documents don‘t get created), so I am afraid that creating another btree is beyond my abilities. Sorry.

But if you are still interested (I am not sure based on this conversation) I will finish the tests and fix the change requests to make the PR mergable.

[nickva]

Those local docs can still be fetched, updated, deleted, if you know the ids. Just scrubbing them from the /dbname/_local_docs response is a partial fix.

Not really, it's a random chance if reading and writting those would work. They are below the cluster level so doc id hashing isn't guaranteed to land it on a right shard

% http put $DB/db'?q=8'
% http post $DB/db/_bulk_docs docs:='[{}, {}, {}]'
% http $DB/db/_local_docs
      ....
       {
            "id": "_local/shard-sync-lX9QKOM429Elg1zgQVjY2A-k77wY4rMfBIYkHRoBveQXA",
            "key": "_local/shard-sync-lX9QKOM429Elg1zgQVjY2A-k77wY4rMfBIYkHRoBveQXA",
            "value": {
                "rev": "0-1"
            }
        }
     ...
}


% http $DB/db/_local/shard-sync-lX9QKOM429Elg1zgQVjY2A-k77wY4rMfBIYkHRoBveQXA
{
    "error": "not_found",
    "reason": "missing"
}

This reminded me that actually nothing stops a user with sufficient privilege from writing a doc of these two forms (not that it would be wise to do so).

That's plausible a user who can write to the db can randomly destroy or those checkpoint and mess up mem3 replicator or view purges. I think we're just trying to hide the noise from users not necessarily prevent them from manipulating these documents.

[rnewson]

@ckruse Please continue. I agree with Nick that it's worthwhile to suppress these items (by default) from casual viewing in the /$db/_local_docs response even if its only a partial solution to the internal use of local documents versus their intended purpose.

The suggestion about a new btree was for the wider team, I agree it would be quite a lot to expect someone new to our codebase to deliver. Should we do that, and change the internal uses to write there instead of into the local_btree, we could then come back and revert this work easily enough (as the items to be suppressed would no longer exist).

This change improves the current situation, I have no objections.

[nickva]

@rnewson good point, these checkpoints and internal docs would ideally live in a separate btree

@ckruse don't hesitate to reach out for help with tests, they can be a bit daunting to figure out. Feel free to drop by our Slack (https://couchdb.apache.org/#slack) #dev channel. My username there vatamane.

[ckruse]

Ok, I guess this PR is ready for review…?

[nickva]

@ckruse almost there! Thanks for writting the test. Let's move it to a separate test module and then I think it will be good to go. Make a new module chttpd_local_docs_tests.erl and move just your suite and needed helper functions to it.

[ckruse]

Ok, makes sense. Done :-)

[nickva]

+1

Thank you for your contribution @ckruse

[ckruse]

🥳 thanks for merging! And thanks for your support!