Merge pull request #122 from brave/master

Release 09/07/2023

.github/workflows/ci.yml

@@ -18,7 +18,7 @@ jobs:
 
       - uses: actions/setup-go@v3
         with:
-          go-version: 1.14
+          go-version: 1.18
 
       - name: lint
         uses: golangci/golangci-lint-action@v3

Makefile

@@ -6,8 +6,15 @@ BUILD_TIME := $(shell date +%s)
 
 all: lint test build
 
+repath-proto:
+	find ./schema/protobuf/sync_pb | grep "\.proto$\" | xargs sed -i 's/import \"components\/sync\/protocol\//import \"/g'
+	find ./schema/protobuf/sync_pb | grep "\.proto$\" | xargs sed -i 's/import \"brave\/components\/sync\/protocol\//import \"/g'
+
+proto-go-module:
+	find ./schema/protobuf/sync_pb | grep "\.proto$\" | xargs sed -i 's/syntax = \"proto2\";/syntax = \"proto2\";\n\noption go_package = \"\.\/sync_pb\";/'
+
 protobuf:
-	protoc -I schema/protobuf/sync_pb/ schema/protobuf/sync_pb/*.proto --go_out=schema/protobuf/sync_pb/
+	protoc -I schema/protobuf/sync_pb/ schema/protobuf/sync_pb/*.proto --go_out=schema/protobuf/
 
 build:
 	go run main.go
@@ -16,7 +23,7 @@ test:
 	go test -v ./...
 
 lint:
-	golangci-lint run -E gofmt -E golint --exclude-use-default=false
+	docker run -t --rm -v "$$(pwd):/app" -w /app golangci/golangci-lint golangci-lint run -v
 
 clean:
 	rm -f sync-server

README.md

@@ -2,7 +2,7 @@
 
 A sync server implemented in go to communicate with Brave sync clients using
 [components/sync/protocol/sync.proto](https://cs.chromium.org/chromium/src/components/sync/protocol/sync.proto).
-Current Chromium version for sync protocol buffer files used in this repo is Chromium 88.0.4324.96.
+Current Chromium version for sync protocol buffer files used in this repo is Chromium 116.0.5845.183.
 
 This server supports endpoints as bellow.
 - The `POST /v2/command/` endpoint handles Commit and GetUpdates requests from sync clients and return corresponding responses both in protobuf format. Detailed of requests and their corresponding responses are defined in `schema/protobuf/sync_pb/sync.proto`. Sync clients are responsible for generating valid access tokens and present them to the server in the Authorization header of requests.
@@ -23,8 +23,15 @@ Currently we use dynamoDB as the datastore, the schema could be found in `schema
 3. Run `make docker-up`
 4. For running unit tests, run `make docker-test`
 
+# Updating protocol definitions
+1. Copy the `.proto` files from `components/sync/protocol` in `chromium` to `schema/protobuf/sync_pb` in `go-sync`.
+2. Copy the `.proto` files from `components/sync/protocol` in `brave-core` to `schema/protobuf/sync_pb` in `go-sync`.
+3. Run `make repath-proto` to set correct import paths in `.proto` files.
+4. Run `make proto-go-module` to add the `go_module` option to `.proto` files.
+5. Run `make protobuf` to generate the Go code from `.proto` definitions.
+
 ## Prometheus Instrumentation
-The instrumented datastore and redis interfaces are generated, providing integration with Prometheus.  The following will re-generate the instrumented code:
+The instrumented datastore and redis interfaces are generated, providing integration with Prometheus.  The following will re-generate the instrumented code, required when updating protocl definitions:
 
 ```
 make instrumented

command/command.go

@@ -87,9 +87,6 @@ func handleGetUpdatesRequest(cache *cache.Cache, guMsg *sync_pb.GetUpdatesMessag
 	}
 
 	maxSize := maxGUBatchSize
-	if guMsg.BatchSize != nil && int(*guMsg.BatchSize) < maxGUBatchSize {
-		maxSize = int(*guMsg.BatchSize)
-	}
 
 	// Process from_progress_marker
 	guRsp.NewProgressMarker = make([]*sync_pb.DataTypeProgressMarker, len(guMsg.FromProgressMarker))
@@ -273,7 +270,7 @@ func handleCommitRequest(cache *cache.Cache, commitMsg *sync_pb.CommitMessage, c
 
 			count++
 		} else { // Update
-			conflict, delete, err := db.UpdateSyncEntity(entityToCommit, oldVersion)
+			conflict, deleted, err := db.UpdateSyncEntity(entityToCommit, oldVersion)
 			if err != nil {
 				log.Error().Err(err).Msg("Update sync entity failed")
 				rspType := sync_pb.CommitResponse_TRANSIENT_ERROR
@@ -286,7 +283,7 @@ func handleCommitRequest(cache *cache.Cache, commitMsg *sync_pb.CommitMessage, c
 				entryRsp.ResponseType = &rspType
 				continue
 			}
-			if delete {
+			if deleted {
 				count--
 			}
 		}
@@ -322,7 +319,7 @@ func handleCommitRequest(cache *cache.Cache, commitMsg *sync_pb.CommitMessage, c
 
 // handleClearServerDataRequest handles clearing user data from the datastore and cache
 // and fills the response
-func handleClearServerDataRequest(cache *cache.Cache, db datastore.Datastore, msg *sync_pb.ClearServerDataMessage, clientID string) (*sync_pb.SyncEnums_ErrorType, error) {
+func handleClearServerDataRequest(cache *cache.Cache, db datastore.Datastore, _ *sync_pb.ClearServerDataMessage, clientID string) (*sync_pb.SyncEnums_ErrorType, error) {
 	errCode := sync_pb.SyncEnums_SUCCESS
 	var err error
 

command/command_test.go

@@ -142,12 +142,11 @@ func getMarker(suite *CommandTestSuite, tokens []int64) []*sync_pb.DataTypeProgr
 
 func getClientToServerGUMsg(marker []*sync_pb.DataTypeProgressMarker,
 	origin sync_pb.SyncEnums_GetUpdatesOrigin, fetchFolders bool,
-	batchSize *int32) *sync_pb.ClientToServerMessage {
+	_ *int32) *sync_pb.ClientToServerMessage {
 	guMsg := &sync_pb.GetUpdatesMessage{
 		FetchFolders:       aws.Bool(fetchFolders),
 		FromProgressMarker: marker,
 		GetUpdatesOrigin:   &origin,
-		BatchSize:          batchSize,
 	}
 	contents := sync_pb.ClientToServerMessage_GET_UPDATES
 	return &sync_pb.ClientToServerMessage{
@@ -400,49 +399,6 @@ func (suite *CommandTestSuite) TestHandleClientToServerMessage_GUBatchSize() {
 		suite.Assert().Equal(commitSuccess, *entryRsp.ResponseType)
 		suite.Assert().Equal(*entryRsp.Mtime, *entryRsp.Version)
 	}
-
-	// Test maxGUBatchSize from client side should be respected when smaller than
-	// the server one.
-	clientBatchSize := 3
-	marker := getMarker(suite, []int64{0, 0})
-	msg = getClientToServerGUMsg(
-		marker, sync_pb.SyncEnums_GU_TRIGGER, false, aws.Int32(int32(clientBatchSize)))
-	rsp = &sync_pb.ClientToServerResponse{}
-	suite.Require().NoError(
-		command.HandleClientToServerMessage(suite.cache, msg, rsp, suite.dynamo, clientID),
-		"HandleClientToServerMessage should succeed")
-	assertCommonResponse(suite, rsp, false)
-	suite.Assert().Equal(clientBatchSize, len(rsp.GetUpdates.Entries))
-	suite.Assert().Equal(
-		int64(len(entries)-clientBatchSize), *rsp.GetUpdates.ChangesRemaining)
-	// nigori1, nigori2, bookmark1
-	expectedName := []*string{entries[1].Name, entries[3].Name, entries[0].Name}
-	for i, entry := range rsp.GetUpdates.Entries {
-		suite.Assert().Equal(expectedName[i], entry.Name)
-	}
-	expectedNewMarker := getMarker(suite,
-		[]int64{*rsp.GetUpdates.Entries[1].Mtime, *rsp.GetUpdates.Entries[2].Mtime})
-	suite.Assert().Equal(expectedNewMarker, rsp.GetUpdates.NewProgressMarker)
-
-	// Test maxGUBatchSize from server side should be respected when smaller than
-	// the client one.
-	defaultServerGUBatchSize := *command.MaxGUBatchSize
-	*command.MaxGUBatchSize = 2
-	rsp = &sync_pb.ClientToServerResponse{}
-	suite.Require().NoError(
-		command.HandleClientToServerMessage(suite.cache, msg, rsp, suite.dynamo, clientID),
-		"HandleClientToServerMessage should succeed")
-	assertCommonResponse(suite, rsp, false)
-	suite.Assert().Equal(int(*command.MaxGUBatchSize), len(rsp.GetUpdates.Entries))
-	suite.Assert().Equal(int64(1), *rsp.GetUpdates.ChangesRemaining)
-	// nigori1, nigori2
-	expectedName = []*string{entries[1].Name, entries[3].Name}
-	for i, entry := range rsp.GetUpdates.Entries {
-		suite.Assert().Equal(expectedName[i], entry.Name)
-	}
-	expectedNewMarker = getMarker(suite, []int64{*rsp.GetUpdates.Entries[1].Mtime, 0})
-	suite.Assert().Equal(expectedNewMarker, rsp.GetUpdates.NewProgressMarker)
-	*command.MaxGUBatchSize = defaultServerGUBatchSize
 }
 
 func (suite *CommandTestSuite) TestHandleClientToServerMessage_QuotaLimit() {
@@ -828,16 +784,16 @@ func (suite *CommandTestSuite) TestHandleClientToServerMessage_TypeMtimeCache_Ch
 	// Send a GU with batch size set to 1, changesRemaining in rsp should be 1
 	// and cache should not be updated.
 	marker := getMarker(suite, []int64{0, 0})
-	clientBatch := int32(1)
+	clientBatch := int32(2)
 	msg = getClientToServerGUMsg(
 		marker, sync_pb.SyncEnums_PERIODIC, true, &clientBatch)
 	rsp = &sync_pb.ClientToServerResponse{}
 	suite.Require().NoError(
 		command.HandleClientToServerMessage(suite.cache, msg, rsp, suite.dynamo, clientID),
 		"HandleClientToServerMessage should succeed")
 	assertCommonResponse(suite, rsp, false)
-	suite.Require().Equal(1, len(rsp.GetUpdates.Entries))
-	suite.Require().Equal(int64(1), *rsp.GetUpdates.ChangesRemaining)
+	suite.Require().Equal(2, len(rsp.GetUpdates.Entries))
+	suite.Require().Equal(int64(0), *rsp.GetUpdates.ChangesRemaining)
 	mtime := *rsp.GetUpdates.Entries[0].Mtime
 	assertTypeMtimeCacheValue(suite, clientID+"#"+strconv.Itoa(int(bookmarkType)),
 		latestBookmarkMtime,

datastore/sync_entity.go

@@ -584,15 +584,15 @@ func (dynamo *Dynamo) UpdateSyncEntity(entity *SyncEntity, oldVersion int64) (bo
 	if err != nil {
 		return false, false, fmt.Errorf("error unmarshalling old sync entity: %w", err)
 	}
-	var delete bool
+	var deleted bool
 	if entity.Deleted == nil { // No updates on Deleted this time.
-		delete = false
+		deleted = false
 	} else if oldEntity.Deleted == nil { // Consider it as Deleted = false.
-		delete = *entity.Deleted
+		deleted = *entity.Deleted
 	} else {
-		delete = !*oldEntity.Deleted && *entity.Deleted
+		deleted = !*oldEntity.Deleted && *entity.Deleted
 	}
-	return false, delete, nil
+	return false, deleted, nil
 }
 
 // GetUpdatesForType returns sync entities of a data type where it's mtime is
@@ -782,7 +782,7 @@ func CreateDBSyncEntity(entity *sync_pb.SyncEntity, cacheGUID *string, clientID
 		Deleted:                deleted,
 		OriginatorCacheGUID:    originatorCacheGUID,
 		OriginatorClientItemID: originatorClientItemID,
-		ClientDefinedUniqueTag: entity.ClientDefinedUniqueTag,
+		ClientDefinedUniqueTag: entity.ClientTagHash,
 		Specifics:              specifics,
 		Folder:                 folder,
 		UniquePosition:         uniquePosition,
@@ -802,7 +802,7 @@ func CreatePBSyncEntity(entity *SyncEntity) (*sync_pb.SyncEntity, error) {
 		Name:                   entity.Name,
 		NonUniqueName:          entity.NonUniqueName,
 		ServerDefinedUniqueTag: entity.ServerDefinedUniqueTag,
-		ClientDefinedUniqueTag: entity.ClientDefinedUniqueTag,
+		ClientTagHash:          entity.ClientDefinedUniqueTag,
 		OriginatorCacheGuid:    entity.OriginatorCacheGUID,
 		OriginatorClientItemId: entity.OriginatorClientItemID,
 		Deleted:                entity.Deleted,

datastore/sync_entity_test.go

@@ -299,10 +299,10 @@ func (suite *SyncEntityTestSuite) TestUpdateSyncEntity_Basic() {
 	updateEntity1.Deleted = aws.Bool(true)
 	updateEntity1.DataTypeMtime = aws.String("123#23456789")
 	updateEntity1.Specifics = []byte{3, 4}
-	conflict, delete, err := suite.dynamo.UpdateSyncEntity(&updateEntity1, *entity1.Version)
+	conflict, deleted, err := suite.dynamo.UpdateSyncEntity(&updateEntity1, *entity1.Version)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().False(conflict, "Successful update should not have conflict")
-	suite.Assert().True(delete, "Delete operation should return true")
+	suite.Assert().True(deleted, "Delete operation should return true")
 
 	// Update with optional fields.
 	updateEntity2 := updateEntity1
@@ -313,30 +313,30 @@ func (suite *SyncEntityTestSuite) TestUpdateSyncEntity_Basic() {
 	updateEntity2.ParentID = aws.String("parentID")
 	updateEntity2.Name = aws.String("name")
 	updateEntity2.NonUniqueName = aws.String("non_unique_name")
-	conflict, delete, err = suite.dynamo.UpdateSyncEntity(&updateEntity2, *entity2.Version)
+	conflict, deleted, err = suite.dynamo.UpdateSyncEntity(&updateEntity2, *entity2.Version)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().False(conflict, "Successful update should not have conflict")
-	suite.Assert().False(delete, "Non-delete operation should return false")
+	suite.Assert().False(deleted, "Non-delete operation should return false")
 
 	// Update with nil Folder and Deleted
 	updateEntity3 := updateEntity1
 	updateEntity3.ID = "id3"
 	updateEntity3.Folder = nil
 	updateEntity3.Deleted = nil
-	conflict, delete, err = suite.dynamo.UpdateSyncEntity(&updateEntity3, *entity3.Version)
+	conflict, deleted, err = suite.dynamo.UpdateSyncEntity(&updateEntity3, *entity3.Version)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().False(conflict, "Successful update should not have conflict")
-	suite.Assert().False(delete, "Non-delete operation should return false")
+	suite.Assert().False(deleted, "Non-delete operation should return false")
 	// Reset these back to false because they will be the expected value in DB.
 	updateEntity3.Folder = aws.Bool(false)
 	updateEntity3.Deleted = aws.Bool(false)
 
 	// Update entity again with the wrong old version as (version mismatch)
 	// should return false.
-	conflict, delete, err = suite.dynamo.UpdateSyncEntity(&updateEntity2, 12345678)
+	conflict, deleted, err = suite.dynamo.UpdateSyncEntity(&updateEntity2, 12345678)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().True(conflict, "Update with the same version should return conflict")
-	suite.Assert().False(delete, "Conflict operation should return false for delete")
+	suite.Assert().False(deleted, "Conflict operation should return false for delete")
 
 	// Check sync entities are updated correctly in DB.
 	syncItems, err = datastoretest.ScanSyncEntities(suite.dynamo)
@@ -375,24 +375,24 @@ func (suite *SyncEntityTestSuite) TestUpdateSyncEntity_ReuseClientTag() {
 	updateEntity1.Folder = aws.Bool(true)
 	updateEntity1.DataTypeMtime = aws.String("123#23456789")
 	updateEntity1.Specifics = []byte{3, 4}
-	conflict, delete, err := suite.dynamo.UpdateSyncEntity(&updateEntity1, *entity1.Version)
+	conflict, deleted, err := suite.dynamo.UpdateSyncEntity(&updateEntity1, *entity1.Version)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().False(conflict, "Successful update should not have conflict")
-	suite.Assert().False(delete, "Non-delete operation should return false")
+	suite.Assert().False(deleted, "Non-delete operation should return false")
 
 	// Soft-delete the item with wrong version should get conflict.
 	updateEntity1.Deleted = aws.Bool(true)
-	conflict, delete, err = suite.dynamo.UpdateSyncEntity(&updateEntity1, *entity1.Version)
+	conflict, deleted, err = suite.dynamo.UpdateSyncEntity(&updateEntity1, *entity1.Version)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().True(conflict, "Version mismatched update should have conflict")
-	suite.Assert().False(delete, "Failed delete operation should return false")
+	suite.Assert().False(deleted, "Failed delete operation should return false")
 
 	// Soft-delete the item with matched version.
 	updateEntity1.Version = aws.Int64(34567890)
-	conflict, delete, err = suite.dynamo.UpdateSyncEntity(&updateEntity1, 23456789)
+	conflict, deleted, err = suite.dynamo.UpdateSyncEntity(&updateEntity1, 23456789)
 	suite.Require().NoError(err, "UpdateSyncEntity should succeed")
 	suite.Assert().False(conflict, "Successful update should not have conflict")
-	suite.Assert().True(delete, "Delete operation should return true")
+	suite.Assert().True(deleted, "Delete operation should return true")
 
 	// Check tag item is deleted.
 	tagItems, err = datastoretest.ScanTagItems(suite.dynamo)
@@ -564,7 +564,7 @@ func (suite *SyncEntityTestSuite) TestCreateDBSyncEntity() {
 		Name:                   aws.String("name"),
 		NonUniqueName:          aws.String("non_unique_name"),
 		ServerDefinedUniqueTag: aws.String("server_tag"),
-		ClientDefinedUniqueTag: aws.String("client_tag"),
+		ClientTagHash:          aws.String("client_tag"),
 		Deleted:                aws.Bool(false),
 		Folder:                 aws.Bool(false),
 		Specifics:              specifics,
@@ -577,7 +577,7 @@ func (suite *SyncEntityTestSuite) TestCreateDBSyncEntity() {
 		Name:                   pbEntity.Name,
 		NonUniqueName:          pbEntity.NonUniqueName,
 		ServerDefinedUniqueTag: pbEntity.ServerDefinedUniqueTag,
-		ClientDefinedUniqueTag: pbEntity.ClientDefinedUniqueTag,
+		ClientDefinedUniqueTag: pbEntity.ClientTagHash,
 		Deleted:                pbEntity.Deleted,
 		Folder:                 pbEntity.Folder,
 		Specifics:              specificsBytes,
@@ -695,7 +695,7 @@ func (suite *SyncEntityTestSuite) TestCreatePBSyncEntity() {
 		Name:                   dbEntity.Name,
 		NonUniqueName:          dbEntity.NonUniqueName,
 		ServerDefinedUniqueTag: dbEntity.ServerDefinedUniqueTag,
-		ClientDefinedUniqueTag: dbEntity.ClientDefinedUniqueTag,
+		ClientTagHash:          dbEntity.ClientDefinedUniqueTag,
 		OriginatorCacheGuid:    dbEntity.OriginatorCacheGUID,
 		OriginatorClientItemId: dbEntity.OriginatorClientItemID,
 		Deleted:                dbEntity.Deleted,

go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/getsentry/sentry-go v0.11.0
 	github.com/go-chi/chi v4.1.2+incompatible
 	github.com/go-redis/redis/v8 v8.10.0
-	github.com/prometheus/client_golang v1.11.0
+	github.com/prometheus/client_golang v1.11.1
 	github.com/rs/zerolog v1.23.0
 	github.com/satori/go.uuid v1.2.0
 	github.com/stretchr/testify v1.7.0
@@ -37,7 +37,7 @@ require (
 	go.opentelemetry.io/otel v0.20.0 // indirect
 	go.opentelemetry.io/otel/metric v0.20.0 // indirect
 	go.opentelemetry.io/otel/trace v0.20.0 // indirect
-	golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a // indirect
-	golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6 // indirect
+	golang.org/x/crypto v0.7.0 // indirect
+	golang.org/x/sys v0.6.0 // indirect
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
 )