From 6bf2928de87f1dab83444b552915a8018afcb86c Mon Sep 17 00:00:00 2001 From: Yann D'Isanto Date: Wed, 17 Jan 2024 16:14:00 +0100 Subject: [PATCH 1/3] feat(tuple import): csv optional fields and support any columns order --- .../tuples_missing_required_headers.csv | 2 +- .../testdata/tuples_other_columns_order.csv | 4 + .../tuples_without_optional_fields.csv | 3 + cmd/tuple/write.go | 136 +++++++++++------- cmd/tuple/write_test.go | 49 ++++++- 5 files changed, 138 insertions(+), 56 deletions(-) create mode 100644 cmd/tuple/testdata/tuples_other_columns_order.csv create mode 100644 cmd/tuple/testdata/tuples_without_optional_fields.csv diff --git a/cmd/tuple/testdata/tuples_missing_required_headers.csv b/cmd/tuple/testdata/tuples_missing_required_headers.csv index e9d29eb..6db4e17 100644 --- a/cmd/tuple/testdata/tuples_missing_required_headers.csv +++ b/cmd/tuple/testdata/tuples_missing_required_headers.csv @@ -1 +1 @@ -user_type,user_id,user_relation,relation,object_type,object_identifier,condition_name,condition_context +user_type,user_id,user_relation,relation,object_type,condition_name,condition_context diff --git a/cmd/tuple/testdata/tuples_other_columns_order.csv b/cmd/tuple/testdata/tuples_other_columns_order.csv new file mode 100644 index 0000000..2849fdb --- /dev/null +++ b/cmd/tuple/testdata/tuples_other_columns_order.csv @@ -0,0 +1,4 @@ +user_id,user_type,user_relation,object_id,object_type,relation,condition_name,condition_context +anne,user,,product,folder,owner,inOfficeIP, +product,folder,,product-2021,folder,parent,inOfficeIP,"{""ip_addr"":""10.0.0.1""}" +fga,team,member,product-2021,folder,viewer,, diff --git a/cmd/tuple/testdata/tuples_without_optional_fields.csv b/cmd/tuple/testdata/tuples_without_optional_fields.csv new file mode 100644 index 0000000..fa6ae05 --- /dev/null +++ b/cmd/tuple/testdata/tuples_without_optional_fields.csv @@ -0,0 +1,3 @@ +user_type,user_id,relation,object_type,object_id +user,anne,owner,folder,product +folder,product,parent,folder,product-2021 diff --git a/cmd/tuple/write.go b/cmd/tuple/write.go index 234600e..6bf437d 100644 --- a/cmd/tuple/write.go +++ b/cmd/tuple/write.go @@ -179,58 +179,42 @@ func parseTuplesFileData(fileName string) ([]client.ClientTupleKey, error) { func parseTuplesFromCSV(data []byte, tuples *[]client.ClientTupleKey) error { reader := csv.NewReader(bytes.NewReader(data)) + columns, err := readHeaders(reader) + if err != nil { + return err + } for index := 0; true; index++ { - if index == 0 { - if err := guardAgainstInvalidHeaderWithinCSV(reader); err != nil { - return err - } - - continue - } - tuple, err := reader.Read() if err != nil { if errors.Is(err, io.EOF) { break } - return fmt.Errorf("failed to read tuple from csv file: %w", err) } - const ( - UserType = iota - UserID - UserRelation - Relation - ObjectType - ObjectID - ConditionName - ConditionContext - ) - - tupleUserKey := tuple[UserType] + ":" + tuple[UserID] - if tuple[UserRelation] != "" { - tupleUserKey += "#" + tuple[UserRelation] + tupleUserKey := tuple[columns.UserType] + ":" + tuple[columns.UserID] + if columns.UserRelation != -1 && tuple[columns.UserRelation] != "" { + tupleUserKey += "#" + tuple[columns.UserRelation] } var condition *openfga.RelationshipCondition - if tuple[ConditionName] != "" { - conditionContext, err := cmdutils.ParseQueryContextInner(tuple[ConditionContext]) + if columns.ConditionName != -1 && tuple[columns.ConditionName] != "" { + conditionContext, err := cmdutils.ParseQueryContextInner(tuple[columns.ConditionContext]) if err != nil { return fmt.Errorf("failed to read condition context on line %d: %w", index, err) } condition = &openfga.RelationshipCondition{ - Name: tuple[ConditionName], + Name: tuple[columns.ConditionName], Context: conditionContext, } } tupleKey := client.ClientTupleKey{ User: tupleUserKey, - Relation: tuple[Relation], - Object: tuple[ObjectType] + ":" + tuple[ObjectID], + Relation: tuple[columns.Relation], + Object: tuple[columns.ObjectType] + ":" + tuple[columns.ObjectID], Condition: condition, } @@ -240,42 +224,86 @@ func parseTuplesFromCSV(data []byte, tuples *[]client.ClientTupleKey) error { return nil } -func guardAgainstInvalidHeaderWithinCSV(reader *csv.Reader) error { - headers, err := reader.Read() - if err != nil { - return fmt.Errorf("failed to read csv headers: %w", err) - } +type csvColumns struct { + UserType int + UserID int + UserRelation int + Relation int + ObjectType int + ObjectID int + ConditionName int + ConditionContext int +} - headerMap := make(map[string]bool) - for _, header := range headers { - headerMap[strings.TrimSpace(header)] = true +func (columns *csvColumns) setHeaderIndex(headerName string, index int) error { + switch headerName { + case "user_type": + columns.UserType = index + case "user_id": + columns.UserID = index + case "user_relation": + columns.UserRelation = index + case "relation": + columns.Relation = index + case "object_type": + columns.ObjectType = index + case "object_id": + columns.ObjectID = index + case "condition_name": + columns.ConditionName = index + case "condition_context": + columns.ConditionContext = index + default: + return fmt.Errorf("invalid header %q, valid headers are user_type,user_id,user_relation,relation,object_type,object_id,condition_name,condition_context", headerName) } + return nil +} - requiredHeaders := []string{ - "user_type", - "user_id", - "user_relation", - "relation", - "object_type", - "object_id", - "condition_name", - "condition_context", +func (columns *csvColumns) validate() error { + if columns.UserType == -1 { + return errors.New("required csv header \"user_type\" not found") + } + if columns.UserID == -1 { + return errors.New("required csv header \"user_id\" not found") + } + if columns.Relation == -1 { + return errors.New("required csv header \"relation\" not found") + } + if columns.ObjectType == -1 { + return errors.New("required csv header \"object_type\" not found") + } + if columns.ObjectID == -1 { + return errors.New("required csv header \"object_id\" not found") } - if len(headerMap) != len(requiredHeaders) { - return fmt.Errorf( //nolint:goerr113 - "csv file must have exactly these headers in order: %q", - strings.Join(requiredHeaders, ","), - ) + // TODO: can't have only one of ConditionName and ConditionContext (both or none) + return nil +} + +func readHeaders(reader *csv.Reader) (*csvColumns, error) { + headers, err := reader.Read() + if err != nil { + return nil, fmt.Errorf("failed to read csv headers: %w", err) } - for _, header := range requiredHeaders { - if _, ok := headerMap[header]; !ok { - return fmt.Errorf("required csv header %q not found", header) //nolint:goerr113 + columns := &csvColumns{ + UserType: -1, + UserID: -1, + UserRelation: -1, + Relation: -1, + ObjectType: -1, + ObjectID: -1, + ConditionName: -1, + ConditionContext: -1, + } + for index, header := range headers { + err = columns.setHeaderIndex(strings.TrimSpace(header), index) + if err != nil { + return nil, err } } - return nil + return columns, columns.validate() } func init() { diff --git a/cmd/tuple/write_test.go b/cmd/tuple/write_test.go index 2595180..1716af0 100644 --- a/cmd/tuple/write_test.go +++ b/cmd/tuple/write_test.go @@ -49,6 +49,53 @@ func TestParseTuplesFileData(t *testing.T) { //nolint:funlen }, }, }, + { + name: "it can correctly parse a csv file regardless of columns order", + file: "testdata/tuples_other_columns_order.csv", + expectedTuples: []client.ClientTupleKey{ + { + User: "user:anne", + Relation: "owner", + Object: "folder:product", + Condition: &openfga.RelationshipCondition{ + Name: "inOfficeIP", + Context: &map[string]interface{}{}, + }, + }, + { + User: "folder:product", + Relation: "parent", + Object: "folder:product-2021", + Condition: &openfga.RelationshipCondition{ + Name: "inOfficeIP", + Context: &map[string]interface{}{ + "ip_addr": "10.0.0.1", + }, + }, + }, + { + User: "team:fga#member", + Relation: "viewer", + Object: "folder:product-2021", + }, + }, + }, + { + name: "it can correctly parse a csv file without optional fields", + file: "testdata/tuples_without_optional_fields.csv", + expectedTuples: []client.ClientTupleKey{ + { + User: "user:anne", + Relation: "owner", + Object: "folder:product", + }, + { + User: "folder:product", + Relation: "parent", + Object: "folder:product-2021", + }, + }, + }, { name: "it can correctly parse a json file", file: "testdata/tuples.json", @@ -104,7 +151,7 @@ func TestParseTuplesFileData(t *testing.T) { //nolint:funlen { name: "it fails to parse a csv file with wrong headers", file: "testdata/tuples_wrong_headers.csv", - expectedError: "failed to parse input tuples: csv file must have exactly these headers in order: \"user_type,user_id,user_relation,relation,object_type,object_id,condition_name,condition_context\"", + expectedError: "failed to parse input tuples: invalid header \"a\", valid headers are user_type,user_id,user_relation,relation,object_type,object_id,condition_name,condition_context", }, { name: "it fails to parse a csv file with missing required headers", From bfb718abfa68e878930997d6862e0b794581ed1b Mon Sep 17 00:00:00 2001 From: Yann D'Isanto Date: Wed, 17 Jan 2024 17:55:36 +0100 Subject: [PATCH 2/3] feat: make condition_name required when condition_context is present --- .../tuples_missing_condition_name_header.csv | 1 + ...ondition_name_but_no_condition_context.csv | 4 +++ cmd/tuple/write.go | 14 +++++--- cmd/tuple/write_test.go | 34 +++++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 cmd/tuple/testdata/tuples_missing_condition_name_header.csv create mode 100644 cmd/tuple/testdata/tuples_with_condition_name_but_no_condition_context.csv diff --git a/cmd/tuple/testdata/tuples_missing_condition_name_header.csv b/cmd/tuple/testdata/tuples_missing_condition_name_header.csv new file mode 100644 index 0000000..a8befd9 --- /dev/null +++ b/cmd/tuple/testdata/tuples_missing_condition_name_header.csv @@ -0,0 +1 @@ +user_type,user_id,relation,object_type,object_id,condition_context \ No newline at end of file diff --git a/cmd/tuple/testdata/tuples_with_condition_name_but_no_condition_context.csv b/cmd/tuple/testdata/tuples_with_condition_name_but_no_condition_context.csv new file mode 100644 index 0000000..5368c41 --- /dev/null +++ b/cmd/tuple/testdata/tuples_with_condition_name_but_no_condition_context.csv @@ -0,0 +1,4 @@ +user_type,user_id,user_relation,relation,object_type,object_id,condition_name +user,anne,,owner,folder,product,inOfficeIP +folder,product,,parent,folder,product-2021,inOfficeIP +team,fga,member,viewer,folder,product-2021, diff --git a/cmd/tuple/write.go b/cmd/tuple/write.go index 6bf437d..7c368e1 100644 --- a/cmd/tuple/write.go +++ b/cmd/tuple/write.go @@ -200,9 +200,13 @@ func parseTuplesFromCSV(data []byte, tuples *[]client.ClientTupleKey) error { var condition *openfga.RelationshipCondition if columns.ConditionName != -1 && tuple[columns.ConditionName] != "" { - conditionContext, err := cmdutils.ParseQueryContextInner(tuple[columns.ConditionContext]) - if err != nil { - return fmt.Errorf("failed to read condition context on line %d: %w", index, err) + conditionContext := &(map[string]interface{}{}) + if columns.ConditionContext != -1 { + conditionContext, err = cmdutils.ParseQueryContextInner(tuple[columns.ConditionContext]) + if err != nil { + return fmt.Errorf("failed to read condition context on line %d: %w", index, err) + } + } condition = &openfga.RelationshipCondition{ @@ -276,7 +280,9 @@ func (columns *csvColumns) validate() error { return errors.New("required csv header \"object_id\" not found") } - // TODO: can't have only one of ConditionName and ConditionContext (both or none) + if columns.ConditionContext != -1 && columns.ConditionName == -1 { + return errors.New("missing \"condition_name\" header which is required when \"condition_context\" is present") + } return nil } diff --git a/cmd/tuple/write_test.go b/cmd/tuple/write_test.go index 1716af0..89883cc 100644 --- a/cmd/tuple/write_test.go +++ b/cmd/tuple/write_test.go @@ -96,6 +96,35 @@ func TestParseTuplesFileData(t *testing.T) { //nolint:funlen }, }, }, + { + name: "it can correctly parse a csv file with condition_name header but no condition_context header", + file: "testdata/tuples_with_condition_name_but_no_condition_context.csv", + expectedTuples: []client.ClientTupleKey{ + { + User: "user:anne", + Relation: "owner", + Object: "folder:product", + Condition: &openfga.RelationshipCondition{ + Name: "inOfficeIP", + Context: &map[string]interface{}{}, + }, + }, + { + User: "folder:product", + Relation: "parent", + Object: "folder:product-2021", + Condition: &openfga.RelationshipCondition{ + Name: "inOfficeIP", + Context: &map[string]interface{}{}, + }, + }, + { + User: "team:fga#member", + Relation: "viewer", + Object: "folder:product-2021", + }, + }, + }, { name: "it can correctly parse a json file", file: "testdata/tuples.json", @@ -158,6 +187,11 @@ func TestParseTuplesFileData(t *testing.T) { //nolint:funlen file: "testdata/tuples_missing_required_headers.csv", expectedError: "failed to parse input tuples: required csv header \"object_id\" not found", }, + { + name: "it fails to parse a csv file with missing condition_name header when condition_context is present", + file: "testdata/tuples_missing_condition_name_header.csv", + expectedError: "failed to parse input tuples: missing \"condition_name\" header which is required when \"condition_context\" is present", + }, { name: "it fails to parse an empty csv file", file: "testdata/tuples_empty.csv", From d63889a7e90c1190963e32027c7799ac28626ae6 Mon Sep 17 00:00:00 2001 From: Raghd Hamzeh Date: Mon, 22 Jan 2024 18:05:26 -0500 Subject: [PATCH 3/3] chore(lint): fix lint issues --- cmd/tuple/write.go | 67 ++++++++++++++++++++++----------- cmd/tuple/write_test.go | 2 +- internal/clierrors/clierrors.go | 5 +++ 3 files changed, 50 insertions(+), 24 deletions(-) diff --git a/cmd/tuple/write.go b/cmd/tuple/write.go index 7c368e1..a9b241f 100644 --- a/cmd/tuple/write.go +++ b/cmd/tuple/write.go @@ -27,6 +27,8 @@ import ( "path" "strings" + "github.com/openfga/cli/internal/clierrors" + openfga "github.com/openfga/go-sdk" "github.com/openfga/go-sdk/client" "github.com/spf13/cobra" @@ -183,12 +185,14 @@ func parseTuplesFromCSV(data []byte, tuples *[]client.ClientTupleKey) error { if err != nil { return err } + for index := 0; true; index++ { tuple, err := reader.Read() if err != nil { if errors.Is(err, io.EOF) { break } + return fmt.Errorf("failed to read tuple from csv file: %w", err) } @@ -197,22 +201,9 @@ func parseTuplesFromCSV(data []byte, tuples *[]client.ClientTupleKey) error { tupleUserKey += "#" + tuple[columns.UserRelation] } - var condition *openfga.RelationshipCondition - - if columns.ConditionName != -1 && tuple[columns.ConditionName] != "" { - conditionContext := &(map[string]interface{}{}) - if columns.ConditionContext != -1 { - conditionContext, err = cmdutils.ParseQueryContextInner(tuple[columns.ConditionContext]) - if err != nil { - return fmt.Errorf("failed to read condition context on line %d: %w", index, err) - } - - } - - condition = &openfga.RelationshipCondition{ - Name: tuple[columns.ConditionName], - Context: conditionContext, - } + condition, err := parseConditionColumnsForRow(columns, tuple, index) + if err != nil { + return err } tupleKey := client.ClientTupleKey{ @@ -228,6 +219,30 @@ func parseTuplesFromCSV(data []byte, tuples *[]client.ClientTupleKey) error { return nil } +func parseConditionColumnsForRow(columns *csvColumns, tuple []string, index int) (*openfga.RelationshipCondition, error) { + var condition *openfga.RelationshipCondition + + if columns.ConditionName != -1 && tuple[columns.ConditionName] != "" { + conditionContext := &(map[string]interface{}{}) + + if columns.ConditionContext != -1 { + var err error + + conditionContext, err = cmdutils.ParseQueryContextInner(tuple[columns.ConditionContext]) + if err != nil { + return nil, fmt.Errorf("failed to read condition context on line %d: %w", index, err) + } + } + + condition = &openfga.RelationshipCondition{ + Name: tuple[columns.ConditionName], + Context: conditionContext, + } + } + + return condition, nil +} + type csvColumns struct { UserType int UserID int @@ -258,31 +273,37 @@ func (columns *csvColumns) setHeaderIndex(headerName string, index int) error { case "condition_context": columns.ConditionContext = index default: - return fmt.Errorf("invalid header %q, valid headers are user_type,user_id,user_relation,relation,object_type,object_id,condition_name,condition_context", headerName) + return fmt.Errorf("invalid header %q, valid headers are user_type,user_id,user_relation,relation,object_type,object_id,condition_name,condition_context", headerName) //nolint:goerr113 } + return nil } func (columns *csvColumns) validate() error { if columns.UserType == -1 { - return errors.New("required csv header \"user_type\" not found") + return clierrors.MissingRequiredCsvHeaderError("user_type") //nolint:wrapcheck } + if columns.UserID == -1 { - return errors.New("required csv header \"user_id\" not found") + return clierrors.MissingRequiredCsvHeaderError("user_id") //nolint:wrapcheck } + if columns.Relation == -1 { - return errors.New("required csv header \"relation\" not found") + return clierrors.MissingRequiredCsvHeaderError("relation") //nolint:wrapcheck } + if columns.ObjectType == -1 { - return errors.New("required csv header \"object_type\" not found") + return clierrors.MissingRequiredCsvHeaderError("object_type") //nolint:wrapcheck } + if columns.ObjectID == -1 { - return errors.New("required csv header \"object_id\" not found") + return clierrors.MissingRequiredCsvHeaderError("object_id") //nolint:wrapcheck } if columns.ConditionContext != -1 && columns.ConditionName == -1 { - return errors.New("missing \"condition_name\" header which is required when \"condition_context\" is present") + return errors.New("missing \"condition_name\" header which is required when \"condition_context\" is present") //nolint:goerr113 } + return nil } diff --git a/cmd/tuple/write_test.go b/cmd/tuple/write_test.go index 89883cc..fa340b1 100644 --- a/cmd/tuple/write_test.go +++ b/cmd/tuple/write_test.go @@ -185,7 +185,7 @@ func TestParseTuplesFileData(t *testing.T) { //nolint:funlen { name: "it fails to parse a csv file with missing required headers", file: "testdata/tuples_missing_required_headers.csv", - expectedError: "failed to parse input tuples: required csv header \"object_id\" not found", + expectedError: "failed to parse input tuples: csv header missing (\"object_id\")", }, { name: "it fails to parse a csv file with missing condition_name header when condition_context is present", diff --git a/internal/clierrors/clierrors.go b/internal/clierrors/clierrors.go index a9d52fc..9b90ccd 100644 --- a/internal/clierrors/clierrors.go +++ b/internal/clierrors/clierrors.go @@ -28,8 +28,13 @@ var ( ErrStoreNotFound = errors.New("store not found") ErrAuthorizationModelNotFound = errors.New("authorization model not found") ErrModelInputMissing = errors.New("model input not provided") + ErrRequiredCsvHeaderMissing = errors.New("csv header missing") ) func ValidationError(op string, details string) error { return fmt.Errorf("%w - %s: %s", ErrValidation, op, details) } + +func MissingRequiredCsvHeaderError(headerName string) error { + return fmt.Errorf("%w (\"%s\")", ErrRequiredCsvHeaderMissing, headerName) +}