feat: extract type by node + add schema fuzz test #2138
Conversation
worstell
requested review from
a team and
deniseli
and removed request for
a team
worstell
force-pushed
the
worstell/20240718-wip
branch
3 times, most recently
from
d955f10 to
8398587
Compare
go-runtime/schema/extract_test.go
Outdated
| if strings.HasPrefix(symbol, "[]") { | ||
| return &schema.Array{Element: getSchemaType(symbol[2:])} | ||
| } | ||
| if strings.HasPrefix(symbol, "map[") { | ||
| idx := strings.Index(symbol, "]") | ||
| if idx == 5 { | ||
| // handle map[[]...]... | ||
| idx = strings.Index(symbol[6:], "]") + 6 | ||
| } | ||
| key := getSchemaType(symbol[4:idx]) | ||
| value := getSchemaType(symbol[idx+1:]) | ||
| return &schema.Map{Key: key, Value: value} | ||
| } | ||
| if strings.HasPrefix(symbol, "ftl.Option[") { | ||
| return &schema.Optional{Type: getSchemaType(symbol[11 : len(symbol)-1])} | ||
| } | ||
| panic(fmt.Sprintf("unexpected symbol: %s", symbol)) | ||
| } |
There was a problem hiding this comment.
I don't have a ton of context here, but this seems a bit brittle. Is this just how things have to work here with string checking?
There was a problem hiding this comment.
cleaned up a little and added some comments to make it clearer, e.g. indexing after 4 because map[ is 4 characters log
go-runtime/schema/extract_test.go
Outdated
| codeBuilder.WriteString("package test\n\n") | ||
| codeBuilder.WriteString(`import (` + "\n") | ||
| codeBuilder.WriteString(` "context"` + "\n") |
There was a problem hiding this comment.
Would it be better/easier to use raw string literals here instead of this codeBuilder? It seems a bit hard to write and update like this
There was a problem hiding this comment.
true! looks better
| } | ||
|
|
||
| // generate all possible permutations for slices, maps and ftl.Option[...] | ||
| func generatePermutations(symbols []string) []string { |
There was a problem hiding this comment.
niiiiice I wonder if we'll want to make this available as a general test util eventually
| permutations = append(permutations, "[]"+symbol) | ||
| permutations = append(permutations, "ftl.Option["+symbol+"]") | ||
| for _, nestedSymbol := range symbols { | ||
| permutations = append(permutations, fmt.Sprintf("map[%s]%s", symbol, nestedSymbol)) |
There was a problem hiding this comment.
would we ever need nested arrays like map[%s][]%s, or is that way overkill?
There was a problem hiding this comment.
ah yeah i omitted because the performance becomes exponentially worse and it seemed untenable
go-runtime/schema/extract_test.go
Outdated
| idx := strings.Index(symbol, "]") | ||
| if idx == 5 { |
There was a problem hiding this comment.
Could this mistakenly capture map[a]blah where a is some single-character alias of a supported type, or have we already swapped out all aliased types for their underlying values by this point? I wonder if it'd safer to just strings.HasPrefix(symbol, "map[[]") even if it duplicates part of the check... at least it would let us remove the comment
| } | ||
| } | ||
|
|
||
| func generateSourceCode(symbol string) string { |
There was a problem hiding this comment.
Do you think we could templatize this to make it a bit more readable/maintainable? I've only used https://pkg.go.dev/text/template#Must in the past but it's very similar to the scaffolder stuff we have (maybe it's built on top of this... I never actually checked)
go-runtime/schema/extract_test.go
Outdated
| verb sourceFunc(Unit) %s | ||
| } | ||
| `, typename, typename, typename, typename, typename, typename, typename, typename, valueenum, typeenum, typename, | ||
| typename, typename, typename, typename, typename, typename, typename) // there simply must be a better way |
There was a problem hiding this comment.
Oh I think you could use https://pkg.go.dev/text/template#Must here too!
go-runtime/schema/data/analyzer.go
Outdated
| unquoted, err := strconv.Unquote(tag.Value) | ||
| if err != nil { | ||
| common.Wrapf(pass, f, err, "failed to unquote tag value %q", tag.Value) | ||
| fieldErrors = true | ||
| } | ||
| tagContent := reflect.StructTag(unquoted).Get(aliasFieldTag) | ||
| tagParts := strings.Split(tagContent, ",") | ||
| jsonFieldName := "" | ||
| if len(tagParts) > 0 { | ||
| jsonFieldName = tagParts[0] | ||
| } | ||
|
|
||
| if jsonFieldName != "" { | ||
| metadata = append(metadata, &schema.MetadataAlias{ | ||
| Pos: common.GoPosToSchemaPos(pass.Fset, node.Pos()), | ||
| Kind: schema.AliasKindJSON, | ||
| Alias: jsonFieldName, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Could we move this into a helper function (parseTag or something like that)?
worstell
force-pushed
the
worstell/20240718-wip
branch
9 times, most recently
from
a83d83f to
0e62479
Compare
this change allows us to support `type Foo = external.Bar` aliases
worstell
force-pushed
the
worstell/20240718-wip
branch
from
0e62479 to
5769ba0
Compare
this change also allows us to support `type Foo = external.Bar` aliases fixes #2145
user requested support for `type Foo = other.Bar` pattern when declaring type aliases. this allows them to leverage member functions of the underlying type. after this change, the above pattern as well as the prior pattern, `type Foo other.Bar` will be supported for type aliasing. also fixes #2145
user requested support for `type Foo = other.Bar` pattern when declaring type aliases. this allows them to leverage member functions of the underlying type. after this change, the above pattern as well as the prior pattern, `type Foo other.Bar` will be supported for type aliasing. also fixes #2145
this change also allows us to support
type Foo = external.Baraliases