Merge pull request juju#18407 from Aflynn50/objectstore-hash-error

juju#18407

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.

The error check in agent_binary was removed as this was handled at a lower level and the comment above it was incorrect. The error is ignored below because there is already a binary, not because the hash is the same.
## QA 
It compiles and unit tests pass.
A grep for old error and old error text returns nothing.
<!-- Describe steps to verify that the change works. -->

domain/objectstore/errors/errors.go

@@ -14,6 +14,7 @@ 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 path already exists
+	// with a different hash.
+	ErrPathAlreadyExistsDifferentHash = errors.ConstError("path already exists with different hash")
 )

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")
 		}

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) {

internal/bootstrap/agentbinary.go

@@ -19,7 +19,6 @@ import (
 	"github.com/juju/juju/core/logger"
 	coreos "github.com/juju/juju/core/os"
 	jujuversion "github.com/juju/juju/core/version"
-	objectstoreerrors "github.com/juju/juju/domain/objectstore/errors"
 	"github.com/juju/juju/state/binarystorage"
 )
 
@@ -64,8 +63,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 {
 		return nil, errors.Trace(err)
 	}
 

internal/bootstrap/agentbinary_test.go

@@ -19,7 +19,6 @@ import (
 	"github.com/juju/juju/core/arch"
 	coreos "github.com/juju/juju/core/os"
 	jujuversion "github.com/juju/juju/core/version"
-	objectstoreerrors "github.com/juju/juju/domain/objectstore/errors"
 	"github.com/juju/juju/state/binarystorage"
 )
 
@@ -63,40 +62,6 @@ func (s *agentBinarySuite) TestPopulateAgentBinary(c *gc.C) {
 	s.expectNoTools(c, toolsPath)
 }
 
-func (s *agentBinarySuite) TestPopulateAgentBinaryTwiceShouldSucceed(c *gc.C) {
-	defer s.setupMocks(c).Finish()
-
-	current := version.Binary{
-		Number:  jujuversion.Current,
-		Arch:    arch.HostArch(),
-		Release: coreos.HostOSTypeName(),
-	}
-
-	dir, toolsPath := s.ensureDirs(c, current)
-	size := int64(4)
-
-	s.writeDownloadTools(c, toolsPath, downloadTools{
-		Version: current.String(),
-		URL:     filepath.Join(dir, "tools", fmt.Sprintf("%s.tgz", current.String())),
-		SHA256:  "sha256",
-		Size:    size,
-	})
-
-	s.writeAgentBinary(c, toolsPath, current)
-
-	s.storage.EXPECT().Add(gomock.Any(), gomock.Any(), binarystorage.Metadata{
-		Version: current.String(),
-		Size:    size,
-		SHA256:  "sha256",
-	}).Return(objectstoreerrors.ErrHashAlreadyExists)
-
-	cleanup, err := PopulateAgentBinary(context.Background(), dir, s.storage, s.logger)
-	c.Assert(err, jc.ErrorIsNil)
-	cleanup()
-
-	s.expectNoTools(c, toolsPath)
-}
-
 func (s *agentBinarySuite) TestPopulateAgentBinaryAddError(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 

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")
 	}
 

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,

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() {