Add support for Transforms that map fields in the Type Converter #116

stevededalus · 2017-06-01T02:06:49Z

Generates code that allows fields of the same type to be mapped to one another based on a provided mapping of field names. Performs overwritting if provided flag is set.

Remaining work for future PRs:

Wire transform configs from a middleware config file in the the type map
Add nil checks for deeply nested structs (that can contain required fields nested in optional fields)
Add camelCasing and other transforms
Ability to remove mapped field
Support mapping optional -> required using zero-values and pointer dereferencing

coveralls · 2017-06-01T05:47:12Z

Coverage increased (+0.6%) to 75.326% when pulling 4dde80c on transforms into 3154c35 on master.

Raynos

partial comments, more review to come.

Raynos · 2017-06-02T01:50:05Z

codegen/type_converter_test.go

+		program.Types[toStruct].(*compile.StructSpec).Fields,
+		"")
+
+	fmt.Printf("%v", overrideMap)


nitpick: remove dead print.

Raynos · 2017-06-02T01:53:06Z

codegen/type_converter.go

+// converting and overriding fields.
+type FieldMapperEntry struct {
+	QualifiedName string
+	Field         *compile.FieldSpec


comment about whether this is source or destination FieldSpec ?

coveralls · 2017-06-12T18:54:12Z

Coverage increased (+0.7%) to 75.397% when pulling c54fc89 on transforms into 3154c35 on master.

Raynos

I do not understand the use case for this "Override" feature. it really complicates the code generation and the generated code.

It might benefit to add tests that describe intent instead of just "one two three".

Raynos · 2017-06-12T20:05:50Z

codegen/type_converter.go

+	}
+
+	// TODO: Verify all intermediate objects are not nil
+	c.append(indent, toIdentifier, " = ", typeName, "(", overriddenIdentifier, ")")


Why do we assign from both fields ?

It looks like a transform should only support the transformed field not the original field.

Raynos

Can we have a markdown document for all possible permutations ?

toFieldReqOpt, originalFromFieldReqOpt, overriddenFromFieldReqOpt, overrideBool.

There are 16 permutations and we should state : "use orig field" , "use overriden field" , "try overridden then orig" , "try orig then overridden" or "invalid permutation, junk semantics"

Once we agree these 16 permutations make since from a documentation perspective we can then verify we have tests for 16 cases and that the code is correct.

Raynos · 2017-06-12T21:22:53Z

codegen/type_converter_test.go

+		QualifiedName: "Two",
+		Override:      true,
+	} // Map from required filed, unconditional assignment
+	fieldMap["Two"] = codegen.FieldMapperEntry{


It's confusing to set "override: true" but still have the field come from in.Two instead of in.One

Maybe we should make both fields required if override is true and fail the code generation if one of them is optional.

Raynos · 2017-06-12T21:23:56Z

codegen/type_converter_test.go

+		QualifiedName: "One",
+		Override:      true,
+	}
+	fieldMap["Three"] = codegen.FieldMapperEntry{


Override true should preferablly not read from in.Three

Raynos · 2017-06-12T21:25:26Z

codegen/type_converter_test.go

+	}
+	fieldMap["Four"] = codegen.FieldMapperEntry{
+		QualifiedName: "Three",
+		Override:      false,


Very sneaky but correct.

Raynos · 2017-06-12T21:28:34Z

codegen/type_converter_test.go

+	assert.NoError(t, err)
+	assert.Equal(t, trim(`
+		out.One = (*bool)(in.Two)
+		if in.One != nil {


Might be cleaner to generate an if ... {} else {} to reduce confusion.

Add md file to https://github.com/uber/zanzibar/pull/116/files#diff-ab1cd02cf41c42554ab67a9479e64e29

Raynos · 2017-06-12T21:33:20Z

codegen/type_converter_test.go

+		if in.Four != nil {
+			out.Four = &structs.NestedBar{}
+			out.Four.One = string(in.Four.One)
+			out.Four.Two = (*string)(in.Four.One)


cannot cast string to *string generated code does not work.

Need to use ptr.String(in.Four.One) to cast from required to optional.

coveralls · 2017-07-18T23:41:59Z

Coverage increased (+0.3%) to 77.955% when pulling a27fb97 on transforms into 342e355 on master.

coveralls · 2017-08-02T22:18:27Z

Coverage increased (+0.2%) to 76.406% when pulling f64e17d on transforms into eaf12b8 on master.

coveralls · 2017-08-03T20:58:57Z

Coverage increased (+0.5%) to 76.704% when pulling 5a6027b on transforms into eaf12b8 on master.

…er configs

coveralls · 2017-08-08T00:47:17Z

Coverage increased (+0.006%) to 76.561% when pulling b06db68 on transforms into d85a193 on master.

Raynos

I'll do a little cleanup and then merge

Raynos · 2017-08-07T23:29:37Z

glide.yaml

@@ -47,3 +47,4 @@ import:
  version: 5b691c8ebc4af5191baa426561d62f1b5115d6e5
 - package: github.com/stretchr/testify
  version: f6abca593680b2315d2075e0f5e2a9751e3f431a
+- package: github.com/wacul/ptr


lets use the version of ptr that comes with thriftrw if we can.

Raynos · 2017-08-07T23:30:03Z

glide.lock

@@ -21,12 +21,24 @@ imports:
  version: 3e4f037f12a1221a0864cf0dd2e81c452ab22448
 - name: github.com/go-yaml/yaml
  version: a83829b6f1293c91addabc89d0571c246397bbf4
+- name: github.com/golang/lint


will need to regenerate with all this extra development only deps removed.

Not sure why glide suddenly added everything :/

coveralls · 2017-08-08T21:32:34Z

Coverage decreased (-0.09%) to 76.461% when pulling db38037 on transforms into d85a193 on master.

stevededalus requested a review from Raynos June 1, 2017 02:06

stevededalus requested a review from zhenghuiwang June 5, 2017 18:02

zhenghuiwang approved these changes Jun 5, 2017

View reviewed changes

Raynos reviewed Jun 6, 2017

View reviewed changes

Raynos reviewed Jun 12, 2017

View reviewed changes

stevededalus force-pushed the transforms branch from c54fc89 to 8d351f2 Compare July 18, 2017 23:08

stevededalus force-pushed the transforms branch from 841273c to f64e17d Compare August 2, 2017 21:37

stevededalus force-pushed the transforms branch from d81a300 to e2c91b8 Compare August 7, 2017 21:48

stevededalus added 16 commits August 7, 2017 17:26

Add transforms with overrride

585e2ea

Fix rebase

7beea0f

add helper method for tertiary assignment

c72c9fc

Add support for override map and list types

2c1822d

Add support for override map and list types

7ee4969

Add transformMap to testing function

db3c144

Use full paths for fields in transform mappers

5fd6358

Add tests, fix converter to conform to tests

47531f1

Add tests for bad configuration error cases

74498a0

Add tests for bad configuration error cases

3628ffd

Coverage to 100%

8dbb486

Fix tabs

6e5e7f3

Fix tabs

58b53be

Add tabel describing transforms based on types

4f809fe

Add description text

7e93946

Remove debugging print statements

20950d4

stevededalus added 10 commits August 7, 2017 17:26

Add hardcoded transfromRequest middleware and pipe through to convert…

71a08ee

…er configs

Fix test

4370fe9

Add error test case, get coverager to 100

b828c66

Regenerate

4d06238

Fix bug in recursive addSpec function that caused flakiness in testing

501cc49

Add ptr helper package

bb07345

Add pointer dereferencing for optional -> required mappings

629ddb0

More tests, coverage to 100%

95ab63a

Add response transforms

6585399

Add transform middleware schema

b06db68

stevededalus force-pushed the transforms branch from e2c91b8 to b06db68 Compare August 8, 2017 00:27

Raynos reviewed Aug 8, 2017

View reviewed changes

Raynos added 2 commits August 8, 2017 14:17

remove wacul/ptr

6da035e

cleanup glide.lock

db38037

Raynos merged commit a64843a into master Aug 8, 2017

Raynos deleted the transforms branch August 8, 2017 21:46

CortezFrazierJr mentioned this pull request Nov 29, 2023

[Snyk] Security upgrade eclint from 1.1.5 to 2.6.0 CortezFrazierJr/zanzibar#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Transforms that map fields in the Type Converter #116

Add support for Transforms that map fields in the Type Converter #116

stevededalus commented Jun 1, 2017 •

edited

Loading

coveralls commented Jun 1, 2017 •

edited

Loading

Raynos left a comment

Raynos Jun 2, 2017

stevededalus Jul 18, 2017

Raynos Jun 2, 2017

coveralls commented Jun 12, 2017 •

edited

Loading

Raynos left a comment

Raynos Jun 12, 2017

Raynos left a comment

Raynos Jun 12, 2017

Raynos Jun 12, 2017

Raynos Jun 12, 2017

Raynos Jun 12, 2017

stevededalus Aug 4, 2017

Raynos Jun 12, 2017

coveralls commented Jul 18, 2017 •

edited

Loading

coveralls commented Aug 2, 2017

coveralls commented Aug 3, 2017 •

edited

Loading

coveralls commented Aug 8, 2017 •

edited

Loading

Raynos left a comment

Raynos Aug 7, 2017

Raynos Aug 7, 2017

coveralls commented Aug 8, 2017 •

edited

Loading

Add support for Transforms that map fields in the Type Converter #116

Add support for Transforms that map fields in the Type Converter #116

Conversation

stevededalus commented Jun 1, 2017 • edited Loading

coveralls commented Jun 1, 2017 • edited Loading

Raynos 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

coveralls commented Jun 12, 2017 • edited Loading

Raynos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raynos 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 18, 2017 • edited Loading

coveralls commented Aug 2, 2017

coveralls commented Aug 3, 2017 • edited Loading

coveralls commented Aug 8, 2017 • edited Loading

Raynos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 8, 2017 • edited Loading

stevededalus commented Jun 1, 2017 •

edited

Loading

coveralls commented Jun 1, 2017 •

edited

Loading

coveralls commented Jun 12, 2017 •

edited

Loading

coveralls commented Jul 18, 2017 •

edited

Loading

coveralls commented Aug 3, 2017 •

edited

Loading

coveralls commented Aug 8, 2017 •

edited

Loading

coveralls commented Aug 8, 2017 •

edited

Loading