Skip to content

Conversation

@RIT3shSapata
Copy link
Contributor

@RIT3shSapata RIT3shSapata commented Dec 24, 2025

CBG-4945

Describe your PR here...

  • Upstream PR- CBG-4412
  • This PR fixes the behaviour of sync function dry run when the default sync function is used

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

Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

This only works for the default collection

db/crud.go Outdated
}
} else {
if syncFn == "" {
syncFn = channels.DocChannelsSyncFunction
Copy link
Member

Choose a reason for hiding this comment

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

This is only the default sync function for the default collection. Named collections have a slightly different default sync function.

I'd probably use the GetDefaultSyncFunction function instead and pull out the scope/collection name from db *DatabaseCollectionWithUser

For reference: The code inside getChannelsAndAccess is doing the job of "faking" a sync function when ChannelMapper is nil, by looking at specific properties in the doc body (around lines 3331-3344)

https://github.com/couchbase/sync_gateway/blob/main/db/crud.go#L3332-L3344

@RIT3shSapata RIT3shSapata changed the base branch from CBG-4412 to main December 24, 2025 11:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the behavior of the sync function dry run feature when using the default sync function in Sync Gateway. Previously, when no custom sync function was provided, the dry run would fail to properly apply the default sync function for the collection.

Key changes:

  • Modified the conditional logic in SyncFnDryrun to properly handle cases where no custom sync function is defined
  • Added test cases to verify the default sync function behavior in dry run scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
db/crud.go Updated SyncFnDryrun method to correctly apply the default sync function when no custom sync function is provided
rest/diagnostic_doc_api_test.go Added two new test cases to verify dry run behavior with the default sync function for both new and existing documents

Comment on lines 1217 to 1220
// Since the tests run in named Scopes and Collections therefore the
// default sync function is:
// function(doc){channel("<collection_name>");}
// therefore the channels returned will be named collections
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The phrase 'therefore the' appears twice in succession. The comment should be restructured for better readability. Consider: 'Since the tests run in named Scopes and Collections, the default sync function is: function(doc){channel("<collection_name>");}. Therefore, the channels returned will be named collections.'

Suggested change
// Since the tests run in named Scopes and Collections therefore the
// default sync function is:
// function(doc){channel("<collection_name>");}
// therefore the channels returned will be named collections
// Since the tests run in named Scopes and Collections, the default sync function is:
// function(doc){channel("<collection_name>");}. Therefore, the channels returned will be named collections.

Copilot uses AI. Check for mistakes.
var output *channels.ChannelMapperOutput
var syncErr error
if syncFn == "" {
if syncFn == "" && db.ChannelMapper != nil {
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The logic flow has become less clear with the nested conditions. The outer condition checks syncFn == \"\" twice in different branches. Consider restructuring to: first check if syncFn is empty and set it to the default if needed, then proceed with a single conditional based on whether to use ChannelMapper or SyncRunner.

Copilot uses AI. Check for mistakes.
bbrks
bbrks previously approved these changes Dec 24, 2025
@bbrks bbrks enabled auto-merge (squash) December 24, 2025 11:43
@bbrks bbrks merged commit 918df77 into main Dec 24, 2025
41 of 42 checks passed
@bbrks bbrks deleted the CBG-4945 branch December 24, 2025 12:41
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.

3 participants