feat: bulk deletion of relation tuples #799

vancanhuit · 2021-12-18T08:28:54Z

Related issue(s)

Closes #599

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

zepatrik

This is already a good start, but needs some improvements. Also, the same behavior should be reflected over gRPC as well.

zepatrik · 2021-12-20T10:34:50Z

internal/relationtuple/transact_server.go

+	}
+	l.Debug("deleting relation tuples")
+
+	rels, _, err := h.d.RelationTupleManager().GetRelationTuples(r.Context(), query)


This is quite problematic as you only delete the first page here. Can you instead implement the selection of delete candidates on the persistence layer? A request DELETE /relation-tuples?namespace=foo should result in the following SQL statement to be executed: DELETE FROM keto_relation_tuples WHERE namespace=foo.

Aha, yeah I will try. For the gRPC part, I may need time to understand it because I've never worked with gRPC before.

Ah so there is no specific delete RPC, but only the transact one. I would add a DeleteReleationTuples RPC here

keto/proto/ory/keto/acl/v1alpha1/write_service.proto

Lines 14 to 20 in 67c4c60

// The write service to create and delete Access Control Lists.

//

// This service is part of the [write-APIs](../concepts/api-overview.mdx#write-apis).

service WriteService {

// Writes one or more relation tuples in a single transaction.

rpc TransactRelationTuples(TransactRelationTuplesRequest) returns (TransactRelationTuplesResponse);

}

It should behave exactly as the REST handler, so you can probably use the same internal helper function. The current transaction handler is implemented here:

keto/internal/relationtuple/transact_server.go

Lines 30 to 53 in 67c4c60

func (h *handler) TransactRelationTuples(ctx context.Context, req *acl.TransactRelationTuplesRequest) (*acl.TransactRelationTuplesResponse, error) {

insertTuples, err := protoTuplesWithAction(req.RelationTupleDeltas, acl.RelationTupleDelta_INSERT)

if err != nil {

return nil, err

}

deleteTuples, err := protoTuplesWithAction(req.RelationTupleDeltas, acl.RelationTupleDelta_DELETE)

if err != nil {

return nil, err

}

err = h.d.RelationTupleManager().TransactRelationTuples(ctx, insertTuples, deleteTuples)

if err != nil {

return nil, err

}

snaptokens := make([]string, len(insertTuples))

for i := range insertTuples {

snaptokens[i] = "not yet implemented"

}

return &acl.TransactRelationTuplesResponse{

Snaptokens: snaptokens,

}, nil

}

, you can just follow the implementation of that as a guidance.

zepatrik

Getting there 😉

zepatrik · 2021-12-23T14:20:16Z

internal/persistence/sql/relationtuples.go

+		sqlRawQuery := "DELETE FROM keto_relation_tuples WHERE"
+		args := []interface{}{}
+		if query.Namespace != "" {
+			n, err := p.GetNamespaceByName(ctx, query.Namespace)
+			if err != nil {
+				return err
+			}
+			sqlRawQuery += " namespace_id = ?"
+			args = append(args, n.ID)
+		}
+		if query.Object != "" {
+			sqlRawQuery += " AND object = ?"
+			args = append(args, query.Object)
+		}
+		if query.Relation != "" {
+			sqlRawQuery += " AND relation = ?"
+			args = append(args, query.Relation)
+		}


Please use the pop query builder for this instead of raw query. Just as we do here:

keto/internal/persistence/sql/relationtuples.go

Lines 218 to 236 in 7ee65b5

if query.Relation != "" {

sqlQuery.Where("relation = ?", query.Relation)

}

if query.Object != "" {

sqlQuery.Where("object = ?", query.Object)

}

if s := query.Subject(); s != nil {

if err := p.whereSubject(ctx, sqlQuery, s); err != nil {

return nil, "", err

}

}

if query.Namespace != "" {

n, err := p.GetNamespaceByName(ctx, query.Namespace)

if err != nil {

return nil, "", err

}

sqlQuery.Where("namespace_id = ?", n.ID)

}

and here

keto/internal/persistence/sql/relationtuples.go

Lines 186 to 196 in 7ee65b5

q := p.QueryWithNetwork(ctx).

Where("namespace_id = ?", n.ID).

Where("object = ?", r.Object).

Where("relation = ?", r.Relation)

if err := p.whereSubject(ctx, q, r.Subject); err != nil {

return err

}

if err := q.Delete(&RelationTuple{}); err != nil {

return err

}

As is, there is no SQL injection possible, but it is easy to add one later if you don't pay attention.

Ah, my bad. I actually used pop query builder but for some reason, it didn't work. Now I realized why 🤪
I will push a new commit.

vancanhuit · 2021-12-23T14:51:48Z

For the gRPC part, I had a basic understanding of it, now trying to figure out how to do it.

zepatrik · 2022-01-03T13:03:14Z

internal/relationtuple/transact_server.go

@@ -52,6 +52,23 @@ func (h *handler) TransactRelationTuples(ctx context.Context, req *acl.TransactR
 	}, nil
 }

+func (h *handler) DeleteRelationTuples(ctx context.Context, req *acl.DeleteRelationTuplesRequest) (*acl.DeleteRelationTuplesResponse, error) {
+	if req.Query == nil {
+		return nil, errors.New("invalid request")


Always return errors.WithStack(herodot.ErrInvalidRequest.WithReason(...)) or similar errors, they will automatically set the right response codes through the middle-ware.

zepatrik

Now this looks very good now. The very last thing would be to add e2e tests.

here are all cases, add some more that test the new feature: https://github.com/ory/keto/blob/7ee65b5cd0827a3de87be3b6a7fa62d9038ea2df/internal/e2e/cases_test.go

here is the interface definition for the clients, add a deleteAll function and implement it in all clients:

keto/internal/e2e/full_suit_test.go

Lines 33 to 41 in 7ee65b5

    
           client interface { 
        
           	createTuple(t require.TestingT, r *relationtuple.InternalRelationTuple) 
        
           	deleteTuple(t require.TestingT, r *relationtuple.InternalRelationTuple) 
        
           	queryTuple(t require.TestingT, q *relationtuple.RelationQuery, opts ...x.PaginationOptionSetter) *relationtuple.GetResponse 
        
           	queryTupleErr(t require.TestingT, expected herodot.DefaultError, q *relationtuple.RelationQuery, opts ...x.PaginationOptionSetter) 
        
           	check(t require.TestingT, r *relationtuple.InternalRelationTuple) bool 
        
           	expand(t require.TestingT, r *relationtuple.SubjectSet, depth int) *expand.Tree 
        
           	waitUntilLive(t require.TestingT) 
        
           }

zepatrik · 2022-01-03T13:14:10Z

internal/persistence/sql/relationtuples.go

+		if query.Namespace != "" {
+			n, err := p.GetNamespaceByName(ctx, query.Namespace)
+			if err != nil {
+				return err
+			}
+			sqlQuery.Where("namespace_id = ?", n.ID)
+		}
+		if query.Object != "" {
+			sqlQuery.Where("object = ?", query.Object)
+		}
+		if query.Relation != "" {
+			sqlQuery.Where("relation = ?", query.Relation)
+		}
+		if s := query.Subject(); s != nil {
+			if err := p.whereSubject(ctx, sqlQuery, s); err != nil {
+				return err
+			}
+		}


Now this looks way better, but it is the same code as in the GetRelationTuples function.
Can you instead add a helper function similar to this (naming is just a draft):

// whereQuery takes a relation tuple query and appends where clauses to the base pop query func whereQuery(ctx context.Context, baseQuery *pop.Query, rq *relationtuple.RelationQuery) (*pop.Query, error) { if query.Namespace != "" { n, err := p.GetNamespaceByName(ctx, rq.Namespace) if err != nil { return nil, err } baseQuery.Where("namespace_id = ?", n.ID) } if rq.Object != "" { baseQuery.Where("object = ?", rq.Object) } if rq.Relation != "" { baseQuery.Where("relation = ?", rq.Relation) } if s := rq.Subject(); s != nil { if err := p.whereSubject(ctx, baseQuery, s); err != nil { return err } } }

And then of course use it in both functions.

Yeah, I will update it.

zepatrik · 2022-01-03T13:17:06Z

internal/relationtuple/transact_server.go

+		return nil, errors.New("invalid request")
+	}
+
+	q, err := (&RelationQuery{}).FromProto((*acl.ListRelationTuplesRequest_Query)(req.Query))


The generated proto definitions have getter functions, so you can instead rewrite func (q *RelationQuery) FromProto(...) to take an interface with the getter functions. That way you don't have to convert the type which might break in the future at some point.

vancanhuit · 2022-01-04T13:36:36Z

I will add e2e tests later on.

vancanhuit · 2022-01-08T08:11:28Z

@zepatrik Currently I'm getting stuck in the CLI client test. The keto relation-tuple delete command only supports JSON files containing a list of tuples. It is impossible to create a test for it based on a relation query. What should I do?

zepatrik · 2022-01-10T12:55:47Z

I think it would make sense to not break keto relation-tuple delete behavior, but maybe add a flag --all-matching and then flags for each key you could delete by? Similar to how the list command works:

keto/cmd/relationtuple/get.go

Lines 22 to 65 in fabf1a0

    
           const ( 
        
           	FlagSubject    = "subject" 
        
           	FlagSubjectID  = "subject-id" 
        
           	FlagSubjectSet = "subject-set" 
        
           	FlagRelation   = "relation" 
        
           	FlagObject     = "object" 
        
           	FlagPageSize   = "page-size" 
        
           	FlagPageToken  = "page-token" 
        
           ) 
        
           func registerRelationTupleFlags(flags *pflag.FlagSet) { 
        
           	flags.String(FlagSubjectID, "", "Set the requested subject ID") 
        
           	flags.String(FlagSubjectSet, "", `Set the requested subject set; format: "namespace:object#relation"`) 
        
           	flags.String(FlagRelation, "", "Set the requested relation") 
        
           	flags.String(FlagObject, "", "Set the requested object") 
        
           	flags.String(FlagSubject, "", "") 
        
           	if err := flags.MarkHidden(FlagSubject); err != nil { 
        
           		panic(err.Error()) 
        
           	} 
        
           } 
        
           func readQueryFromFlags(cmd *cobra.Command, namespace string) (*acl.ListRelationTuplesRequest_Query, error) { 
        
           	query := &acl.ListRelationTuplesRequest_Query{ 
        
           		Namespace: namespace, 
        
           		Object:    flagx.MustGetString(cmd, FlagObject), 
        
           		Relation:  flagx.MustGetString(cmd, FlagRelation), 
        
           	} 
        
           	switch flags := cmd.Flags(); { 
        
           	case flags.Changed(FlagSubjectID) && flags.Changed(FlagSubjectSet): 
        
           		return nil, relationtuple.ErrDuplicateSubject 
        
           	case flags.Changed(FlagSubjectID): 
        
           		query.Subject = (&relationtuple.SubjectID{ID: flagx.MustGetString(cmd, FlagSubjectID)}).ToProto() 
        
           	case flags.Changed(FlagSubjectSet): 
        
           		s, err := (&relationtuple.SubjectSet{}).FromString(flagx.MustGetString(cmd, FlagSubjectSet)) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		query.Subject = s.ToProto() 
        
           	} 
        
           	return query, nil 
        
           }

vancanhuit · 2022-01-10T13:10:03Z

I think it would make sense to not break keto relation-tuple delete behavior, but maybe add a flag --all-matching and then flags for each key you could delete by? Similar to how the list command works:

keto/cmd/relationtuple/get.go

Lines 22 to 65 in fabf1a0

const (

FlagSubject = "subject"

FlagSubjectID = "subject-id"

FlagSubjectSet = "subject-set"

FlagRelation = "relation"

FlagObject = "object"

FlagPageSize = "page-size"

FlagPageToken = "page-token"

)

func registerRelationTupleFlags(flags *pflag.FlagSet) {

flags.String(FlagSubjectID, "", "Set the requested subject ID")

flags.String(FlagSubjectSet, "", `Set the requested subject set; format: "namespace:object#relation"`)

flags.String(FlagRelation, "", "Set the requested relation")

flags.String(FlagObject, "", "Set the requested object")

flags.String(FlagSubject, "", "")

if err := flags.MarkHidden(FlagSubject); err != nil {

panic(err.Error())

}

}

func readQueryFromFlags(cmd *cobra.Command, namespace string) (*acl.ListRelationTuplesRequest_Query, error) {

query := &acl.ListRelationTuplesRequest_Query{

Namespace: namespace,

Object: flagx.MustGetString(cmd, FlagObject),

Relation: flagx.MustGetString(cmd, FlagRelation),

}

switch flags := cmd.Flags(); {

case flags.Changed(FlagSubjectID) && flags.Changed(FlagSubjectSet):

return nil, relationtuple.ErrDuplicateSubject

case flags.Changed(FlagSubjectID):

query.Subject = (&relationtuple.SubjectID{ID: flagx.MustGetString(cmd, FlagSubjectID)}).ToProto()

case flags.Changed(FlagSubjectSet):

s, err := (&relationtuple.SubjectSet{}).FromString(flagx.MustGetString(cmd, FlagSubjectSet))

if err != nil {

return nil, err

}

query.Subject = s.ToProto()

}

return query, nil

}

Does this mean that we will support two cases: one is deleting from JSON files and one is deleting by specifying query keys?

vancanhuit · 2022-01-15T11:18:01Z

@zepatrik How do you think about my latest change?

zepatrik

Thank you, this looks very good now 👍
I am really sorry for the late review.

zepatrik · 2022-01-21T12:05:43Z

internal/relationtuple/transact_server.go

@@ -152,11 +166,11 @@ func (h *handler) createRelation(w http.ResponseWriter, r *http.Request, _ httpr
 	h.d.Writer().WriteCreated(w, r, RouteBase+"?"+q.Encode(), &rel)
 }

-// swagger:route DELETE /relation-tuples write deleteRelationTuple
+// swagger:route DELETE /relation-tuples write deleteRelationTuples


This will result in breaking changes in the SDK, right @aeneasr? The behavior changed a bit, but nothing that should break existing integrations.

zepatrik · 2022-01-21T12:07:21Z

proto/ory/keto/acl/v1alpha1/write_service.proto

+  // Deletes relation tuples based on relation query
+  rpc DeleteRelationTuples(DeleteRelationTuplesRequest) returns (DeleteRelationTuplesResponse);


FYI, as this is a new rpc it will not break existing integrations.

Closes #599

zepatrik · 2022-01-21T13:05:51Z

I actually just realized that splitting the delete and delete-all into two subcommands makes more sense, as it now reflects the two behaviors and will not break existing behavior. Also, with this it should be harder to delete stuff by mistake. Thought it is easier if I just quickly do that instead of annoying you again 😅
Thanks again for your work there, appreciate 👍

vancanhuit requested a review from zepatrik as a code owner December 18, 2021 08:28

zepatrik requested changes Dec 20, 2021

View reviewed changes

zepatrik requested changes Dec 23, 2021

View reviewed changes

zepatrik reviewed Jan 3, 2022

View reviewed changes

zepatrik approved these changes Jan 21, 2022

View reviewed changes

vancanhuit and others added 12 commits January 21, 2022 14:01

feat: bulk deletion of relation tuples

5cbdd40

Closes #599

Rename handler function name

96d717f

Update test

8655e8d

Update swagger comment for deleteRelations handler

532d21d

Update swagger comment for deleteRelations handler

6c35e90

Delete all relation tuples based on relation query

73696ca

Delete all relation tuples based on relation query

7f9fe84

Add DeleteRelationTuples for gRPC

457e94a

Update returned error values

7984f3f

Refactor

9f97367

add query keys for keto relation-tuple delete command

55dab10

feat: keto relation-tuple delete-all command

ff079e2

fix: code samples

2d4ce93

zepatrik merged commit c1e8546 into ory:master Jan 21, 2022

zepatrik mentioned this pull request Jan 21, 2022

fix: openapi spec and internal SDK #819

Merged

vancanhuit deleted the feat/bulk-deletion-relation-tuples branch January 23, 2022 03:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bulk deletion of relation tuples #799

feat: bulk deletion of relation tuples #799

vancanhuit commented Dec 18, 2021

zepatrik left a comment

zepatrik Dec 20, 2021

vancanhuit Dec 20, 2021

zepatrik Dec 20, 2021

zepatrik left a comment

zepatrik Dec 23, 2021

vancanhuit Dec 23, 2021

vancanhuit commented Dec 23, 2021 •

edited

Loading

zepatrik Jan 3, 2022

zepatrik left a comment

zepatrik Jan 3, 2022

zepatrik Jan 3, 2022

vancanhuit Jan 3, 2022

zepatrik Jan 3, 2022

vancanhuit commented Jan 4, 2022

vancanhuit commented Jan 8, 2022 •

edited

Loading

zepatrik commented Jan 10, 2022

vancanhuit commented Jan 10, 2022

vancanhuit commented Jan 15, 2022

zepatrik left a comment

zepatrik Jan 21, 2022

zepatrik Jan 21, 2022

zepatrik commented Jan 21, 2022

	// The write service to create and delete Access Control Lists.
	//
	// This service is part of the [write-APIs](../concepts/api-overview.mdx#write-apis).
	service WriteService {
	// Writes one or more relation tuples in a single transaction.
	rpc TransactRelationTuples(TransactRelationTuplesRequest) returns (TransactRelationTuplesResponse);
	}

	func (h handler) TransactRelationTuples(ctx context.Context, req acl.TransactRelationTuplesRequest) (*acl.TransactRelationTuplesResponse, error) {
	insertTuples, err := protoTuplesWithAction(req.RelationTupleDeltas, acl.RelationTupleDelta_INSERT)
	if err != nil {
	return nil, err
	}

	deleteTuples, err := protoTuplesWithAction(req.RelationTupleDeltas, acl.RelationTupleDelta_DELETE)
	if err != nil {
	return nil, err
	}

	err = h.d.RelationTupleManager().TransactRelationTuples(ctx, insertTuples, deleteTuples)
	if err != nil {
	return nil, err
	}

	snaptokens := make([]string, len(insertTuples))
	for i := range insertTuples {
	snaptokens[i] = "not yet implemented"
	}
	return &acl.TransactRelationTuplesResponse{
	Snaptokens: snaptokens,
	}, nil
	}

	if query.Relation != "" {
	sqlQuery.Where("relation = ?", query.Relation)
	}
	if query.Object != "" {
	sqlQuery.Where("object = ?", query.Object)
	}
	if s := query.Subject(); s != nil {
	if err := p.whereSubject(ctx, sqlQuery, s); err != nil {
	return nil, "", err
	}
	}

	if query.Namespace != "" {
	n, err := p.GetNamespaceByName(ctx, query.Namespace)
	if err != nil {
	return nil, "", err
	}
	sqlQuery.Where("namespace_id = ?", n.ID)
	}

	q := p.QueryWithNetwork(ctx).
	Where("namespace_id = ?", n.ID).
	Where("object = ?", r.Object).
	Where("relation = ?", r.Relation)
	if err := p.whereSubject(ctx, q, r.Subject); err != nil {
	return err
	}

	if err := q.Delete(&RelationTuple{}); err != nil {
	return err
	}

	client interface {
	createTuple(t require.TestingT, r *relationtuple.InternalRelationTuple)
	deleteTuple(t require.TestingT, r *relationtuple.InternalRelationTuple)
	queryTuple(t require.TestingT, q relationtuple.RelationQuery, opts ...x.PaginationOptionSetter) relationtuple.GetResponse
	queryTupleErr(t require.TestingT, expected herodot.DefaultError, q *relationtuple.RelationQuery, opts ...x.PaginationOptionSetter)
	check(t require.TestingT, r *relationtuple.InternalRelationTuple) bool
	expand(t require.TestingT, r relationtuple.SubjectSet, depth int) expand.Tree
	waitUntilLive(t require.TestingT)
	}

		// Deletes relation tuples based on relation query
		rpc DeleteRelationTuples(DeleteRelationTuplesRequest) returns (DeleteRelationTuplesResponse);

feat: bulk deletion of relation tuples #799

feat: bulk deletion of relation tuples #799

Conversation

vancanhuit commented Dec 18, 2021

Related issue(s)

Checklist

Further Comments

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancanhuit commented Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancanhuit commented Jan 4, 2022

vancanhuit commented Jan 8, 2022 • edited Loading

zepatrik commented Jan 10, 2022

vancanhuit commented Jan 10, 2022

vancanhuit commented Jan 15, 2022

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zepatrik commented Jan 21, 2022

vancanhuit commented Dec 23, 2021 •

edited

Loading

vancanhuit commented Jan 8, 2022 •

edited

Loading