Fix setting host catalog attributes to nil (#1743)

Internally attributes are not allowed to be the null value. However, as
currently implemented, this then breaks our normal model of sending an
explicit `null` in the JSON to reset/clear a field. This is a problem
because the CLI doesn't have a way to say "set this but to empty"
without a lot of hoops, and causes some issues for API users and TF as
well.

This changes PatchBytes and PatchStruct to return an empty value (or
marshaled empty value in the case of PatchBytes) if the source is null
or empty. In the host catalog update function we then always set this
value if it's in the db mask (removing the nil check). Together this
means a null value ends up coming out as an empty value (and thus valid
for the DB) from PatchStruct/PatchBytes, and our API translation
function already returns nothing for attributes if the length of the
fields are zero.

internal/host/plugin/repository_host_catalog.go

@@ -286,7 +286,7 @@ func (r *Repository) UpdateCatalog(ctx context.Context, c *HostCatalog, version
 		}
 	}
 
-	if updateAttributes && c.Attributes != nil {
+	if updateAttributes {
 		dbMask = append(dbMask, "attributes")
 		newCatalog.Attributes, err = patchstruct.PatchBytes(newCatalog.Attributes, c.Attributes)
 		if err != nil {

internal/libs/patchstruct/patchstruct.go

@@ -7,15 +7,16 @@ import (
 	"google.golang.org/protobuf/types/known/structpb"
 )
 
-// PatchStruct updates the struct found in dst with the values found in src.
-// The intent of this helper is to provide a fallback mechanism for subtype
+// PatchStruct updates the struct found in dst with the values found in src. The
+// intent of this helper is to provide a fallback mechanism for subtype
 // attributes when the actual schema of the subtype attributes are unknown. As
 // such, it's preferred to use other methods (such as mask mapping) when an
 // actual message for the subtype is known.
 //
 // The following rules apply:
 //
-// * The source (src) map is merged into the destination map (dst).
+// * The source (src) map is merged into the destination map (dst). If the
+// source map is nil, the destination will be a valid, but empty, map.
 //
 // * Values are overwritten by the source map if they exist in both.
 //
@@ -32,6 +33,10 @@ import (
 //
 // PatchStruct returns the updated map as a copy, dst and src are not altered.
 func PatchStruct(dst, src *structpb.Struct) *structpb.Struct {
+	if src == nil {
+		ret, _ := structpb.NewStruct(nil)
+		return ret
+	}
 	result, err := structpb.NewStruct(patchM(dst.AsMap(), src.AsMap()))
 	if err != nil {
 		// Should never error as values are source from structpb values
@@ -41,21 +46,25 @@ func PatchStruct(dst, src *structpb.Struct) *structpb.Struct {
 	return result
 }
 
-// PatchBytes follows the same rules as above with PatchStruct, but
-// instead of patching structpb.Structs, it patches the protobuf
-// encoding. An error is returned if there are issues working with
-// the data.
+// PatchBytes follows the same rules as above with PatchStruct, but instead of
+// patching structpb.Structs, it patches the protobuf encoding. An error is
+// returned if there are issues working with the data. If src is nil or empty,
+// the result is a marshaled, empty struct.
 func PatchBytes(dst, src []byte) ([]byte, error) {
 	srcpb, dstpb := new(structpb.Struct), new(structpb.Struct)
-	if err := proto.Unmarshal(dst, dstpb); err != nil {
-		return nil, fmt.Errorf("error reading destination data: %w", err)
-	}
-
-	if err := proto.Unmarshal(src, srcpb); err != nil {
-		return nil, fmt.Errorf("error reading source data: %w", err)
+	if len(src) != 0 {
+		if err := proto.Unmarshal(dst, dstpb); err != nil {
+			return nil, fmt.Errorf("error reading destination data: %w", err)
+		}
+		if err := proto.Unmarshal(src, srcpb); err != nil {
+			return nil, fmt.Errorf("error reading source data: %w", err)
+		}
+		dstpb = PatchStruct(dstpb, srcpb)
+	} else {
+		dstpb, _ = structpb.NewStruct(nil)
 	}
 
-	result, err := proto.Marshal(PatchStruct(dstpb, srcpb))
+	result, err := proto.Marshal(dstpb)
 	if err != nil {
 		return nil, fmt.Errorf("error writing result data: %w", err)
 	}

internal/libs/patchstruct/patchstruct_test.go

@@ -118,10 +118,8 @@ var testCases = []testCase{
 		dst: map[string]interface{}{
 			"foo": "bar",
 		},
-		src: nil,
-		expected: map[string]interface{}{
-			"foo": "bar",
-		},
+		src:      nil,
+		expected: nil,
 	},
 	{
 		name: "nil dst",
@@ -189,12 +187,6 @@ func TestPatchBytes(t *testing.T) {
 }
 
 func TestPatchBytesErr(t *testing.T) {
-	t.Run("dst", func(t *testing.T) {
-		require := require.New(t)
-		_, err := PatchBytes([]byte("foo"), nil)
-		require.ErrorIs(err, proto.Error)
-		require.True(strings.HasPrefix(err.Error(), "error reading destination data: "))
-	})
 	t.Run("src", func(t *testing.T) {
 		require := require.New(t)
 		_, err := PatchBytes(nil, []byte("foo"))

internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go

@@ -1470,6 +1470,20 @@ func TestUpdate_Plugin(t *testing.T) {
 				assert.Equal(t, map[string]interface{}{"newkey": "newvalue"}, in.GetAttributes().AsMap())
 			},
 		},
+		{
+			name:  "Update empty attributes for catalog",
+			masks: []string{"attributes"},
+			changes: []updateFn{
+				clearReadOnlyFields(),
+				updateAttrs(func() *structpb.Struct {
+					ret, _ := structpb.NewStruct(nil)
+					return ret
+				}()),
+			},
+			check: func(t *testing.T, in *pb.HostCatalog) {
+				assert.Equal(t, (*structpb.Struct)(nil), in.GetAttributes())
+			},
+		},
 		{
 			name:  "Update secrets",
 			masks: []string{"secrets.key1", "secrets.key2"},