Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(policy): 1421 tech debt migrate Resource Mappings object queries to sqlc #1422

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
48f42ef
migrate RM db logic to sqlc
ryanulit Aug 21, 2024
26d5544
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Aug 23, 2024
7e7f4c1
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Aug 27, 2024
9b8d269
update CRUD logic
ryanulit Aug 27, 2024
4bddc3f
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Aug 29, 2024
109e684
refactor updates
ryanulit Aug 29, 2024
61b2636
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Sep 4, 2024
823728e
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Sep 4, 2024
d57f62b
update sql queries to follow adr
ryanulit Sep 4, 2024
fc4463c
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Sep 12, 2024
507fb59
db and service changes to meet ADR
ryanulit Sep 12, 2024
5e8dc65
add missing import
ryanulit Sep 12, 2024
8b56379
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Oct 1, 2024
dea840a
refactor service update calls to us db layer return value directly
ryanulit Oct 1, 2024
58aea0f
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Oct 1, 2024
4b35725
final changes
ryanulit Oct 1, 2024
8628b38
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Oct 1, 2024
2812a25
undo temp public routes
ryanulit Oct 1, 2024
514f039
test cleanup
ryanulit Oct 1, 2024
e5d931c
pr feedback changes
ryanulit Oct 2, 2024
aa30fa1
Merge branch 'main' into feat/1421-migrate-resource-mapping-object-qu…
ryanulit Oct 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 17 additions & 29 deletions service/integration/resource_mappings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"log/slog"
"testing"
"time"

"github.com/opentdf/platform/protocol/go/common"
"github.com/opentdf/platform/protocol/go/policy/resourcemapping"
Expand Down Expand Up @@ -178,8 +177,6 @@ func (s *ResourceMappingsSuite) Test_UpdateResourceMappingGroup() {
"new": newLabel,
}

start := time.Now().Add(-time.Second)

createdGroup, err := s.db.PolicyClient.CreateResourceMappingGroup(s.ctx, &resourcemapping.CreateResourceMappingGroupRequest{
NamespaceId: s.getExampleDotComNamespace().ID,
Name: "example.com_ns_group_created",
Expand Down Expand Up @@ -207,19 +204,16 @@ func (s *ResourceMappingsSuite) Test_UpdateResourceMappingGroup() {
s.Require().NoError(err)
s.NotNil(gotGroup)

end := time.Now().Add(time.Second)

s.Equal(createdGroup.GetId(), gotGroup.GetId())
s.Equal(updateReq.GetNamespaceId(), gotGroup.GetNamespaceId())
s.Equal(updateReq.GetName(), gotGroup.GetName())
metadata := gotGroup.GetMetadata()
createdAt := metadata.GetCreatedAt()
updatedAt := metadata.GetUpdatedAt()
s.True(createdAt.AsTime().After(start))
s.False(createdAt.AsTime().IsZero())
s.False(updatedAt.AsTime().IsZero())
s.True(updatedAt.AsTime().After(createdAt.AsTime()))
s.True(createdAt.AsTime().Before(end))
s.True(updatedAt.AsTime().Before(end))
s.Equal(expectedLabels, gotGroup.GetMetadata().GetLabels())
s.Equal(expectedLabels, metadata.GetLabels())
}

func (s *ResourceMappingsSuite) Test_UpdateResourceMappingGroupWithUnknownIdFails() {
Expand Down Expand Up @@ -621,34 +615,19 @@ func (s *ResourceMappingsSuite) Test_UpdateResourceMapping() {

terms := []string{"some term", "other term"}
updateTerms := []string{"updated term1", "updated term 2"}

attrValue := s.f.GetAttributeValueKey("example.com/attr/attr2/value/value2")
start := time.Now().Add(-time.Second)

createdMapping, err := s.db.PolicyClient.CreateResourceMapping(s.ctx, &resourcemapping.CreateResourceMappingRequest{
AttributeValueId: attrValue.ID,
Metadata: &common.MetadataMutable{
Labels: labels,
},
Terms: terms,
Terms: terms,
GroupId: rmGroup.ID,
})
end := time.Now().Add(time.Second)
metadata := createdMapping.GetMetadata()
updatedAt := metadata.GetUpdatedAt()
createdAt := metadata.GetCreatedAt()
s.True(createdAt.AsTime().After(start))
s.True(createdAt.AsTime().Before(end))
s.Require().NoError(err)
s.NotNil(createdMapping)

if v, err := s.db.PolicyClient.GetResourceMapping(s.ctx, createdMapping.GetId()); err != nil {
s.Require().NoError(err)
} else {
s.NotNil(v)
s.Equal(createdMapping.GetId(), v.GetId())
s.Equal(createdMapping.GetAttributeValue().GetId(), v.GetAttributeValue().GetId())
s.Equal(createdMapping.GetTerms(), v.GetTerms())
}

updateWithoutChange, err := s.db.PolicyClient.UpdateResourceMapping(s.ctx, createdMapping.GetId(), &resourcemapping.UpdateResourceMappingRequest{})
s.Require().NoError(err)
s.NotNil(updateWithoutChange)
Expand All @@ -662,11 +641,15 @@ func (s *ResourceMappingsSuite) Test_UpdateResourceMapping() {
Labels: updateLabels,
},
MetadataUpdateBehavior: common.MetadataUpdateEnum_METADATA_UPDATE_ENUM_EXTEND,
GroupId: rmGroup.ID,
GroupId: createdMapping.GetGroup().GetId(),
})
s.Require().NoError(err)
s.NotNil(updateWithChange)
s.Equal(createdMapping.GetId(), updateWithChange.GetId())
s.Equal(createdMapping.GetAttributeValue().GetId(), updateWithChange.GetAttributeValue().GetId())
s.Equal(updateTerms, updateWithChange.GetTerms())
s.EqualValues(expectedLabels, updateWithChange.GetMetadata().GetLabels())
s.Equal(createdMapping.GetGroup().GetId(), updateWithChange.GetGroup().GetId())

// get after update to verify db reflects changes made
got, err := s.db.PolicyClient.GetResourceMapping(s.ctx, createdMapping.GetId())
Expand All @@ -676,7 +659,12 @@ func (s *ResourceMappingsSuite) Test_UpdateResourceMapping() {
s.Equal(createdMapping.GetAttributeValue().GetId(), got.GetAttributeValue().GetId())
s.Equal(updateTerms, got.GetTerms())
s.EqualValues(expectedLabels, got.GetMetadata().GetLabels())
s.True(got.GetMetadata().GetUpdatedAt().AsTime().After(updatedAt.AsTime()))
metadata := got.GetMetadata()
createdAt := metadata.GetCreatedAt()
updatedAt := metadata.GetUpdatedAt()
s.False(createdAt.AsTime().IsZero())
s.False(updatedAt.AsTime().IsZero())
s.True(updatedAt.AsTime().After(createdAt.AsTime()))
s.Equal(rmGroup.ID, got.GetGroup().GetId())
}

Expand Down
68 changes: 68 additions & 0 deletions service/logger/audit/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,32 @@ func Test_CreatePolicyEvent_WithOriginal(t *testing.T) {
"event original did not match expected: got %+v expected %+v", event.Original, expected)
}

func Test_CreatePolicyEvent_WithUpdated_StringPropertyModified(t *testing.T) {
params := PolicyEventParams{
ActionType: ActionTypeCreate,
ObjectID: originalPolicyObject.GetId(),
ObjectType: ObjectTypeKeyObject,
Original: originalPolicyObject,
Updated: &TestPolicyObject{
Id: "5678",
},
}

expected := map[string]interface{}{
"id": "5678",
"active": true,
"version": "TEST_POLICY_OBJECT_VERSION_ENUM_OLD",
// []interface{} must be used because json.Unmarshal returns []interface{} for JSON arrays
"tags": []interface{}{"tag1", "tag2"},
"username": "test-username",
"metadata": map[string]interface{}{
"labels": map[string]interface{}{"key": "value"},
},
}

runWithUpdatedTest(t, params, expected)
}

func Test_CreatePolicyEvent_WithUpdated_BoolPropertyModified(t *testing.T) {
params := PolicyEventParams{
ActionType: ActionTypeCreate,
Expand Down Expand Up @@ -263,3 +289,45 @@ func Test_CreatePolicyEvent_WithUpdated_MetadataPropertyModified(t *testing.T) {

runWithUpdatedTest(t, params, expected)
}

/*
This test ensures that any nil values in the updated object returned from the DB layer
are populated in the audit event updated object appropriately. For conditional updates,
the DB layer returns the values received from the service layer request as-is, meaning
that any optional properties not specified in the update request are returned from the DB
layer as the appropriate zero value. The CreatePolicyEvent function should then use the
value from the audit event original object to populate the updated object value.

TODO: This will be simplified in the future by querying the DB for the updated state, so
we can remove this test once that is implemented safely with transactions.
*/
func Test_CreatePolicyEvent_WithUpdated_ZeroValuePropertiesModified_UseOriginalPropertyValues(t *testing.T) {
params := PolicyEventParams{
ActionType: ActionTypeCreate,
ObjectID: originalPolicyObject.GetId(),
ObjectType: ObjectTypeKeyObject,
Original: originalPolicyObject,
Updated: &TestPolicyObject{
Id: "",
Active: nil,
Version: TestPolicyObjectVersionEnum_TEST_POLICY_OBJECT_VERSION_ENUM_UNSPECIFIED,
Tags: nil,
PolicyUser: nil,
Metadata: nil,
},
}

expected := map[string]interface{}{
"id": "1234",
"active": true,
"version": "TEST_POLICY_OBJECT_VERSION_ENUM_OLD",
// []interface{} must be used because json.Unmarshal returns []interface{} for JSON arrays
"tags": []interface{}{"tag1", "tag2"},
"username": "test-username",
"metadata": map[string]interface{}{
"labels": map[string]interface{}{"key": "value"},
},
}

runWithUpdatedTest(t, params, expected)
}
2 changes: 1 addition & 1 deletion service/policy/db/attribute_values.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func (c PolicyDBClient) CreateAttributeValue(ctx context.Context, attributeID st
return nil, db.WrapIfKnownInvalidQueryErr(err)
}

if err = unmarshalMetadata(metadataJSON, metadata, c.logger); err != nil {
if err = unmarshalMetadata(metadataJSON, metadata); err != nil {
return nil, err
}

Expand Down
4 changes: 2 additions & 2 deletions service/policy/db/attributes.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ func (c PolicyDBClient) GetAttributeByFqn(ctx context.Context, fqn string) (*pol

m := new(common.Metadata)
if fullAttr.Metadata != nil {
err = unmarshalMetadata(fullAttr.Metadata, m, c.logger)
err = unmarshalMetadata(fullAttr.Metadata, m)
if err != nil {
c.logger.Error("could not unmarshal metadata", slog.String("error", err.Error()))
return nil, err
Expand Down Expand Up @@ -518,7 +518,7 @@ func (c PolicyDBClient) CreateAttribute(ctx context.Context, r *attributes.Creat
return nil, db.WrapIfKnownInvalidQueryErr(err)
}

if err = unmarshalMetadata(metadataJSON, metadata, c.logger); err != nil {
if err = unmarshalMetadata(metadataJSON, metadata); err != nil {
return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion service/policy/db/namespaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (c PolicyDBClient) ListNamespaces(ctx context.Context, state string) ([]*po

for i, ns := range list {
metadata := &common.Metadata{}
if err = unmarshalMetadata(ns.Metadata, metadata, c.logger); err != nil {
if err = unmarshalMetadata(ns.Metadata, metadata); err != nil {
return nil, err
}

Expand Down
48 changes: 44 additions & 4 deletions service/policy/db/query.sql
Original file line number Diff line number Diff line change
Expand Up @@ -252,14 +252,13 @@ INSERT INTO resource_mapping_groups (namespace_id, name, metadata)
VALUES ($1, $2, $3)
RETURNING id;

-- name: UpdateResourceMappingGroup :one
-- name: UpdateResourceMappingGroup :execrows
UPDATE resource_mapping_groups
SET
namespace_id = COALESCE(sqlc.narg('namespace_id'), namespace_id),
name = COALESCE(sqlc.narg('name'), name),
metadata = COALESCE(sqlc.narg('metadata'), metadata)
WHERE id = $1
RETURNING id;
WHERE id = $1;

-- name: DeleteResourceMappingGroup :execrows
DELETE FROM resource_mapping_groups WHERE id = $1;
Expand All @@ -268,6 +267,18 @@ DELETE FROM resource_mapping_groups WHERE id = $1;
-- RESOURCE MAPPING
----------------------------------------------------------------

-- name: ListResourceMappings :many
SELECT
m.id,
JSON_BUILD_OBJECT('id', av.id, 'value', av.value) as attribute_value,
m.terms,
JSON_STRIP_NULLS(JSON_BUILD_OBJECT('labels', m.metadata -> 'labels', 'created_at', m.created_at, 'updated_at', m.updated_at)) as metadata,
COALESCE(m.group_id::TEXT, '')::TEXT as group_id
FROM resource_mappings m
LEFT JOIN attribute_values av on m.attribute_value_id = av.id
WHERE (NULLIF(@group_id, '') IS NULL OR m.group_id = @group_id::UUID)
GROUP BY av.id, m.id;

-- name: ListResourceMappingsByFullyQualifiedGroup :many
SELECT
m.id,
Expand All @@ -283,7 +294,36 @@ SELECT
FROM resource_mappings m
LEFT JOIN resource_mapping_groups g ON m.group_id = g.id
LEFT JOIN attribute_namespaces ns ON g.namespace_id = ns.id
WHERE ns.name = LOWER(@namespace_name) AND g.name = LOWER(@group_name);
WHERE ns.name = @namespace_name AND g.name = @group_name;

-- name: GetResourceMapping :one
SELECT
m.id,
JSON_BUILD_OBJECT('id', av.id, 'value', av.value) as attribute_value,
m.terms,
JSON_STRIP_NULLS(JSON_BUILD_OBJECT('labels', m.metadata -> 'labels', 'created_at', m.created_at, 'updated_at', m.updated_at)) as metadata,
COALESCE(m.group_id::TEXT, '')::TEXT as group_id
FROM resource_mappings m
LEFT JOIN attribute_values av on m.attribute_value_id = av.id
WHERE m.id = $1
GROUP BY av.id, m.id;

-- name: CreateResourceMapping :one
INSERT INTO resource_mappings (attribute_value_id, terms, metadata, group_id)
VALUES ($1, $2, $3, $4)
RETURNING id;

-- name: UpdateResourceMapping :execrows
UPDATE resource_mappings
SET
attribute_value_id = COALESCE(sqlc.narg('attribute_value_id'), attribute_value_id),
terms = COALESCE(sqlc.narg('terms'), terms),
metadata = COALESCE(sqlc.narg('metadata'), metadata),
group_id = COALESCE(sqlc.narg('group_id'), group_id)
WHERE id = $1;

-- name: DeleteResourceMapping :execrows
DELETE FROM resource_mappings WHERE id = $1;

----------------------------------------------------------------
-- NAMESPACES
Expand Down
Loading
Loading