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.

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

feat: support external types with type widening #2010

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

worstell
Copy link
Contributor

@worstell worstell commented Jul 9, 2024

fixes #1296

external types can be declared using type aliases. they are widened to Any in the FTL schema, with metadata specifying the external type they will be serialized to/from (standard JSON serialization is used for external types)

import github.com/external/bar

type FtlType struct {
   Field AliasedNonFtlType
}

//ftl:typealias
//ftl:typemap kotlin "com.external.bar.NonFTLType"
type NonFtlType bar.NonFtlType

=>

data FtlType {
   Field foo.NonFtlType
}

typealias NonFtlType Any
  +typemap go "github.com/external/bar.Type"
  +typemap kotlin "com.external.bar.NonFTLType"

@worstell worstell requested a review from alecthomas as a code owner July 9, 2024 01:25
@worstell worstell requested review from a team and wesbillman and removed request for a team July 9, 2024 01:25
@ftl-robot ftl-robot mentioned this pull request Jul 9, 2024
@worstell worstell force-pushed the worstell/20240708-type-widening branch 8 times, most recently from 78a12d8 to 5e9b481 Compare July 9, 2024 02:10
Copy link
Collaborator

@wesbillman wesbillman left a comment

Choose a reason for hiding this comment

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

Nice @worstell!

Does this need any integration tests or are the unit tests covering what we need?

type MetadataTypeMap struct {
Pos Position `parser:"" protobuf:"1,optional"`

Runtime string `parser:"'+' 'typemap' @('go' | 'kotlin')" protobuf:"2"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might do this in other places, but I wonder if it will be tough to know where we need to add new languages to FTL. Like maybe we need to have rust in here too ? :)

Comment on lines +126 to +128
typealias NonFtlType Any
+typemap go "github.com/foo/bar.Type"
+typemap kotlin "com.foo.bar.Type"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

// and the package is "baz".
//
// in this case, import has an alias with the real package name: `import baz "github.com/foo/bar"`
im = fmt.Sprintf("%s %s", pkg, im)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This stuff is pretty hairy! ^

Copy link
Contributor Author

@worstell worstell Jul 9, 2024

Choose a reason for hiding this comment

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

i know 😬 lemme revisit or at least maybe clarify it with some more variable names

Comment on lines 41 to 42
type ExtractedDecl struct {
Decl schema.Decl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this type still useful now that it's just Decl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it needs to be wrapped in a "fact"

@@ -193,7 +193,7 @@ func nodeToJSSchema(node Node, refs map[RefKey]*Ref) *jsonschema.Schema {
*MetadataAlias, IngressPathComponent, *IngressPathLiteral, *IngressPathParameter, *Module,
*Schema, Type, *Database, *Verb, *EnumVariant, *MetadataCronJob, Value,
*StringValue, *IntValue, *TypeValue, *Config, *Secret, Symbol, Named,
*FSM, *FSMTransition, *MetadataRetry, *Topic, *Subscription, *MetadataSubscriber:
*FSM, *FSMTransition, *MetadataRetry, *Topic, *Subscription, *MetadataSubscriber, *MetadataTypeMap:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What should this do for jsonSchema? Would we have all the type info we need to support widen types in jsonSchema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will show up like an Any in jsonschema, which is the best we can do i think

@worstell
Copy link
Contributor Author

worstell commented Jul 9, 2024

Nice @worstell!

Does this need any integration tests or are the unit tests covering what we need?

there's one integration test! it deploys a verb that uses external types + calls it

i'll actually add another one that uses external types on an ingress request as well

@worstell worstell force-pushed the worstell/20240708-type-widening branch 6 times, most recently from 13119a2 to b03f194 Compare July 10, 2024 00:31
@worstell worstell force-pushed the worstell/20240708-type-widening branch from b03f194 to b073fb1 Compare July 10, 2024 00:41
@worstell worstell merged commit 2e07ee4 into main Jul 10, 2024
49 checks passed
@worstell worstell deleted the worstell/20240708-type-widening branch July 10, 2024 00:48
matt2e added a commit that referenced this pull request Jul 10, 2024
matt2e added a commit that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow FTL to support external types by utilising type widening
2 participants