Create storage conversion helper functions #1495

theunrepentantgeek · 2021-05-19T03:14:23Z

What this PR does / why we need it:

Create storage variants of all resources and complex object types, injecting helper conversion functions for conversion back and forth. These helper functions will be used to build the actual conversions in a later PR.

Special notes for your reviewer:

The crux of operation is the StorageTypeFactory (see storage_type_factory.go) which uses some distinct utility classes to make the required changes.

Also includes:

Some improvements to debugger output for various types
Use of new abstractions PropertyContainer and FunctionContainer
Improvements to field & parameter names, and other code gardening

How does this PR make you feel:
#headingintherightdirection

If applicable:

this PR contains documentation
this PR contains tests

codecov-commenter · 2021-05-24T04:47:10Z

Codecov Report

Merging #1495 (e28774c) into master (7a9f076) will decrease coverage by 0.85%.
The diff coverage is 23.21%.

@@            Coverage Diff             @@
##           master    #1495      +/-   ##
==========================================
- Coverage   63.77%   62.91%   -0.86%     
==========================================
  Files         169      174       +5     
  Lines       11267    11497     +230     
==========================================
+ Hits         7185     7233      +48     
- Misses       3433     3611     +178     
- Partials      649      653       +4

Impacted Files	Coverage Δ
hack/generator/pkg/astmodel/known_locals_set.go	`100.00% <ø> (ø)`
...ck/generator/pkg/astmodel/package_reference_set.go	`94.73% <0.00%> (-5.27%)`	⬇️
hack/generator/pkg/astmodel/resource_type.go	`71.00% <0.00%> (-0.30%)`	⬇️
...enerator/pkg/astmodel/storage_package_reference.go	`90.00% <0.00%> (+20.00%)`	⬆️
hack/generator/pkg/astmodel/type_visitor.go	`51.61% <0.00%> (ø)`
hack/generator/pkg/astmodel/types.go	`56.97% <0.00%> (-5.05%)`	⬇️
hack/generator/pkg/astmodel/validated_type.go	`81.01% <0.00%> (ø)`
...rator/pkg/codegen/pipeline_create_storage_types.go	`10.34% <0.00%> (+7.18%)`	⬆️
...rator/pkg/codegen/pipeline_mark_storage_version.go	`60.46% <0.00%> (-6.21%)`	⬇️
...generator/pkg/codegen/storage/function_injector.go	`0.00% <0.00%> (ø)`
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a9f076...e28774c. Read the comment docs.

babbageclunk

This looks great - I really like the split-out type- and property converter.

babbageclunk · 2021-05-25T04:36:11Z

hack/generator/pkg/codegen/storage/storage_type_factory.go

+	result := &StorageTypeFactory{
+		service:                    service,
+		types:                      make(astmodel.Types),
+		pendingStorageConversion:   astmodel.MakeTypeNameQueue(),
+		pendingConversionInjection: astmodel.MakeTypeNameQueue(),
+		pendingMarkAsHubVersion:    astmodel.MakeTypeNameQueue(),
+		idFactory:                  idFactory,
+		conversionMap:              make(map[astmodel.PackageReference]astmodel.PackageReference),
+		functionInjector:           NewFunctionInjector(),
+		resourceHubMarker:          NewHubVersionMarker(),
+	}
+
+	result.typeConverter = NewTypeConverter(result.types)


Maybe this? It seems a bit cleaner than constructing result and then assigning typeConverter.

Suggested change

result := &StorageTypeFactory{

service: service,

types: make(astmodel.Types),

pendingStorageConversion: astmodel.MakeTypeNameQueue(),

pendingConversionInjection: astmodel.MakeTypeNameQueue(),

pendingMarkAsHubVersion: astmodel.MakeTypeNameQueue(),

idFactory: idFactory,

conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference),

functionInjector: NewFunctionInjector(),

resourceHubMarker: NewHubVersionMarker(),

}

result.typeConverter = NewTypeConverter(result.types)

types := make(astmodel.Types)

result := &StorageTypeFactory{

service: service,

types: types,

pendingStorageConversion: astmodel.MakeTypeNameQueue(),

pendingConversionInjection: astmodel.MakeTypeNameQueue(),

pendingMarkAsHubVersion: astmodel.MakeTypeNameQueue(),

idFactory: idFactory,

conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference),

functionInjector: NewFunctionInjector(),

resourceHubMarker: NewHubVersionMarker(),

typeConverter: NewTypeConverter(types),

}

Agreed. I've oscilated a bit on the style for initializing interrelated members, I think this works better.

babbageclunk · 2021-05-25T05:00:38Z

hack/generator/pkg/codegen/storage/function_injector.go

+func (_ *FunctionInjector) injectFunctionIntoObject(
+	_ *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) {
+	fn := ctx.(astmodel.Function)
+	return ot.WithFunction(fn), nil
+}
+
+// injectFunctionIntoResource takes the function provided as a context and includes it on the
+// provided resource type
+func (_ *FunctionInjector) injectFunctionIntoResource(
+	_ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) {
+	fn := ctx.(astmodel.Function)
+	return rt.WithFunction(fn), nil
+}


These methods are simple enough that I think the visitor would be clearer with them inline in NewFunctionInjector.

They are simple functions, but giving them names makes the intentions super clear when the TypeVisitorBuilder is used. #wontfix

babbageclunk · 2021-05-25T05:01:40Z

hack/generator/pkg/codegen/storage/hub_version_marker.go

+// MarkAsStorageVersion mark the supplied type definition as the storage version
+func (m *HubVersionMarker) MarkAsStorageVersion(def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) {
+	return m.visitor.VisitDefinition(def, nil)
+}
+
+// markResourceAsStorageVersion marks the supplied resource as the canonical hub (storage) version
+func (m *HubVersionMarker) markResourceAsStorageVersion(
+	_ *astmodel.TypeVisitor, rt *astmodel.ResourceType, _ interface{}) (astmodel.Type, error) {
+	return rt.MarkAsStorageVersion(), nil
+}


Same with this.

I think there's benefit in giving intention revealing names. #wontfix

babbageclunk · 2021-05-25T08:38:36Z

hack/generator/pkg/codegen/storage/storage_type_factory.go

+}
+
+func (f *StorageTypeFactory) process() error {
+	err := f.pendingStorageConversion.Process(f.createStorageVariant)


I'm not sure the queues are buying you much here - each one is only populated from one place, and they're processed one after the other, so they could have just been slices. That said, they don't really hurt either. I was just expecting the population logic to be more complicated.

Agree: it wasn't clear to me if the queue type was actually needed here or not (I think not?)

Would the whole relationship between types and pendingStorageConversion be simpler if instead you just did away with pendingStorageConversion entirely, had Add add to f.Types, and then had a result astmodel.Types which was produced by process?

So:

inputTypes astmodel.Types // input types to be processed by process() - this is filled out by Add

The set of types that are pendingStorageConversion then is just all inputTypes? So you loop over that and call createStorageVariant for each one, except now that you're not doing it using the queue-processing function you have a bit more flexibility in return type, so you could just have createStorageVariant return the new TypeDefinition it created, which you could store in a local result astmodel.Types variable inside process()? This code ends up looking a lot more like a mini-pipeline stage (in that it's dealing with sets of types?)

I feel like the separation of the set of input types and the set of output types helps reason about things like createStorageVariant more easily, as that operates on f.types, but if f.types is simultanously being modified to include the result of added storage variants there's more confusion in "what exactly is in f.types" than there has to be?

hack/generator/pkg/astbuilder/builder.go

hack/generator/pkg/astmodel/std_references.go

hack/generator/pkg/astmodel/storage_conversion_factories.go

hack/generator/pkg/astmodel/storage_conversion_function.go

hack/generator/pkg/codegen/storage/property_converter.go

matthchr · 2021-05-25T18:30:20Z

hack/generator/pkg/codegen/storage/storage_type_factory.go

+	pendingStorageConversion   astmodel.TypeNameQueue                                  // Queue of types that need storage variants created for them
+	pendingConversionInjection astmodel.TypeNameQueue                                  // Queue of types that need conversion functions injected
+	pendingMarkAsHubVersion    astmodel.TypeNameQueue                                  // Queue of types that need to be flagged as the hub storage version
+	idFactory                  astmodel.IdentifierFactory                              // Factory for creating identifiers


minor, style: Is the description in a tabbed-over string a standard golang style? (@babbageclunk knows maybe?)

It reads pretty cleanly here, but also isn't a "golang-style" comment as it's not starting with the name of the variable it refers to?

Mostly just wondering for my own knowledge.

I'm sure I've seen this - can change it if not idiomatic

matthchr · 2021-05-25T18:34:05Z

hack/generator/pkg/codegen/storage/storage_type_factory.go

+}
+
+func (f *StorageTypeFactory) process() error {
+	err := f.pendingStorageConversion.Process(f.createStorageVariant)


Agree: it wasn't clear to me if the queue type was actually needed here or not (I think not?)

matthchr · 2021-05-25T18:49:06Z

hack/generator/pkg/codegen/storage/storage_type_factory.go

+}
+
+func (f *StorageTypeFactory) process() error {
+	err := f.pendingStorageConversion.Process(f.createStorageVariant)


Would the whole relationship between types and pendingStorageConversion be simpler if instead you just did away with pendingStorageConversion entirely, had Add add to f.Types, and then had a result astmodel.Types which was produced by process?

So:

inputTypes astmodel.Types // input types to be processed by process() - this is filled out by Add

The set of types that are pendingStorageConversion then is just all inputTypes? So you loop over that and call createStorageVariant for each one, except now that you're not doing it using the queue-processing function you have a bit more flexibility in return type, so you could just have createStorageVariant return the new TypeDefinition it created, which you could store in a local result astmodel.Types variable inside process()? This code ends up looking a lot more like a mini-pipeline stage (in that it's dealing with sets of types?)

I feel like the separation of the set of input types and the set of output types helps reason about things like createStorageVariant more easily, as that operates on f.types, but if f.types is simultanously being modified to include the result of added storage variants there's more confusion in "what exactly is in f.types" than there has to be?

hack/generator/pkg/codegen/storage/type_converter.go

matthchr

A few more minor suggestions - last main big topic is the queues + processing which I think @babbageclunk and I had comments on. (Not necessarily agitating for a specific change there but you hadn't responded to those comments yet so unsure if you're planning changes there or not).

hack/generator/pkg/astmodel/storage_conversion_factories.go

hack/generator/pkg/codegen/pipeline_create_storage_types.go

matthchr · 2021-05-26T17:53:40Z

hack/generator/pkg/codegen/storage/property_converter.go

+}
+
+// shortCircuitNamesOfSimpleTypes redirects or replaces TypeNames
+//   o  If a TypeName points into an API namespace, it is redirected into the appropriate storage namespace


namespace -> package (I think that's the appropriate Golang terminology?)

hack/generator/pkg/codegen/storage/property_converter.go

hack/generator/pkg/codegen/storage/storage_type_factory.go

matthchr · 2021-05-26T18:58:43Z

hack/generator/pkg/astmodel/types.go

@@ -204,3 +203,22 @@ func (types Types) ResolveResourceStatusDefinition(
 	// preserve outer spec name
 	return resourceStatusDef.WithName(statusName), nil
 }
+
+// Process applies a func to transform all members of this set of type definitions, returning a new set of type
+// definitions containing the results of the transfomration, or possibly an error


Suggested change

// definitions containing the results of the transfomration, or possibly an error

// definitions containing the results of the transformation, or possibly an error

Missed this fix - included in my current branch instead.

matthchr

LGTM!

Co-authored-by: Matthew Christopher <matthchr@microsoft.com>

theunrepentantgeek · 2021-05-26T20:38:05Z

MC> A few more minor suggestions - last main big topic is the queues + processing which I think
MC> @babbageclunk and I had comments on.

Sorry, I missed responding to those comments - I've removed the use of queues completely.

theunrepentantgeek requested review from babbageclunk, davefellows, matthchr and Porges as code owners May 19, 2021 03:14

theunrepentantgeek force-pushed the feature/storage-conversions branch from ef458d4 to 02bfb2a Compare May 23, 2021 20:01

theunrepentantgeek self-assigned this May 24, 2021

theunrepentantgeek added this to the codegen-alpha-0 milestone May 24, 2021

babbageclunk approved these changes May 25, 2021

View reviewed changes

matthchr reviewed May 25, 2021

View reviewed changes

theunrepentantgeek added 19 commits May 26, 2021 11:33

Move supporting types into subpackage

a381d35

Move type visitor creation onto StorageTypeFactory

40d0640

Remove StorageTypesVisitorContext

2232244

Rename all receivers

8e32767

Use AsEnumType() instead of hard cast

4886c47

Move creation of storage types into the factory

98bb10f

Use a separate factory for each group

e0a8960

Fix bug generating storage versions of resources

1f65c9f

Use queue for just in time creation of storage types

e9eec16

Improve logging of conversion issues

14445fb

Don't case convert endpoint names

1632b82

Update StorageTypeFactory to use the name queue

26bb92b

Code gardening

39d9bce

Add injection of conversion functions

6f1bc35

Improve debug diagnostics

8d19b86

Change to using TypeVisitorBuilder

cc635d4

Code gardening

ae17cc6

Rename member to otherDefinition

59e9c9e

Use PropertyContainer to generate conversions

24a855b

theunrepentantgeek added 2 commits May 26, 2021 11:34

Add Process() to Types

e0b6a65

Preserve ResourceReferences

ace581a

theunrepentantgeek force-pushed the feature/storage-conversions branch from 83b43f0 to 49ebcfe Compare May 25, 2021 23:34

theunrepentantgeek added 7 commits May 26, 2021 12:11

Code gardening

366eb26

Add support for ResourceReference properties

6d2ce02

Simplify error handling when creating conversions

f1b5efe

Refactor StorageTypeFactory

3bcd98b

Add warning

5a55825

Simplify handling of aggregate errors

53ed56c

Rename member to group

89009cd

theunrepentantgeek force-pushed the feature/storage-conversions branch from 49ebcfe to da79cc3 Compare May 26, 2021 00:12

Include property type in error

52c3aaf

theunrepentantgeek force-pushed the feature/storage-conversions branch from da79cc3 to 52c3aaf Compare May 26, 2021 00:18

theunrepentantgeek added 2 commits May 26, 2021 14:15

Go fmt

1cec073

Merge branch 'master' into feature/storage-conversions

e8d4af1

matthchr reviewed May 26, 2021

View reviewed changes

matthchr approved these changes May 26, 2021

View reviewed changes

theunrepentantgeek and others added 2 commits May 27, 2021 07:50

Apply suggestions from code review

aa83be6

Co-authored-by: Matthew Christopher <matthchr@microsoft.com>

Fix terminology, using package instead of namespace

21df630

Merge from master

5435b85

theunrepentantgeek force-pushed the feature/storage-conversions branch from a2f32fa to 5435b85 Compare May 26, 2021 21:15

theunrepentantgeek modified the milestones: codegen-alpha-0, codegen-alpha-1 May 26, 2021

Add GitHub errors reference

e28774c

theunrepentantgeek mentioned this pull request May 27, 2021

Create conversions package #1520

Merged

1 task

theunrepentantgeek merged commit 7a6e709 into master May 27, 2021

theunrepentantgeek deleted the feature/storage-conversions branch May 27, 2021 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create storage conversion helper functions #1495

Create storage conversion helper functions #1495

theunrepentantgeek commented May 19, 2021

codecov-commenter commented May 24, 2021 •

edited

Loading

babbageclunk left a comment

babbageclunk May 25, 2021

theunrepentantgeek May 25, 2021

babbageclunk May 25, 2021

theunrepentantgeek May 25, 2021 •

edited

Loading

babbageclunk May 25, 2021

theunrepentantgeek May 25, 2021

babbageclunk May 25, 2021

matthchr May 25, 2021

matthchr May 25, 2021

matthchr May 25, 2021

theunrepentantgeek May 26, 2021

matthchr May 25, 2021

matthchr May 25, 2021

matthchr left a comment

matthchr May 26, 2021

theunrepentantgeek May 26, 2021

matthchr May 26, 2021

theunrepentantgeek May 27, 2021

matthchr left a comment

theunrepentantgeek commented May 26, 2021

	// definitions containing the results of the transfomration, or possibly an error
	// definitions containing the results of the transformation, or possibly an error

Create storage conversion helper functions #1495

Create storage conversion helper functions #1495

Conversation

theunrepentantgeek commented May 19, 2021

codecov-commenter commented May 24, 2021 • edited Loading

Codecov Report

babbageclunk 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

theunrepentantgeek May 25, 2021 • edited Loading

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

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

matthchr 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

matthchr left a comment

Choose a reason for hiding this comment

theunrepentantgeek commented May 26, 2021

codecov-commenter commented May 24, 2021 •

edited

Loading

theunrepentantgeek May 25, 2021 •

edited

Loading