CBG-3768 Update only metadata during import #6782
Conversation
During document import, perform an xattr-only update of the document.
adamcfraser
changed the title
There was a problem hiding this comment.
I'll wait for an update as to why TestXattrOnDemandImportPreservesExpiry is failing.
rest/importtest/import_test.go
Outdated
| base.SkipImportTestsIfNotEnabled(t) | ||
|
|
||
| rtConfig := rest.RestTesterConfig{ | ||
| SyncFn: `function(doc, oldDoc) { channel(doc.channels) }`, |
There was a problem hiding this comment.
I assume you are defining a syncfn because it will undergo js sync function and not skip? I'm not sure if that's needed for this test, I'd document why this is.
Autoimport should be enabled by default as far as I can tell by the RestTesterso you could use the default config.
There was a problem hiding this comment.
No, this was just copy/paste of the setup code from another test. I agree it can be simplified.
There was a problem hiding this comment.
It turns out autoimport is only enabled by default for EE, so still need the explicit setting here for the time being.
| mobileBody := make(map[string]interface{}) | ||
| mobileBody["channels"] = "ABC" | ||
| _, err := dataStore.Add(mobileKey, 0, mobileBody) | ||
| assert.NoError(t, err, "Error writing SDK doc") |
There was a problem hiding this comment.
| assert.NoError(t, err, "Error writing SDK doc") | |
| require.NoError(t, err, "Error writing SDK doc") |
| @@ -1899,7 +1899,8 @@ type updateAndReturnDocCallback func(*Document) (resultDoc *Document, resultAtta | |||
| // 1. Receive the updated document body in the response | |||
| // 2. Specify the existing document body/xattr/cas, to avoid initial retrieval of the doc in cases that the current contents are already known (e.g. import). | |||
| // On cas failure, the document will still be reloaded from the bucket as usual. | |||
| func (db *DatabaseCollectionWithUser) updateAndReturnDoc(ctx context.Context, docid string, allowImport bool, expiry *uint32, opts *sgbucket.MutateInOptions, existingDoc *sgbucket.BucketDocument, callback updateAndReturnDocCallback) (doc *Document, newRevID string, err error) { | |||
| // 3. If isImport=true, document body will not be updated - only metadata xattr(s) | |||
| func (db *DatabaseCollectionWithUser) updateAndReturnDoc(ctx context.Context, docid string, allowImport bool, expiry *uint32, opts *sgbucket.MutateInOptions, existingDoc *sgbucket.BucketDocument, isImport bool, callback updateAndReturnDocCallback) (doc *Document, newRevID string, err error) { | |||
There was a problem hiding this comment.
Take or leave this comment as maybe out of scope.
I noticed that allowImport and isImport are both args to this function and I dug deeper in the code to figure out why because these variables seem very similar and it was not clear why they would both need to be set and are different. As far as I can tell, allowImport could be removed because
Line 1809 in c4ba903
err = validateExistingDoc(doc, col.UseXattrs(), docExists)
and this would avoid the need to pass in allowImport at all.
There was a problem hiding this comment.
You're correct that today allowImport is equivalent to "is shared bucket access enabled", which could be checked by UseXattrs(). I don't think that's related to this PR, though, and is different than 'isImport', which mean 'is the current operation being performed an import'
There was a problem hiding this comment.
Understood, I was commenting on the fact that these have very similar argument names and it was a bit confusing to me.
It is different than this PR, only related in that I look at the function signature and saw two bool arguments with import in the name and wondered how they were different.
During document import, perform an xattr-only update of the document.
CBG-3768
Dependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2387/