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

CBG-4369 optionally return CV on rest API #7203

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Nov 20, 2024

CBG-4369 optionally return CV on rest API

Implements getting cv out of the requests used in https://github.com/couchbaselabs/couchbase-lite-tests/blob/main/client/src/cbltest/api/syncgateway.py

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

rest/bulk_api.go Outdated
@@ -276,9 +285,10 @@ func (h *handler) handleDump() error {
func (h *handler) handleRepair() error {
// TODO: If repair is re-enabled, it may need to be modified to support xattrs and GSI
err := errors.New("_repair endpoint disabled")
if err != nil {
/*if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a change in auditing behaviour (to not audit when a no-op call to repair is made). Does it make sense in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

golangci-lint ci didn't pass without this change, I am not immediately sure why because it's not changed code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it did pass with this removed, I fixed this issue on main but anemone hasn't been rebased recently.

@@ -364,6 +374,7 @@ func (h *handler) handleBulkGet() error {

includeAttachments := h.getBoolQuery("attachments")
showExp := h.getBoolQuery("show_exp")
showCV := h.getBoolQuery("show_cv")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be consistent with the query parameter used to include cv in the responses. I'm fine with cvs=true like you've got in the all docs case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did show_cv somewhat arbitrarily becuase I thought it made it clearer that it is an output parameter, especially when rev or revs can be input parameters to some functions.

Also cvs reminds me of a US pharmacy or the legacy version control system. I am happy to switch to cvs though.

@@ -30,6 +30,7 @@ func (h *handler) handleGetDoc() error {
revid := h.getQuery("rev")
openRevs := h.getQuery("open_revs")
showExp := h.getBoolQuery("show_exp")
showCV := h.getBoolQuery("show_cv")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, let's pick a single query param name and use it throughout.

@@ -173,3 +176,70 @@ func TestGuestReadOnly(t *testing.T) {
RequireStatus(t, response, http.StatusForbidden)

}

func TestGetDocWithCV(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test case for bulk_get? I may just have missed it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added missing test.

@adamcfraser adamcfraser merged commit 8b8b03d into release/anemone Nov 21, 2024
36 checks passed
@adamcfraser adamcfraser deleted the CBG-4369 branch November 21, 2024 17:28
adamcfraser pushed a commit that referenced this pull request Nov 29, 2024
* CBG-4369 optionally return CV on rest API

* pass lint

* Switch to show_cv parameter and add bulk_get test cases
bbrks pushed a commit that referenced this pull request Dec 2, 2024
* CBG-4369 optionally return CV on rest API

* pass lint

* Switch to show_cv parameter and add bulk_get test cases
bbrks pushed a commit that referenced this pull request Dec 5, 2024
* CBG-4369 optionally return CV on rest API

* pass lint

* Switch to show_cv parameter and add bulk_get test cases
bbrks pushed a commit that referenced this pull request Dec 17, 2024
* CBG-4369 optionally return CV on rest API

* pass lint

* Switch to show_cv parameter and add bulk_get test cases
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.

2 participants