CBG-3721: [Diagnostic API] Import filter and sync function dry run endpoints #6715

mohammed-madi · 2024-03-05T15:21:26Z

CBG-3721

Describe your PR here...

Added 2 endpoints to the diagnostic API, sync function and import filter dry runs
Added API docs
Added TestGetDocDryRuns

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

Dependencies (if applicable)

Link upstream PRs
Update Go module dependencies when merged

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2348/

github-actions · 2024-03-05T15:21:41Z

Redocly previews

rest/routing.go

db/crud.go

rest/diagnostic_doc_api.go

db/crud.go

rest/routing.go

rest/diagnostic_doc_api.go

db/crud.go

docs/api/paths/diagnostic/keyspace-sync.yaml

adamcfraser · 2024-03-20T22:20:43Z

rest/routing.go

@@ -373,6 +373,9 @@ func createDiagnosticRouter(sc *ServerContext) *mux.Router {
 	keyspace := r.PathPrefix("/{keyspace:" + dbRegex + "}/").Subrouter()
 	keyspace.StrictSlash(true)
 	keyspace.Handle("/{docid:"+docRegex+"}/_all_channels", makeHandler(sc, adminPrivs, []Permission{PermReadAppData}, nil, (*handler).handleGetDocChannels)).Methods("GET")
+	keyspace.Handle("/_sync", makeHandler(sc, adminPrivs, []Permission{PermReadAppData}, nil, (*handler).handleSyncFnDryRun)).Methods("GET")
+	keyspace.Handle("/_import_filter", makeHandler(sc, adminPrivs, []Permission{PermReadAppData}, nil, (*handler).handleImportFilterDryRun)).Methods("GET")


I went back to the spec (after looking at your changes from the last review), and it looks like this endpoint is supposed to support either a docID on the path, or providing the body directly. I think that still makes sense - did you decide to change that intentionally?

The same applies to _sync - the spec looks like we want to support either docID or a provided body. In this case it might be the spec that needs to be updated, but what do you think the ideal behaviour is for this one?

I was going to update the spec once this is merged but if the body is provided I dont think theres a need for a doc id, and the docid can be an optional query for sync functions that use oldDoc

db/crud.go

rest/diagnostic_doc_api.go

adamcfraser · 2024-03-20T22:37:17Z

rest/diagnostic_doc_api.go

+	docid := h.getQuery("docid")
+	revID := h.getQuery("rev")
+
+	body, err := h.readDocument()


Is it required to specify a body for this call? If yes, should we return error if it's empty? If no, does line 64 succeed if not present?

if the body is empty and no doc id is not provided it will error, if the body is not provided but a doc id is provided, the doc in the bucket will be used as the body ( will return an error if the doc isnt there).
Ive added test cases for both scenarios

rest/diagnostic_doc_api_test.go

rest/diagnostic_doc_api.go

rest/diagnostic_doc_api_test.go

rest/diagnostic_doc_api.go

docs/api/components/parameters.yaml

db/crud.go

adamcfraser · 2024-03-21T22:32:59Z

db/crud.go

+		ID:    docID,
+		_body: body,
+	}
+	oldDoc := doc


This looks like if a body is passed in and docID is not found, we're using the body as both doc and oldDoc. I don't think this is what users will want to do - if they are only passing body, I think it should behave as an insert (oldDoc=null)

Its treated as an insert if there is no docID, but olddoc is used in prepareSyncFn. If doc id is not found it returns 404

If this is an insert, we need to ensure we're not populating oldDoc body in the sync function (people trying to test sync functions that have different handling for insert/update aren't going to work). If oldDoc needs to be non-nil, I expect you at least need to set oldDoc._body=nil? Or is this handled in another way that I'm overlooking?

adamcfraser · 2024-03-21T22:36:17Z

db/crud.go

+		return nil, nil, nil, nil, base.HTTPErrorf(http.StatusBadRequest, "Invalid revision ID")
+	}
+	generation++
+	oldDoc.RevID = body.ExtractRev()


What body is this extracting the rev from? It looks like it's the body that was passed in - I expected it to be docInBucket.SyncData.CurrentRev. Do we have unit tests that validate that rev is being properly populated into doc and oldDoc when doing a dry run of the sync function?

If no doc id is given, the rev id is not needed anywhere. It's only used for getting the parent rev if a doc id is given. I verified it with debug logging and I would probably have to return the rev id in the dry run response if we wanted to write unit tests to check it

My concern was whether oldDoc._rev is being set for use by the sync function. Looking at TestSyncFnOldDocBodyProperties it seems like we don't populate it in any case.

It's still be incorrect to set oldDoc.RevID to the value specified in body though (which is doc.rev, not oldDoc.rev). Does something break if you omit setting this property, if you don't think it's used?

db/crud.go

rest/diagnostic_doc_api.go

adamcfraser · 2024-04-04T23:02:32Z

db/crud.go

+		ID:    docID,
+		_body: body,
+	}
+	oldDoc := doc


If this is an insert, we need to ensure we're not populating oldDoc body in the sync function (people trying to test sync functions that have different handling for insert/update aren't going to work). If oldDoc needs to be non-nil, I expect you at least need to set oldDoc._body=nil? Or is this handled in another way that I'm overlooking?

adamcfraser · 2024-04-04T23:10:45Z

db/crud.go

+		return nil, nil, nil, nil, base.HTTPErrorf(http.StatusBadRequest, "Invalid revision ID")
+	}
+	generation++
+	oldDoc.RevID = body.ExtractRev()


My concern was whether oldDoc._rev is being set for use by the sync function. Looking at TestSyncFnOldDocBodyProperties it seems like we don't populate it in any case.

It's still be incorrect to set oldDoc.RevID to the value specified in body though (which is doc.rev, not oldDoc.rev). Does something break if you omit setting this property, if you don't think it's used?

db/crud.go

…ng stats on runsyncfun

adamcfraser

Everything looks good, except for one question about whether setting both doc and oldDoc is intentional when only docID is specified.

adamcfraser · 2024-04-08T23:05:24Z

rest/diagnostic_doc_api_test.go

+	assert.Equal(t, respMap.Access, channels.AccessMap{"user1": channels.BaseSetOf(t, "channel2")})
+
+	// get doc from bucket with no body provided
+	response = rt.SendDiagnosticRequest("GET", fmt.Sprintf("/{{.keyspace}}/_sync?doc_id=doc"), ``)


This case is using the document from the bucket as both doc and oldDoc in the sync function. Is this intentional? Using it as doc only seems like a more natural use case to me, but I'm open to other points of view.

I remember we discussed allowing this in sync up, either we use it as doc only or we attempt to get the previous rev as oldDoc?

Ive made the change to use it as doc only, let me know if we need to pull the old rev as old doc

…old doc

adamcfraser reviewed Mar 6, 2024

View reviewed changes

mohammed-madi assigned adamcfraser Mar 12, 2024

adamcfraser requested changes Mar 20, 2024

View reviewed changes

adamcfraser assigned mohammed-madi and unassigned adamcfraser Mar 20, 2024

mohammed-madi assigned adamcfraser and unassigned mohammed-madi Mar 21, 2024

adamcfraser requested changes Mar 21, 2024

View reviewed changes

adamcfraser assigned mohammed-madi and unassigned adamcfraser Mar 25, 2024

mohammed-madi assigned adamcfraser and unassigned mohammed-madi Apr 4, 2024

adamcfraser requested changes Apr 4, 2024

View reviewed changes

adamcfraser assigned mohammed-madi and unassigned adamcfraser Apr 4, 2024

bbrks force-pushed the main branch from a9811cd to 1879730 Compare April 8, 2024 11:02

mohammed-madi force-pushed the CBG-3721 branch from 82d97f6 to 6e29be5 Compare April 8, 2024 12:55

mohammed-madi added 12 commits April 8, 2024 16:17

Add import filter handler and half done sync fn handler

6e87ae1

Implement sync fn dry run

d8be4ff

Add test and fix handlers

6e201bc

Fix comments

11aaa96

Add docs

a819e8a

Fix lint

4db8eaa

Fix docs

18b7f21

Fix lint

a7fdd69

Fix comment

988b291

Fix test on CBS

44290a7

Address most comments, need olddoc compatibility and avoid incrementi…

e6a0000

…ng stats on runsyncfun

Fix oldDoc handling and stop incrementing stats on dry run

f641fa9

mohammed-madi added 13 commits April 8, 2024 16:17

Fix docs

dc67e79

Fix comment

03a9f95

Add missing doc page and fix lint

f16c5a2

Fix tests

3717116

Address comments

06b66c4

Add doc id query for import filter dry run

32b6350

Add doc id to import filter dry run docs

5f61d46

Fix lint

ad5905a

Skip removing rev id from history

57ed5d0

Remove dryrun bool and refactor SyncFnDryRun

b5240ec

Add dryrun to evaluate function for import filter

f769591

Fix crud_test.go

3a5f9e1

Fix test

9826b18

mohammed-madi force-pushed the CBG-3721 branch from 6e29be5 to 9826b18 Compare April 8, 2024 13:19

mohammed-madi added 2 commits April 8, 2024 18:31

Remove unnecessary code

75338bd

Add coverage for different old/new handling

0fa6ffb

adamcfraser assigned adamcfraser and unassigned mohammed-madi Apr 8, 2024

adamcfraser requested changes Apr 8, 2024

View reviewed changes

adamcfraser assigned mohammed-madi and unassigned adamcfraser Apr 8, 2024

When doc is given but no body is given, doc in bucket is not used as …

8117701

…old doc

mohammed-madi assigned adamcfraser and unassigned mohammed-madi Apr 9, 2024

adamcfraser approved these changes Apr 9, 2024

View reviewed changes

adamcfraser merged commit dd9430c into main Apr 9, 2024
33 checks passed

adamcfraser deleted the CBG-3721 branch April 9, 2024 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-3721: [Diagnostic API] Import filter and sync function dry run endpoints #6715

CBG-3721: [Diagnostic API] Import filter and sync function dry run endpoints #6715

mohammed-madi commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024 •

edited

Loading

adamcfraser Mar 20, 2024

adamcfraser Mar 20, 2024

mohammed-madi Mar 21, 2024 •

edited

Loading

adamcfraser Mar 20, 2024

mohammed-madi Mar 21, 2024

adamcfraser Mar 21, 2024

mohammed-madi Apr 2, 2024

adamcfraser Apr 4, 2024

adamcfraser Mar 21, 2024

mohammed-madi Apr 2, 2024

adamcfraser Apr 4, 2024

adamcfraser Apr 4, 2024

adamcfraser Apr 4, 2024

adamcfraser left a comment

adamcfraser Apr 8, 2024

mohammed-madi Apr 9, 2024

mohammed-madi Apr 9, 2024

CBG-3721: [Diagnostic API] Import filter and sync function dry run endpoints #6715

CBG-3721: [Diagnostic API] Import filter and sync function dry run endpoints #6715

Conversation

mohammed-madi commented Mar 5, 2024 • edited Loading

Pre-review checklist

Dependencies (if applicable)

github-actions bot commented Mar 5, 2024 • edited Loading

Redocly previews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammed-madi Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammed-madi commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024 •

edited

Loading

mohammed-madi Mar 21, 2024 •

edited

Loading