From a3b589200fc96ac5f1446d287ff0446bebf709e4 Mon Sep 17 00:00:00 2001 From: Nitin Mohan Date: Mon, 23 Mar 2020 17:30:50 -0700 Subject: [PATCH 1/3] Fix cyclic definition for CollectionOf --- codegen/service/testdata/views_code.go | 165 +++++++++++++++++++++++++ codegen/service/testdata/views_dsls.go | 32 +++++ codegen/service/views_test.go | 1 + dsl/result_type.go | 62 ++++++---- 4 files changed, 239 insertions(+), 21 deletions(-) diff --git a/codegen/service/testdata/views_code.go b/codegen/service/testdata/views_code.go index d2496fd277..27ad51ccf9 100644 --- a/codegen/service/testdata/views_code.go +++ b/codegen/service/testdata/views_code.go @@ -547,3 +547,168 @@ func ValidateRTViewTiny(result *RTView) (err error) { return } ` + +const ResultWithRecursiveCollectionOfResultTypeCode = `// SomeRT is the viewed result type that is projected based on a view. +type SomeRT struct { + // Type to project + Projected *SomeRTView + // View to render + View string +} + +// AnotherResult is the viewed result type that is projected based on a view. +type AnotherResult struct { + // Type to project + Projected *AnotherResultView + // View to render + View string +} + +// SomeRTView is a type that runs validations on a projected type. +type SomeRTView struct { + A SomeRTCollectionView +} + +// SomeRTCollectionView is a type that runs validations on a projected type. +type SomeRTCollectionView []*SomeRTView + +// AnotherResultView is a type that runs validations on a projected type. +type AnotherResultView struct { + A AnotherResultCollectionView +} + +// AnotherResultCollectionView is a type that runs validations on a projected +// type. +type AnotherResultCollectionView []*AnotherResultView + +var ( + // SomeRTMap is a map of attribute names in result type SomeRT indexed by view + // name. + SomeRTMap = map[string][]string{ + "default": []string{ + "a", + }, + "tiny": []string{ + "a", + }, + } + // AnotherResultMap is a map of attribute names in result type AnotherResult + // indexed by view name. + AnotherResultMap = map[string][]string{ + "default": []string{ + "a", + }, + } + // SomeRTCollectionMap is a map of attribute names in result type + // SomeRTCollection indexed by view name. + SomeRTCollectionMap = map[string][]string{ + "default": []string{ + "a", + }, + "tiny": []string{ + "a", + }, + } + // AnotherResultCollectionMap is a map of attribute names in result type + // AnotherResultCollection indexed by view name. + AnotherResultCollectionMap = map[string][]string{ + "default": []string{ + "a", + }, + } +) + +// ValidateSomeRT runs the validations defined on the viewed result type SomeRT. +func ValidateSomeRT(result *SomeRT) (err error) { + switch result.View { + case "default", "": + err = ValidateSomeRTView(result.Projected) + case "tiny": + err = ValidateSomeRTViewTiny(result.Projected) + default: + err = goa.InvalidEnumValueError("view", result.View, []interface{}{"default", "tiny"}) + } + return +} + +// ValidateAnotherResult runs the validations defined on the viewed result type +// AnotherResult. +func ValidateAnotherResult(result *AnotherResult) (err error) { + switch result.View { + case "default", "": + err = ValidateAnotherResultView(result.Projected) + default: + err = goa.InvalidEnumValueError("view", result.View, []interface{}{"default"}) + } + return +} + +// ValidateSomeRTView runs the validations defined on SomeRTView using the +// "default" view. +func ValidateSomeRTView(result *SomeRTView) (err error) { + + if result.A != nil { + if err2 := ValidateSomeRTCollectionViewTiny(result.A); err2 != nil { + err = goa.MergeErrors(err, err2) + } + } + return +} + +// ValidateSomeRTViewTiny runs the validations defined on SomeRTView using the +// "tiny" view. +func ValidateSomeRTViewTiny(result *SomeRTView) (err error) { + + if result.A != nil { + if err2 := ValidateSomeRTCollectionView(result.A); err2 != nil { + err = goa.MergeErrors(err, err2) + } + } + return +} + +// ValidateSomeRTCollectionView runs the validations defined on +// SomeRTCollectionView using the "default" view. +func ValidateSomeRTCollectionView(result SomeRTCollectionView) (err error) { + for _, item := range result { + if err2 := ValidateSomeRTView(item); err2 != nil { + err = goa.MergeErrors(err, err2) + } + } + return +} + +// ValidateSomeRTCollectionViewTiny runs the validations defined on +// SomeRTCollectionView using the "tiny" view. +func ValidateSomeRTCollectionViewTiny(result SomeRTCollectionView) (err error) { + for _, item := range result { + if err2 := ValidateSomeRTViewTiny(item); err2 != nil { + err = goa.MergeErrors(err, err2) + } + } + return +} + +// ValidateAnotherResultView runs the validations defined on AnotherResultView +// using the "default" view. +func ValidateAnotherResultView(result *AnotherResultView) (err error) { + + if result.A != nil { + if err2 := ValidateAnotherResultCollectionView(result.A); err2 != nil { + err = goa.MergeErrors(err, err2) + } + } + return +} + +// ValidateAnotherResultCollectionView runs the validations defined on +// AnotherResultCollectionView using the "default" view. +func ValidateAnotherResultCollectionView(result AnotherResultCollectionView) (err error) { + for _, item := range result { + if err2 := ValidateAnotherResultView(item); err2 != nil { + err = goa.MergeErrors(err, err2) + } + } + return +} +` diff --git a/codegen/service/testdata/views_dsls.go b/codegen/service/testdata/views_dsls.go index fb31f611a7..475eb5002e 100644 --- a/codegen/service/testdata/views_dsls.go +++ b/codegen/service/testdata/views_dsls.go @@ -169,6 +169,38 @@ var ResultWithRecursiveResultTypeDSL = func() { }) } +var ResultWithRecursiveCollectionOfResultTypeDSL = func() { + var SomeRT = ResultType("application/vnd.some_result", func() { + TypeName("SomeRT") + Attributes(func() { + Attribute("a", CollectionOf("SomeRT")) + Required("a") + }) + View("default", func() { + Attribute("a", func() { + View("tiny") + }) + }) + View("tiny", func() { + Attribute("a") + }) + }) + var AnotherRT = ResultType("application/vnd.another_result", func() { + Attributes(func() { + Attribute("a", CollectionOf("application/vnd.another_result")) + Required("a") + }) + }) + Service("ResultWithRecursiveCollectionOfResultType", func() { + Method("A", func() { + Result(SomeRT) + }) + Method("B", func() { + Result(AnotherRT) + }) + }) +} + var ResultWithCustomFieldsDSL = func() { var RT = ResultType("application/vnd.result", func() { TypeName("RT") diff --git a/codegen/service/views_test.go b/codegen/service/views_test.go index caa5ebd3aa..bd3fa876c8 100644 --- a/codegen/service/views_test.go +++ b/codegen/service/views_test.go @@ -23,6 +23,7 @@ func TestViews(t *testing.T) { {"result-with-result-type", testdata.ResultWithResultTypeDSL, testdata.ResultWithResultTypeCode}, {"result-with-recursive-result-type", testdata.ResultWithRecursiveResultTypeDSL, testdata.ResultWithRecursiveResultTypeCode}, {"result-type-with-custom-fields", testdata.ResultWithCustomFieldsDSL, testdata.ResultWithCustomFieldsCode}, + {"result-with-recursive-collection-of-result-type", testdata.ResultWithRecursiveCollectionOfResultTypeDSL, testdata.ResultWithRecursiveCollectionOfResultTypeCode}, } for _, c := range cases { t.Run(c.Name, func(t *testing.T) { diff --git a/dsl/result_type.go b/dsl/result_type.go index e52e493c67..bd9e497c8c 100644 --- a/dsl/result_type.go +++ b/dsl/result_type.go @@ -87,27 +87,8 @@ func ResultType(identifier string, fn func()) *expr.ResultTypeExpr { } } identifier = mime.FormatMediaType(identifier, params) - lastPart := identifier - lastPartIndex := strings.LastIndex(identifier, "/") - if lastPartIndex > -1 { - lastPart = identifier[lastPartIndex+1:] - } - plusIndex := strings.Index(lastPart, "+") - if plusIndex > 0 { - lastPart = lastPart[:plusIndex] - } - lastPart = strings.TrimPrefix(lastPart, "vnd.") - elems := strings.Split(lastPart, ".") - for i, e := range elems { - elems[i] = strings.Title(e) - } - typeName := strings.Join(elems, "") - if typeName == "" { - resultTypeCount++ - typeName = fmt.Sprintf("ResultType%d", resultTypeCount) - } // Now save the type in the API result types map - mt := expr.NewResultTypeExpr(typeName, identifier, fn) + mt := expr.NewResultTypeExpr(buildTypeName(identifier), identifier, fn) expr.Root.ResultTypes = append(expr.Root.ResultTypes, mt) return mt @@ -282,10 +263,24 @@ func CollectionOf(v interface{}, adsl ...func()) *expr.ResultTypeExpr { m, ok = v.(*expr.ResultTypeExpr) if !ok { if id, ok := v.(string); ok { - if dt := expr.Root.UserType(expr.CanonicalIdentifier(id)); dt != nil { + // Check if a result type exists with the given type name + if dt := expr.Root.UserType(id); dt != nil { if mt, ok := dt.(*expr.ResultTypeExpr); ok { m = mt } + } else { + // Check if a result type exists with the given identifier + id, params, err := mime.ParseMediaType(id) + if err != nil { + eval.ReportError("invalid result type identifier %#v: %s", id, err) + return nil + } + id = mime.FormatMediaType(id, params) + if dt := expr.Root.UserType(buildTypeName(id)); dt != nil { + if mt, ok := dt.(*expr.ResultTypeExpr); ok { + m = mt + } + } } } } @@ -447,6 +442,31 @@ func Attributes(fn func()) { eval.Execute(fn, mt) } +// buildTypeName builds the result type name from the formatted identifier and +// returns it. +func buildTypeName(identifier string) string { + lastPart := identifier + lastPartIndex := strings.LastIndex(identifier, "/") + if lastPartIndex > -1 { + lastPart = identifier[lastPartIndex+1:] + } + plusIndex := strings.Index(lastPart, "+") + if plusIndex > 0 { + lastPart = lastPart[:plusIndex] + } + lastPart = strings.TrimPrefix(lastPart, "vnd.") + elems := strings.Split(lastPart, ".") + for i, e := range elems { + elems[i] = strings.Title(e) + } + typeName := strings.Join(elems, "") + if typeName == "" { + resultTypeCount++ + typeName = fmt.Sprintf("ResultType%d", resultTypeCount) + } + return typeName +} + // buildView builds a view expression given an attribute and a corresponding // result type. The attribute must be an object listing the child attributes // that make up the view. From 4319f5f27d4ece14e3a45d6fd50170ae08d5f79c Mon Sep 17 00:00:00 2001 From: Nitin Mohan Date: Mon, 23 Mar 2020 18:06:08 -0700 Subject: [PATCH 2/3] Encapsulate result type name derivation logic --- dsl/result_type.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/dsl/result_type.go b/dsl/result_type.go index bd9e497c8c..54b006a801 100644 --- a/dsl/result_type.go +++ b/dsl/result_type.go @@ -67,14 +67,13 @@ func ResultType(identifier string, fn func()) *expr.ResultTypeExpr { return nil } + identifier, typeName, err := mediaTypeToResultType(identifier) // Validate Result Type - identifier, params, err := mime.ParseMediaType(identifier) if err != nil { eval.ReportError("invalid result type identifier %#v: %s", identifier, err) // We don't return so that other errors may be captured in this // one run. - identifier = "text/plain" } canonicalID := expr.CanonicalIdentifier(identifier) // Validate that result type identifier doesn't clash @@ -86,9 +85,8 @@ func ResultType(identifier string, fn func()) *expr.ResultTypeExpr { return nil } } - identifier = mime.FormatMediaType(identifier, params) // Now save the type in the API result types map - mt := expr.NewResultTypeExpr(buildTypeName(identifier), identifier, fn) + mt := expr.NewResultTypeExpr(typeName, identifier, fn) expr.Root.ResultTypes = append(expr.Root.ResultTypes, mt) return mt @@ -270,13 +268,11 @@ func CollectionOf(v interface{}, adsl ...func()) *expr.ResultTypeExpr { } } else { // Check if a result type exists with the given identifier - id, params, err := mime.ParseMediaType(id) + id, typeName, err := mediaTypeToResultType(id) if err != nil { - eval.ReportError("invalid result type identifier %#v: %s", id, err) - return nil + eval.ReportError("invalid result type identifier %#v in CollectionOf: %s", id, err) } - id = mime.FormatMediaType(id, params) - if dt := expr.Root.UserType(buildTypeName(id)); dt != nil { + if dt := expr.Root.UserType(typeName); dt != nil { if mt, ok := dt.(*expr.ResultTypeExpr); ok { m = mt } @@ -442,9 +438,15 @@ func Attributes(fn func()) { eval.Execute(fn, mt) } -// buildTypeName builds the result type name from the formatted identifier and -// returns it. -func buildTypeName(identifier string) string { +// mediaTypeToResultType returns the formatted identifier and the result type +// name from the given identifier string. If the given identifier is invalid it +// returns text/plain as the identifier and an error. +func mediaTypeToResultType(identifier string) (string, string, error) { + identifier, params, err := mime.ParseMediaType(identifier) + if err != nil { + identifier = "text/plain" + } + identifier = mime.FormatMediaType(identifier, params) lastPart := identifier lastPartIndex := strings.LastIndex(identifier, "/") if lastPartIndex > -1 { @@ -464,7 +466,7 @@ func buildTypeName(identifier string) string { resultTypeCount++ typeName = fmt.Sprintf("ResultType%d", resultTypeCount) } - return typeName + return identifier, typeName, err } // buildView builds a view expression given an attribute and a corresponding From f93038424b32e51ba0bcea152186a38c79f0c33a Mon Sep 17 00:00:00 2001 From: Nitin Mohan Date: Tue, 24 Mar 2020 09:51:02 -0700 Subject: [PATCH 3/3] Address review comments --- dsl/result_type.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dsl/result_type.go b/dsl/result_type.go index 54b006a801..68606b4c28 100644 --- a/dsl/result_type.go +++ b/dsl/result_type.go @@ -269,14 +269,14 @@ func CollectionOf(v interface{}, adsl ...func()) *expr.ResultTypeExpr { } else { // Check if a result type exists with the given identifier id, typeName, err := mediaTypeToResultType(id) - if err != nil { - eval.ReportError("invalid result type identifier %#v in CollectionOf: %s", id, err) - } if dt := expr.Root.UserType(typeName); dt != nil { if mt, ok := dt.(*expr.ResultTypeExpr); ok { m = mt } } + if err != nil { + eval.ReportError("invalid result type identifier %#v in CollectionOf: %s", id, err) + } } } }