From 1c39691023a9303721d995fefe3325aecbfec675 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Fri, 26 Nov 2021 13:58:19 -0800 Subject: [PATCH] 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. --- .../host/plugin/repository_host_catalog.go | 2 +- internal/libs/patchstruct/patchstruct.go | 37 ++++++++++++------- internal/libs/patchstruct/patchstruct_test.go | 12 +----- .../host_catalog_service_test.go | 14 +++++++ 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/internal/host/plugin/repository_host_catalog.go b/internal/host/plugin/repository_host_catalog.go index 1139109780..8f2114c7ce 100644 --- a/internal/host/plugin/repository_host_catalog.go +++ b/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 { diff --git a/internal/libs/patchstruct/patchstruct.go b/internal/libs/patchstruct/patchstruct.go index 313edb821f..6ed6e1eabb 100644 --- a/internal/libs/patchstruct/patchstruct.go +++ b/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) } diff --git a/internal/libs/patchstruct/patchstruct_test.go b/internal/libs/patchstruct/patchstruct_test.go index efb8f192f8..7eb4118013 100644 --- a/internal/libs/patchstruct/patchstruct_test.go +++ b/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")) diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go index a8dcf3b078..6e62c68d99 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go +++ b/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"},