From 2a855527bd4b1593fb3497d0347d2bc3ab784317 Mon Sep 17 00:00:00 2001 From: Matthew Christopher Date: Fri, 18 Jun 2021 11:38:42 -0700 Subject: [PATCH 1/2] All config options should error if no match by default - Ensure that all config transformers and filters raise an error by default if they don't match anything. - Removed config options that had no matches. --- hack/generator/azure-arm.yaml | 18 ++---- hack/generator/azure-crossplane.yaml | 9 --- .../codegen/pipeline_apply_export_filters.go | 11 ++-- .../pipeline_apply_property_rewrites.go | 6 ++ .../pkg/codegen/pipeline_load_schema.go | 11 ++++ hack/generator/pkg/config/configuration.go | 63 ++++++++++++++----- .../pkg/config/configuration_test.go | 12 ++-- hack/generator/pkg/config/type_matcher.go | 44 ++++++++----- hack/generator/pkg/config/type_transformer.go | 12 +--- 9 files changed, 111 insertions(+), 75 deletions(-) diff --git a/hack/generator/azure-arm.yaml b/hack/generator/azure-arm.yaml index 63d0ac7f4f8..acf18d1bf7e 100644 --- a/hack/generator/azure-arm.yaml +++ b/hack/generator/azure-arm.yaml @@ -32,10 +32,6 @@ typeFilters: group: microsoft.compute name: VirtualMachineScaleSetsExtensionsChildResource;VirtualMachinesExtensionsChildResource because: _childResource type which "points" to a resource type that doesn't exist. Need to work with schema owners on why. - - action: prune - group: microsoft.automation - name: AutomationAccountsRunbooksDraftChildResource - because: Uses OneOf in a weird way that makes detecting actual resource difficult, skipping for now. - action: prune group: microsoft.web version: '*2018*' @@ -76,10 +72,6 @@ typeFilters: because: Some types use OneOf to model inheritance, which we don't currently support. exportFilters: # First some always-applicable exclusions: - - action: exclude - group: definitions - name: NumberOrExpression - because: this type is not really used, it's just float64. We remap this type to float64 in typetransformers so we can skip exporting it too - action: exclude group: definitions name: '*ResourceBase*' @@ -184,11 +176,11 @@ typeTransformers: - name: "*" property: Tags ifType: - map: - key: - name: string - value: - name: any + map: + key: + name: string + value: + name: any target: map: key: diff --git a/hack/generator/azure-crossplane.yaml b/hack/generator/azure-crossplane.yaml index 096beaa031c..37b469fba26 100644 --- a/hack/generator/azure-crossplane.yaml +++ b/hack/generator/azure-crossplane.yaml @@ -30,10 +30,6 @@ typeFilters: version: '*' because: Prune everything else exportFilters: - - action: exclude - group: definitions - name: NumberOrExpression - because: this type is not really used, it's just float64. We remap this type to float64 in typetransformers so we can skip exporting it too - action: exclude group: definitions name: '*ResourceBase*' @@ -147,11 +143,6 @@ typeTransformers: optional: true remove: true because: It exists on ARMResource but doesn't make sense in the context of a CRD - - group: deploymenttemplate - name: ResourceLocations - target: - name: string - because: Modeling this as an enum doesn't work well in the context of CRDs because new regions are regularly added # Deal with readonly properties that were not properly pruned in the JSON schema - name: ResourceIdentity diff --git a/hack/generator/pkg/codegen/pipeline_apply_export_filters.go b/hack/generator/pkg/codegen/pipeline_apply_export_filters.go index c3dbdfe510c..a949b88d91a 100644 --- a/hack/generator/pkg/codegen/pipeline_apply_export_filters.go +++ b/hack/generator/pkg/codegen/pipeline_apply_export_filters.go @@ -31,10 +31,7 @@ func filterTypes( newDefinitions := make(astmodel.Types) - filterer, err := configuration.BuildExportFilterer(definitions) - if err != nil { - return nil, err - } + filterer := configuration.BuildExportFilterer(definitions) for _, def := range definitions { defName := def.Name() @@ -57,5 +54,11 @@ func filterTypes( } } + // Ensure that the export filters had no issues + err := configuration.GetExportFiltersError() + if err != nil { + return nil, err + } + return newDefinitions, nil } diff --git a/hack/generator/pkg/codegen/pipeline_apply_property_rewrites.go b/hack/generator/pkg/codegen/pipeline_apply_property_rewrites.go index c051f03750c..6c2197cc690 100644 --- a/hack/generator/pkg/codegen/pipeline_apply_property_rewrites.go +++ b/hack/generator/pkg/codegen/pipeline_apply_property_rewrites.go @@ -42,6 +42,12 @@ func applyPropertyRewrites(config *config.Configuration) PipelineStage { newTypes.Add(t.WithType(objectType)) } + // Ensure that the property transformers had no errors + err := config.GetPropertyTransformersError() + if err != nil { + return nil, err + } + return newTypes, nil }) } diff --git a/hack/generator/pkg/codegen/pipeline_load_schema.go b/hack/generator/pkg/codegen/pipeline_load_schema.go index 52cc5305567..89872a1bb53 100644 --- a/hack/generator/pkg/codegen/pipeline_load_schema.go +++ b/hack/generator/pkg/codegen/pipeline_load_schema.go @@ -159,6 +159,17 @@ func loadSchemaIntoTypes( return nil, errors.Wrapf(err, "failed to walk JSON schema") } + // Ensure that the type filters/transformers that are applied during schema graph walking + // are checked for errors before proceeding. These are the TypeTransformers and TypeFilters + err = configuration.GetTypeFiltersError() + if err != nil { + return nil, err + } + err = configuration.GetTypeTransformersError() + if err != nil { + return nil, err + } + return defs, nil }) } diff --git a/hack/generator/pkg/config/configuration.go b/hack/generator/pkg/config/configuration.go index cc0a5036ce8..a1ba37d9fef 100644 --- a/hack/generator/pkg/config/configuration.go +++ b/hack/generator/pkg/config/configuration.go @@ -84,6 +84,46 @@ func (config *Configuration) FullTypesRegistrationOutputFilePath() string { config.TypeRegistrationOutputFile) } +func (config *Configuration) GetTypeFiltersError() error { + for _, filter := range config.TypeFilters { + if !filter.HasMatches() { + return errors.Errorf("Type filter action: %q, target: %q matched no types", filter.Action, filter.String()) + } + } + + return nil +} + +func (config *Configuration) GetTypeTransformersError() error { + for _, filter := range config.typeTransformers { + if !filter.HasMatches() { + return errors.Errorf("Type transformer target: %q matched no types", filter.String()) + } + } + + return nil +} + +func (config *Configuration) GetPropertyTransformersError() error { + for _, filter := range config.propertyTransformers { + if !filter.HasMatches() { + return errors.Errorf("Type transformer target: %q for property %q matched no types", filter.String(), filter.Property) + } + } + + return nil +} + +func (config *Configuration) GetExportFiltersError() error { + for _, filter := range config.ExportFilters { + if !filter.HasMatches() { + return errors.Errorf("Export filter action: %q, target: %q matched no types", filter.Action, filter.String()) + } + } + + return nil +} + // NewConfiguration returns a new empty Configuration func NewConfiguration() *Configuration { return &Configuration{} @@ -267,7 +307,7 @@ func absDirectoryPathToURL(path string) *url.URL { type ExportFilterFunc func(astmodel.TypeName) (result ShouldExportResult, because string) -func buildExportFilterFunc(f *ExportFilter, allTypes astmodel.Types) (ExportFilterFunc, error) { +func buildExportFilterFunc(f *ExportFilter, allTypes astmodel.Types) ExportFilterFunc { switch f.Action { case ExportFilterExclude: return func(typeName astmodel.TypeName) (ShouldExportResult, string) { @@ -276,7 +316,7 @@ func buildExportFilterFunc(f *ExportFilter, allTypes astmodel.Types) (ExportFilt } return "", "" - }, nil + } case ExportFilterInclude: return func(typeName astmodel.TypeName) (ShouldExportResult, string) { @@ -285,29 +325,23 @@ func buildExportFilterFunc(f *ExportFilter, allTypes astmodel.Types) (ExportFilt } return "", "" - }, nil + } case ExportFilterIncludeTransitive: applicableTypes := make(astmodel.TypeNameSet) - foundMatch := false for tn := range allTypes { if f.AppliesToType(tn) { - foundMatch = true collectAllReferencedTypes(allTypes, tn, applicableTypes) } } - if !foundMatch { - return nil, errors.Errorf("no types matched for include-transitive filter: %v", f) - } - return func(typeName astmodel.TypeName) (ShouldExportResult, string) { if applicableTypes.Contains(typeName) { return Export, f.Because } return "", "" - }, nil + } default: panic(errors.Errorf("unknown exportfilter directive: %s", f.Action)) @@ -325,13 +359,10 @@ func collectAllReferencedTypes(allTypes astmodel.Types, root astmodel.TypeName, // BuildExportFilterer tests for whether a given type should be exported as Go code // Returns a result indicating whether export should occur as well as a reason for logging -func (config *Configuration) BuildExportFilterer(allTypes astmodel.Types) (ExportFilterFunc, error) { +func (config *Configuration) BuildExportFilterer(allTypes astmodel.Types) ExportFilterFunc { var filters []ExportFilterFunc for _, f := range config.ExportFilters { - filter, err := buildExportFilterFunc(f, allTypes) - if err != nil { - return nil, errors.Wrap(err, "building export filter func") - } + filter := buildExportFilterFunc(f, allTypes) filters = append(filters, filter) } @@ -345,7 +376,7 @@ func (config *Configuration) BuildExportFilterer(allTypes astmodel.Types) (Expor // By default we export all types return Export, "" - }, nil + } } // ShouldPrune tests for whether a given type should be extracted from the JSON schema or pruned diff --git a/hack/generator/pkg/config/configuration_test.go b/hack/generator/pkg/config/configuration_test.go index da450c0a563..ea94bd90219 100644 --- a/hack/generator/pkg/config/configuration_test.go +++ b/hack/generator/pkg/config/configuration_test.go @@ -42,8 +42,7 @@ func Test_WithSingleFilter_FiltersExpectedTypes(t *testing.T) { c := config.NewConfiguration() c = c.WithExportFilters(&filter) - f, err := c.BuildExportFilterer(make(astmodel.Types)) - g.Expect(err).ToNot(HaveOccurred()) + f := c.BuildExportFilterer(make(astmodel.Types)) g.Expect(f(person)).To(Equal(config.Export)) g.Expect(f(post)).To(Equal(config.Export)) @@ -68,8 +67,7 @@ func Test_WithMultipleFilters_FiltersExpectedTypes(t *testing.T) { c := config.NewConfiguration() c = c.WithExportFilters(&versionFilter, &nameFilter) - f, err := c.BuildExportFilterer(make(astmodel.Types)) - g.Expect(err).ToNot(HaveOccurred()) + f := c.BuildExportFilterer(make(astmodel.Types)) g.Expect(f(person)).To(Equal(config.Export)) g.Expect(f(post)).To(Equal(config.Export)) @@ -89,8 +87,7 @@ func Test_WithMultipleFilters_GivesPrecedenceToEarlierFilters(t *testing.T) { c := config.NewConfiguration() c = c.WithExportFilters(&alwaysExportPerson, &exclude2019) - f, err := c.BuildExportFilterer(make(astmodel.Types)) - g.Expect(err).ToNot(HaveOccurred()) + f := c.BuildExportFilterer(make(astmodel.Types)) g.Expect(f(person2019TypeName)).To(Equal(config.Export)) g.Expect(f(student2019TypeName)).To(Equal(config.Skip)) @@ -118,8 +115,7 @@ func Test_IncludeTransitive(t *testing.T) { types.Add(astmodel.MakeTypeDefinition(person2019TypeName, astmodel.NewArrayType(student2019TypeName))) types.Add(astmodel.MakeTypeDefinition(student2019TypeName, astmodel.StringType)) - f, err := c.BuildExportFilterer(types) - g.Expect(err).ToNot(HaveOccurred()) + f := c.BuildExportFilterer(types) // Person is top-level: g.Expect(f(person2019TypeName)).To(Equal(config.Export)) diff --git a/hack/generator/pkg/config/type_matcher.go b/hack/generator/pkg/config/type_matcher.go index 5b73292ce8c..e3bb4810857 100644 --- a/hack/generator/pkg/config/type_matcher.go +++ b/hack/generator/pkg/config/type_matcher.go @@ -26,32 +26,36 @@ type TypeMatcher struct { nameRegex *regexp.Regexp // Because is used to articulate why the filter applied to a type (used to generate explanatory logs in debug mode) Because string + + // MatchedTypes is the set of all type names matched by this matcher + matchedTypes astmodel.TypeNameSet } var _ fmt.Stringer = &TypeMatcher{} // Initialize initializes the type matcher -func (typeMatcher *TypeMatcher) Initialize() error { - typeMatcher.groupRegex = createGlobbingRegex(typeMatcher.Group) - typeMatcher.versionRegex = createGlobbingRegex(typeMatcher.Version) - typeMatcher.nameRegex = createGlobbingRegex(typeMatcher.Name) +func (t *TypeMatcher) Initialize() error { + t.groupRegex = createGlobbingRegex(t.Group) + t.versionRegex = createGlobbingRegex(t.Version) + t.nameRegex = createGlobbingRegex(t.Name) + t.matchedTypes = make(astmodel.TypeNameSet) return nil } -func (typeMatcher *TypeMatcher) groupMatches(schema string) bool { - return typeMatcher.matches(typeMatcher.Group, &typeMatcher.groupRegex, schema) +func (t *TypeMatcher) groupMatches(schema string) bool { + return t.matches(t.Group, &t.groupRegex, schema) } -func (typeMatcher *TypeMatcher) versionMatches(version string) bool { - return typeMatcher.matches(typeMatcher.Version, &typeMatcher.versionRegex, version) +func (t *TypeMatcher) versionMatches(version string) bool { + return t.matches(t.Version, &t.versionRegex, version) } -func (typeMatcher *TypeMatcher) nameMatches(name string) bool { - return typeMatcher.matches(typeMatcher.Name, &typeMatcher.nameRegex, name) +func (t *TypeMatcher) nameMatches(name string) bool { + return t.matches(t.Name, &t.nameRegex, name) } -func (typeMatcher *TypeMatcher) matches(glob string, regex **regexp.Regexp, name string) bool { +func (t *TypeMatcher) matches(glob string, regex **regexp.Regexp, name string) bool { if glob == "" { return true } @@ -64,14 +68,19 @@ func (typeMatcher *TypeMatcher) matches(glob string, regex **regexp.Regexp, name } // AppliesToType indicates whether this filter should be applied to the supplied type definition -func (typeMatcher *TypeMatcher) AppliesToType(typeName astmodel.TypeName) bool { +func (t *TypeMatcher) AppliesToType(typeName astmodel.TypeName) bool { if localRef, ok := typeName.PackageReference.AsLocalPackage(); ok { group := localRef.Group() version := localRef.Version() - result := typeMatcher.groupMatches(group) && - typeMatcher.versionMatches(version) && - typeMatcher.nameMatches(typeName.Name()) + result := t.groupMatches(group) && + t.versionMatches(version) && + t.nameMatches(typeName.Name()) + + // Track this match so we can later report if we didn't match anything + if result { + t.matchedTypes = t.matchedTypes.Add(typeName) + } return result } @@ -80,6 +89,11 @@ func (typeMatcher *TypeMatcher) AppliesToType(typeName astmodel.TypeName) bool { return false } +// HasMatches returns true if this matcher has ever matched at least 1 type +func (t *TypeMatcher) HasMatches() bool { + return len(t.matchedTypes) > 0 +} + // String returns a description of this filter func (t *TypeMatcher) String() string { var result strings.Builder diff --git a/hack/generator/pkg/config/type_transformer.go b/hack/generator/pkg/config/type_transformer.go index b828ca48bd1..d6306161631 100644 --- a/hack/generator/pkg/config/type_transformer.go +++ b/hack/generator/pkg/config/type_transformer.go @@ -174,16 +174,8 @@ func (transformer *TypeTransformer) propertyNameMatches(propName astmodel.Proper // TransformTypeName transforms the type with the specified name into the TypeTransformer target type if // the provided type name matches the pattern(s) specified in the TypeTransformer func (transformer *TypeTransformer) TransformTypeName(typeName astmodel.TypeName) astmodel.Type { - if localRef, ok := typeName.PackageReference.AsLocalPackage(); ok { - group := localRef.Group() - version := localRef.Version() - name := typeName.Name() - - if transformer.groupMatches(group) && - transformer.versionMatches(version) && - transformer.nameMatches(name) { - return transformer.targetType - } + if transformer.AppliesToType(typeName) { + return transformer.targetType } // Didn't match so return nil From dfc21e3f7b776035fc4c7ddec2526883b4db2716 Mon Sep 17 00:00:00 2001 From: Matthew Christopher Date: Fri, 18 Jun 2021 11:52:58 -0700 Subject: [PATCH 2/2] Add MatchRequired option --- hack/generator/azure-crossplane.yaml | 1 + hack/generator/pkg/config/configuration.go | 8 ++++---- hack/generator/pkg/config/type_matcher.go | 18 +++++++++++++++++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/hack/generator/azure-crossplane.yaml b/hack/generator/azure-crossplane.yaml index 37b469fba26..6b0a01477f4 100644 --- a/hack/generator/azure-crossplane.yaml +++ b/hack/generator/azure-crossplane.yaml @@ -75,6 +75,7 @@ typeTransformers: because: NumberOrExpression is an ARM template artifact that doesn't make sense in CRDs target: name: float + matchRequired: false # TODO: Remove this if/when we actually require it - name: "*" property: Tags ifType: diff --git a/hack/generator/pkg/config/configuration.go b/hack/generator/pkg/config/configuration.go index a1ba37d9fef..ebc7bd5e562 100644 --- a/hack/generator/pkg/config/configuration.go +++ b/hack/generator/pkg/config/configuration.go @@ -86,7 +86,7 @@ func (config *Configuration) FullTypesRegistrationOutputFilePath() string { func (config *Configuration) GetTypeFiltersError() error { for _, filter := range config.TypeFilters { - if !filter.HasMatches() { + if !filter.MatchedRequiredTypes() { return errors.Errorf("Type filter action: %q, target: %q matched no types", filter.Action, filter.String()) } } @@ -96,7 +96,7 @@ func (config *Configuration) GetTypeFiltersError() error { func (config *Configuration) GetTypeTransformersError() error { for _, filter := range config.typeTransformers { - if !filter.HasMatches() { + if !filter.MatchedRequiredTypes() { return errors.Errorf("Type transformer target: %q matched no types", filter.String()) } } @@ -106,7 +106,7 @@ func (config *Configuration) GetTypeTransformersError() error { func (config *Configuration) GetPropertyTransformersError() error { for _, filter := range config.propertyTransformers { - if !filter.HasMatches() { + if !filter.MatchedRequiredTypes() { return errors.Errorf("Type transformer target: %q for property %q matched no types", filter.String(), filter.Property) } } @@ -116,7 +116,7 @@ func (config *Configuration) GetPropertyTransformersError() error { func (config *Configuration) GetExportFiltersError() error { for _, filter := range config.ExportFilters { - if !filter.HasMatches() { + if !filter.MatchedRequiredTypes() { return errors.Errorf("Export filter action: %q, target: %q matched no types", filter.Action, filter.String()) } } diff --git a/hack/generator/pkg/config/type_matcher.go b/hack/generator/pkg/config/type_matcher.go index e3bb4810857..d50c340613a 100644 --- a/hack/generator/pkg/config/type_matcher.go +++ b/hack/generator/pkg/config/type_matcher.go @@ -26,8 +26,11 @@ type TypeMatcher struct { nameRegex *regexp.Regexp // Because is used to articulate why the filter applied to a type (used to generate explanatory logs in debug mode) Because string + // MatchRequired indicates if an error will be raised if this TypeMatcher doesn't match at least one type. + // The default is true. + MatchRequired *bool `yaml:"matchRequired,omitempty"` - // MatchedTypes is the set of all type names matched by this matcher + // matchedTypes is the set of all type names matched by this matcher matchedTypes astmodel.TypeNameSet } @@ -39,6 +42,11 @@ func (t *TypeMatcher) Initialize() error { t.versionRegex = createGlobbingRegex(t.Version) t.nameRegex = createGlobbingRegex(t.Name) t.matchedTypes = make(astmodel.TypeNameSet) + // Default MatchRequired + if t.MatchRequired == nil { + temp := true + t.MatchRequired = &temp + } return nil } @@ -89,6 +97,14 @@ func (t *TypeMatcher) AppliesToType(typeName astmodel.TypeName) bool { return false } +func (t *TypeMatcher) MatchedRequiredTypes() bool { + if *t.MatchRequired { + return t.HasMatches() + } + + return true +} + // HasMatches returns true if this matcher has ever matched at least 1 type func (t *TypeMatcher) HasMatches() bool { return len(t.matchedTypes) > 0