Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: external_module.go with sinks/sources build failures #1132

Merged
merged 3 commits into from
Mar 25, 2024
Merged

fix: external_module.go with sinks/sources build failures #1132

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cc1ae5
fix: external_module.go with sinks/sources build failures
matt2e Mar 25, 2024
52e85e8
move verb unit logic to VisitExcludingMetadataChildren
matt2e Mar 25, 2024
2bf952c
undo whitespace change
matt2e Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions backend/schema/visit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ func Visit(n Node, visit func(n Node, next func() error) error) error {
// VisitExcludingMetadataChildren visits all nodes in the schema except the children of metadata nodes.
// This is used when generating external modules to avoid adding imports only referenced in the bodies of
// stubbed verbs.
func VisitExcludingMetadataChildren(n Node, visit func(n Node, next func() error) error) error {
return visit(n, func() error {
func VisitExcludingMetadataChildren(n Node, visit func(n Node, parents []Node, next func() error) error) error {
return innerVisitExcludingMetadataChildren(n, []Node{}, visit)
}

func innerVisitExcludingMetadataChildren(n Node, parents []Node, visit func(n Node, parents []Node, next func() error) error) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be simpler to type switch on Verb, manually call Visit() on the Verb's children iff they're not Unit and don't call next() in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that an odd separation of concerns though? As in the needs of a specific go scaffolding use case compared to schema/visit.go providing ways to traverse the graph.
Currently this is also called when scaffolding Kotlin (which to be fair, might not be affected by a change like that, not 100% sure).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you're saying, but no - this is supposed to be universal. It should be true in Kotlin too.

return visit(n, parents, func() error {
if _, ok := n.(Metadata); !ok {
parents = append(parents, n)
for _, child := range n.schemaChildren() {
if err := VisitExcludingMetadataChildren(child, visit); err != nil {
if err := innerVisitExcludingMetadataChildren(child, parents, visit); err != nil {
return err
}
}
Expand Down
1 change: 0 additions & 1 deletion buildengine/build_go_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ package other

import (
"context"
"github.com/TBD54566975/ftl/go-runtime/ftl"
)

var _ = context.Background
Expand Down
2 changes: 1 addition & 1 deletion buildengine/build_kotlin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ var scaffoldFuncs = scaffolder.FuncMap{
},
"imports": func(m *schema.Module) []string {
imports := sets.NewSet[string]()
_ = schema.VisitExcludingMetadataChildren(m, func(n schema.Node, next func() error) error {
_ = schema.VisitExcludingMetadataChildren(m, func(n schema.Node, parents []schema.Node, next func() error) error {
switch n.(type) {
case *schema.Data:
imports.Add("xyz.block.ftl.Export")
Expand Down
12 changes: 10 additions & 2 deletions go-runtime/compile/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ var scaffoldFuncs = scaffolder.FuncMap{
},
"imports": func(m *schema.Module) map[string]string {
imports := map[string]string{}
_ = schema.VisitExcludingMetadataChildren(m, func(n schema.Node, next func() error) error {
_ = schema.VisitExcludingMetadataChildren(m, func(n schema.Node, parents []schema.Node, next func() error) error {
switch n := n.(type) {
case *schema.Ref:
if n.Module == "" || n.Module == m.Name {
Expand All @@ -243,9 +243,17 @@ var scaffoldFuncs = scaffolder.FuncMap{
case *schema.Time:
imports["time"] = "stdtime"

case *schema.Optional, *schema.Unit:
case *schema.Optional:
imports["github.com/TBD54566975/ftl/go-runtime/ftl"] = ""

case *schema.Unit:
switch parents[len(parents)-1].(type) {
case *schema.Verb:
// Go verbs do not include Unit parameters in the signature, so should not cause an ftl import
break
default:
imports["github.com/TBD54566975/ftl/go-runtime/ftl"] = ""
}
default:
}
return next()
Expand Down
2 changes: 1 addition & 1 deletion go-runtime/compile/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func TestExtractModuleSchemaTwo(t *testing.T) {
verb callsTwo(two.Payload<String>) two.Payload<String>
+calls two.two

verb returnsUser(Unit) two.UserResponse?
verb returnsUser(Unit) two.UserResponse

verb two(two.Payload<String>) two.Payload<String>
}
Expand Down
6 changes: 3 additions & 3 deletions go-runtime/compile/testdata/two/two.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ func CallsTwo(ctx context.Context, req Payload[string]) (Payload[string], error)
}

//ftl:export
func ReturnsUser(ctx context.Context) (ftl.Option[UserResponse], error) {
return ftl.Some[UserResponse](UserResponse{
func ReturnsUser(ctx context.Context) (UserResponse, error) {
return UserResponse{
User: User{
Name: "John Doe",
},
}), nil
}, nil
}