fix: change error message from hash to path already exists

Aflynn50 · Aflynn50 · commit b27bb990ac51 · 2024-11-27T11:50:30.000Z
This error was returned when an object was already stored at a given
path and had a different hash to the object being put in. It said hash
already exists which was wrong. Now it reports that the path already
exists with a different hash.
diff --git a/domain/objectstore/errors/errors.go b/domain/objectstore/errors/errors.go
@@ -14,6 +14,6 @@ const (
 	// there is a collision in the hash function.
 	ErrHashAndSizeAlreadyExists = errors.ConstError("hash exists for different file size")
 
-	// ErrHashAlreadyExists is returned when a hash already exists.
-	ErrHashAlreadyExists = errors.ConstError("hash already exists")
+	// ErrPathAlreadyExistsDifferentHash is returned when a hash already exists.
+	ErrPathAlreadyExistsDifferentHash = errors.ConstError("path already exists with different hash")
 )
diff --git a/domain/objectstore/state/state.go b/domain/objectstore/state/state.go
@@ -163,7 +163,7 @@ AND    size = $dbMetadata.size`, dbMetadata, dbMetadataPath)
 
 		err = tx.Query(ctx, pathStmt, dbMetadataPath).Get(&outcome)
 		if database.IsErrConstraintPrimaryKey(err) {
-			return objectstoreerrors.ErrHashAlreadyExists
+			return objectstoreerrors.ErrPathAlreadyExistsDifferentHash
 		} else if err != nil {
 			return errors.Annotatef(err, "inserting metadata path")
 		}
diff --git a/domain/objectstore/state/state_test.go b/domain/objectstore/state/state_test.go
@@ -102,7 +102,7 @@ func (s *stateSuite) TestPutMetadataConflict(c *gc.C) {
 
 	_, err = st.PutMetadata(context.Background(), metadata)
 	c.Assert(err, gc.Not(jc.ErrorIsNil))
-	c.Check(err, jc.ErrorIs, objectstoreerrors.ErrHashAlreadyExists)
+	c.Check(err, jc.ErrorIs, objectstoreerrors.ErrPathAlreadyExistsDifferentHash)
 }
 
 func (s *stateSuite) TestPutMetadataWithSameHashAndSize(c *gc.C) {
diff --git a/internal/bootstrap/agentbinary.go b/internal/bootstrap/agentbinary.go
@@ -65,7 +65,7 @@ func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinar
 	logger.Debugf("Adding agent binary: %v", agentTools.Version)
 
 	// If the hash already exists, we don't need to add it again.
-	if err := storage.Add(ctx, bytes.NewReader(data), metadata); err != nil && !errors.Is(err, objectstoreerrors.ErrHashAlreadyExists) {
+	if err := storage.Add(ctx, bytes.NewReader(data), metadata); err != nil && !errors.Is(err, objectstoreerrors.ErrPathAlreadyExistsDifferentHash) {
 		return nil, errors.Trace(err)
 	}
 
diff --git a/internal/bootstrap/agentbinary_test.go b/internal/bootstrap/agentbinary_test.go
@@ -88,7 +88,7 @@ func (s *agentBinarySuite) TestPopulateAgentBinaryTwiceShouldSucceed(c *gc.C) {
 		Version: current.String(),
 		Size:    size,
 		SHA256:  "sha256",
-	}).Return(objectstoreerrors.ErrHashAlreadyExists)
+	}).Return(objectstoreerrors.ErrPathAlreadyExistsDifferentHash)
 
 	cleanup, err := PopulateAgentBinary(context.Background(), dir, s.storage, s.logger)
 	c.Assert(err, jc.ErrorIsNil)
diff --git a/internal/charm/services/storage.go b/internal/charm/services/storage.go
@@ -72,7 +72,7 @@ func (s *CharmStorage) Store(ctx context.Context, charmURL string, downloadedCha
 	}
 
 	// If the blob is already stored, we can skip the upload.
-	if _, err := s.objectStore.Put(ctx, storagePath, downloadedCharm.CharmData, downloadedCharm.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrHashAlreadyExists) {
+	if _, err := s.objectStore.Put(ctx, storagePath, downloadedCharm.CharmData, downloadedCharm.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrPathAlreadyExistsDifferentHash) {
 		return "", errors.Annotate(err, "cannot add charm to storage")
 	}
 
diff --git a/internal/charm/services/storage_test.go b/internal/charm/services/storage_test.go
@@ -91,7 +91,7 @@ func (s *storageTestSuite) TestStoreBlobAlreadyStored(c *gc.C) {
 		CharmVersion: "the-version",
 	}
 
-	s.storageBackend.EXPECT().Put(gomock.Any(), expStoreCharmPath, gomock.AssignableToTypeOf(dlCharm.CharmData), int64(7337)).Return("", objectstoreerrors.ErrHashAlreadyExists)
+	s.storageBackend.EXPECT().Put(gomock.Any(), expStoreCharmPath, gomock.AssignableToTypeOf(dlCharm.CharmData), int64(7337)).Return("", objectstoreerrors.ErrPathAlreadyExistsDifferentHash)
 	s.stateBackend.EXPECT().UpdateUploadedCharm(state.CharmInfo{
 		StoragePath: expStoreCharmPath,
 		ID:          curl,
diff --git a/state/binarystorage/binarystorage.go b/state/binarystorage/binarystorage.go
@@ -65,7 +65,7 @@ func New(
 func (s *binaryStorage) Add(ctx context.Context, r io.Reader, metadata Metadata) (resultErr error) {
 	// Add the binary file to storage.
 	path := fmt.Sprintf("tools/%s-%s", metadata.Version, metadata.SHA256)
-	if _, err := s.managedStorage.Put(ctx, path, r, metadata.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrHashAlreadyExists) {
+	if _, err := s.managedStorage.Put(ctx, path, r, metadata.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrPathAlreadyExistsDifferentHash) {
 		return errors.Annotate(err, "cannot store binary file")
 	}
 	defer func() {

Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,6 @@ const (`
`14`	`14`	`// there is a collision in the hash function.`
`15`	`15`	`ErrHashAndSizeAlreadyExists = errors.ConstError("hash exists for different file size")`
`16`	`16`
`17`		`- // ErrHashAlreadyExists is returned when a hash already exists.`
`18`		`- ErrHashAlreadyExists = errors.ConstError("hash already exists")`
	`17`	`+ // ErrPathAlreadyExistsDifferentHash is returned when a hash already exists.`
	`18`	`+ ErrPathAlreadyExistsDifferentHash = errors.ConstError("path already exists with different hash")`
`19`	`19`	`)`
Original file line number	Diff line number	Diff line change
@@ -163,7 +163,7 @@ AND size = $dbMetadata.size`, dbMetadata, dbMetadataPath)
`163`	`163`
`164`	`164`	`err = tx.Query(ctx, pathStmt, dbMetadataPath).Get(&outcome)`
`165`	`165`	`if database.IsErrConstraintPrimaryKey(err) {`
`166`		`- return objectstoreerrors.ErrHashAlreadyExists`
	`166`	`+ return objectstoreerrors.ErrPathAlreadyExistsDifferentHash`
`167`	`167`	`} else if err != nil {`
`168`	`168`	`return errors.Annotatef(err, "inserting metadata path")`
`169`	`169`	`}`
Original file line number	Diff line number	Diff line change
`@@ -102,7 +102,7 @@ func (s stateSuite) TestPutMetadataConflict(c gc.C) {`
`102`	`102`
`103`	`103`	`_, err = st.PutMetadata(context.Background(), metadata)`
`104`	`104`	`c.Assert(err, gc.Not(jc.ErrorIsNil))`
`105`		`- c.Check(err, jc.ErrorIs, objectstoreerrors.ErrHashAlreadyExists)`
	`105`	`+ c.Check(err, jc.ErrorIs, objectstoreerrors.ErrPathAlreadyExistsDifferentHash)`
`106`	`106`	`}`
`107`	`107`
`108`	`108`	`func (s stateSuite) TestPutMetadataWithSameHashAndSize(c gc.C) {`
Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ func PopulateAgentBinary(ctx context.Context, dataDir string, storage AgentBinar`
`65`	`65`	`logger.Debugf("Adding agent binary: %v", agentTools.Version)`
`66`	`66`
`67`	`67`	`// If the hash already exists, we don't need to add it again.`
`68`		`- if err := storage.Add(ctx, bytes.NewReader(data), metadata); err != nil && !errors.Is(err, objectstoreerrors.ErrHashAlreadyExists) {`
	`68`	`+ if err := storage.Add(ctx, bytes.NewReader(data), metadata); err != nil && !errors.Is(err, objectstoreerrors.ErrPathAlreadyExistsDifferentHash) {`
`69`	`69`	`return nil, errors.Trace(err)`
`70`	`70`	`}`
`71`	`71`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ func (s *CharmStorage) Store(ctx context.Context, charmURL string, downloadedCha`
`72`	`72`	`}`
`73`	`73`
`74`	`74`	`// If the blob is already stored, we can skip the upload.`
`75`		`- if _, err := s.objectStore.Put(ctx, storagePath, downloadedCharm.CharmData, downloadedCharm.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrHashAlreadyExists) {`
	`75`	`+ if _, err := s.objectStore.Put(ctx, storagePath, downloadedCharm.CharmData, downloadedCharm.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrPathAlreadyExistsDifferentHash) {`
`76`	`76`	`return "", errors.Annotate(err, "cannot add charm to storage")`
`77`	`77`	`}`
`78`	`78`
Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@ func (s storageTestSuite) TestStoreBlobAlreadyStored(c gc.C) {`
`91`	`91`	`CharmVersion: "the-version",`
`92`	`92`	`}`
`93`	`93`
`94`		`- s.storageBackend.EXPECT().Put(gomock.Any(), expStoreCharmPath, gomock.AssignableToTypeOf(dlCharm.CharmData), int64(7337)).Return("", objectstoreerrors.ErrHashAlreadyExists)`
	`94`	`+ s.storageBackend.EXPECT().Put(gomock.Any(), expStoreCharmPath, gomock.AssignableToTypeOf(dlCharm.CharmData), int64(7337)).Return("", objectstoreerrors.ErrPathAlreadyExistsDifferentHash)`
`95`	`95`	`s.stateBackend.EXPECT().UpdateUploadedCharm(state.CharmInfo{`
`96`	`96`	`StoragePath: expStoreCharmPath,`
`97`	`97`	`ID: curl,`
Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ func New(`
`65`	`65`	`func (s *binaryStorage) Add(ctx context.Context, r io.Reader, metadata Metadata) (resultErr error) {`
`66`	`66`	`// Add the binary file to storage.`
`67`	`67`	`path := fmt.Sprintf("tools/%s-%s", metadata.Version, metadata.SHA256)`
`68`		`- if _, err := s.managedStorage.Put(ctx, path, r, metadata.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrHashAlreadyExists) {`
	`68`	`+ if _, err := s.managedStorage.Put(ctx, path, r, metadata.Size); err != nil && !errors.Is(err, objectstoreerrors.ErrPathAlreadyExistsDifferentHash) {`
`69`	`69`	`return errors.Annotate(err, "cannot store binary file")`
`70`	`70`	`}`
`71`	`71`	`defer func() {`