feat: schema changes to support SumType #1322

deniseli · 2024-04-23T20:46:00Z

Design doc: https://hackmd.io/TZqMfuyHTYaXd1gSECbpFg?view

worstell · 2024-04-23T20:51:02Z

backend/schema/sumtype.go

+		Pos:      posToProto(s.Pos),
+		Comments: s.Comments,
+		Name:     s.Name,
+		Variants: variants,


we have a helper for this, so you don't have to construct your own list!

Suggested change

Variants: variants,

Variants: nodeListToProto[*schemapb.Type](s.Variants),

oops, actually:

Suggested change

Variants: variants,

Variants: slices.Map(s.Variants, typeToProto),

alecthomas · 2024-04-23T21:12:00Z

backend/schema/jsonschema.go

+		for i, v := range node.Variants {
+			variants[i] = jsonschema.SchemaOrBool{TypeObject: nodeToJSSchema(v, refs)}
+		}
+		return &jsonschema.Schema{OneOf: variants}


Nice!

Though, this should be using {"kind": xyz, "value": ...}, or is that in a followup?

fyi - from feedback during the design sync we actually lifted the original struct out of value, so it'll just be an additional field named @_kind (prefixed with @_ to avoid conflicts) at the top level now

Mmm, @_kind can still conflict though if the sum type variant is a map:

sumtype SumType = {String: String} | Int var value SumType = {"@_kind": "foo"}

ah good point, i misunderstood your prior comment from the design sync. we can go with the kind/value approach then

alecthomas

LGTM!

alecthomas · 2024-04-23T21:13:25Z

backend/schema/parser.go

@@ -41,6 +41,7 @@ var (
 		{Name: "String", Pattern: `"(?:\\.|[^"])*"`},
 		{Name: "Number", Pattern: `[0-9]+(?:\.[0-9]+)?`},
 		{Name: "Punct", Pattern: `[%/\-\_:[\]{}<>()*+?.,\\^$|#~!\'@]`},
+		{Name: "Equals", Pattern: `=`},


Just add this to Punct instead.

alecthomas · 2024-04-23T21:13:45Z

backend/schema/sumtype.go

+	Pos Position `parser:"" protobuf:"1,optional"`
+
+	Comments []string `parser:"@Comment*" protobuf:"2"`
+	Name     string   `parser:"'sumtype' @Ident Equals" protobuf:"3"`


This should be just '=' by adding = to Punct.

oo lala, thank you!

alecthomas · 2024-04-23T23:00:07Z

backend/schema/jsonschema_test.go

+              "type": "string"
+            }
+          },
+          "type": "integer"


I don't think this is valid is it? Unless I'm misunderstand this schema a type of "integer" can't have properties.

matt2e

approved (pending a decision on where we land on the wrapped + kind vs embeded + @_kind thing)

alecthomas · 2024-04-24T00:42:20Z

I think there's a JSON schema validation round trip test that we should use here to verify it's working as intended.

deniseli · 2024-04-24T18:55:21Z

Hey folks! Posting a summary here to round this out:

Overall, things here look good, except we aren't sure what we want the jsonschema output to look like. Specifically, we aren't sure what the shape of it should be, nor how kind will be inserted into the output schema w.r.t. the oneof.
The current approach just adds a separate Properties map adjacent to the variants' types. We aren't sure if this is ultimately what we'll want, but the unit tests in jsonschema_test.go have been updated to verify that sumtype refs work as intended. I stepped through TestJSONSchemaValidation per Alec's suggestion above and inspected the schema and interface at each step. I can at least say that the code works the way we intend; we just don't know if what we intend is actually the right thing.
The output produced by this change does pass the sniff test in https://www.jsonschemavalidator.net/. Also, when we run ftl dev ./examples/go --recreate, the console does show time and echo correctly. So at least, this shouldn't break things (that I know of).
This PR does not add any go-runtime changes yet. We will likely need to revisit the jsonschema structure for sumtypes when we get to that, if not sooner.
Since this PR is massive and touches a lot of the backend code, we want to go ahead and merge instead of sitting on it and accumulating endless merge conflicts. I will do that now.
If anything breaks as a result of this, please let me know. Thanks!

alecthomas · 2024-04-24T21:59:28Z

I don't see a test here exercising this... ie. there's no input value anywhere (I can see?) with @_kind in it?

alecthomas · 2024-04-24T22:01:12Z

backend/schema/jsonschema_test.go

@@ -307,7 +332,8 @@ func TestInvalidEnumValidation(t *testing.T) {
    "any": [{"name": "Name"}, "string", 1, 1.23, true, "2018-11-13T20:20:39+00:00", ["one"], {"one": 2}, null],
    "keyValue": {"key": "string", "value": 1},
 	"stringEnumRef": "B",
-	"intEnumRef": 3
+	"intEnumRef": 3,
+        "sumTypeRef": 10


Where's the @_kind?

It's here - https://github.com/TBD54566975/ftl/blob/ceb45dcf12c27841ecd7774c4e78f3f19e9ebf83/backend/schema/jsonschema_test.go#L138

Should we add that to TestInvalidEnumValidation too?

Yes I see it in the schema, but it's not used in the value.

Two cases where I don't think this will approach will work:

How do we differentiate between a sum type with Float and Int without a kind?

How does @_kind work if the type being discriminated is a map that can contain @_kind as a key.?

Ahhhhh ok this finally make sense to me now. I'll think about this a bit more and try out whatever I come up with with the rest of the types to make sure we can discriminate.

worstell · 2024-04-25T18:29:27Z

Hey folks! Posting a summary here to round this out:

Overall, things here look good, except we aren't sure what we want the jsonschema output to look like. Specifically, we aren't sure what the shape of it should be, nor how kind will be inserted into the output schema w.r.t. the oneof.

The current approach just adds a separate Properties map adjacent to the variants' types. We aren't sure if this is ultimately what we'll want, but the unit tests in jsonschema_test.go have been updated to verify that sumtype refs work as intended. I stepped through TestJSONSchemaValidation per Alec's suggestion above and inspected the schema and interface at each step. I can at least say that the code works the way we intend; we just don't know if what we intend is actually the right thing.

The output produced by this change does pass the sniff test in https://www.jsonschemavalidator.net/. Also, when we run ftl dev ./examples/go --recreate, the console does show time and echo correctly. So at least, this shouldn't break things (that I know of).

This PR does not add any go-runtime changes yet. We will likely need to revisit the jsonschema structure for sumtypes when we get to that, if not sooner.

Since this PR is massive and touches a lot of the backend code, we want to go ahead and merge instead of sitting on it and accumulating endless merge conflicts. I will do that now.

If anything breaks as a result of this, please let me know. Thanks!

btw, as far as I'm aware jsonschema is only used for the console, so wouldn't expect it to break anything in the lifecycle processes.

we won't need to worry about the issue of representing the kind property on non-struct types with the modified approach—wrapping in a struct that contains fields "kind" and "value", where the original value is nested in "value". let's update in a follow up

alecthomas · 2024-04-25T20:01:03Z

Agreed @worstell, but maybe let's have a quick chat about it in the daily sync? The kind+value combo is robust, but also won't help model a lot of real world use cases. I'm thinking in particular of the very common case where JSON schemas support something like "foo" | ["foo", "bar"], "foo" | {"name": "foo", "field": "bar"}, etc. OTOH if String | [String] is extended with String | [String] | [Int] at some future point, it will break :(

Maybe we support bare sum types if there's no ambiguity, and the kind discriminator when there is, and it's a backwards incompatible change that we warn about if switching from one to the other...?

worstell · 2024-04-25T21:54:34Z

@alecthomas sounds good, let's chat in sync!

making sum types enums could be another solution here, though it introduces a bunch of other complexity (worth it if we determine this to be the right approach conceptually, though). If we structured them as enums, each variant would have a name, a type, and possibly a "raw value" (inspired by your Swift reference). the value of kind is the variant name, rather than its type (so maybe we rename kind to name or something else).

in Go, all variants are necessarily either type aliases or themselves structs. the alias or struct name will become the variant name. in other runtimes we can structure the approach to ensure we have some way of deriving variant "names".

ex:
type Scalar string in user code, ftl/echo.Scalar key in the Scheme, and FTL schema variant name Scalar. Can be identified on ingress with the enum name, Scalar. it would look something like this in the schema:

module echo {
  enum ScalarOrList {
     Scalar String
     List   [String]
  }
}

Reverts everything in #1322 except for the addition of `=` to `Punct` in `backend/schema/parser.go`, which we will still be using for value enums. Going forward, sumtypes will be implemented as type enums.

deniseli added 11 commits April 19, 2024 14:46

initial changes

f1f0356

catching up latest changes

a789639

change format to | instead of newline

b0e670c

buf generate

f43186c

todo fix unit tests

99269ff

trying some more things

2712fc6

regenerate protos after getting rid of variant struct

088eee8

schema tests passing

feb2aab

use equals and pipes

8f5339e

rename types to variants

2aa1d81

lint errors

de64b8a

deniseli requested a review from a team as a code owner April 23, 2024 20:46

deniseli requested review from matt2e and removed request for a team April 23, 2024 20:46

alecthomas mentioned this pull request Apr 23, 2024

Dashboard #728

Open

Merge branch 'main' into dli/sumtypes-decl

ab9f8ac

worstell reviewed Apr 23, 2024

View reviewed changes

alecthomas reviewed Apr 23, 2024

View reviewed changes

alecthomas approved these changes Apr 23, 2024

View reviewed changes

deniseli added 2 commits April 23, 2024 17:43

ingress + tests

4e6b788

add kind to map and improve punct

090ef5e

alecthomas reviewed Apr 23, 2024

View reviewed changes

matt2e approved these changes Apr 23, 2024

View reviewed changes

jsonschema kind

ceb45dc

worstell self-requested a review April 24, 2024 00:41

worstell approved these changes Apr 24, 2024

View reviewed changes

deniseli merged commit c214d92 into main Apr 24, 2024
11 checks passed

deniseli deleted the dli/sumtypes-decl branch April 24, 2024 18:55

alecthomas reviewed Apr 24, 2024

View reviewed changes

deniseli mentioned this pull request Apr 29, 2024

chore: revert obsolete sumtype decl #1355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: schema changes to support SumType #1322

feat: schema changes to support SumType #1322

deniseli commented Apr 23, 2024

worstell Apr 23, 2024

worstell Apr 23, 2024

deniseli Apr 23, 2024

alecthomas Apr 23, 2024

worstell Apr 23, 2024

alecthomas Apr 23, 2024 •

edited

Loading

worstell Apr 25, 2024

alecthomas left a comment

alecthomas Apr 23, 2024

deniseli Apr 23, 2024

alecthomas Apr 23, 2024

deniseli Apr 23, 2024

alecthomas Apr 23, 2024

matt2e left a comment •

edited

Loading

alecthomas commented Apr 24, 2024

deniseli commented Apr 24, 2024 •

edited

Loading

alecthomas commented Apr 24, 2024

alecthomas Apr 24, 2024

deniseli Apr 24, 2024

alecthomas Apr 24, 2024

alecthomas Apr 24, 2024

deniseli Apr 25, 2024

worstell commented Apr 25, 2024

alecthomas commented Apr 25, 2024

worstell commented Apr 25, 2024

	Variants: variants,
	Variants: nodeListToProto[*schemapb.Type](s.Variants),

	Variants: variants,
	Variants: slices.Map(s.Variants, typeToProto),

feat: schema changes to support SumType #1322

feat: schema changes to support SumType #1322

Conversation

deniseli commented Apr 23, 2024

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

alecthomas Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas 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

matt2e left a comment • edited Loading

Choose a reason for hiding this comment

alecthomas commented Apr 24, 2024

deniseli commented Apr 24, 2024 • edited Loading

alecthomas commented Apr 24, 2024

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

worstell commented Apr 25, 2024

alecthomas commented Apr 25, 2024

worstell commented Apr 25, 2024

alecthomas Apr 23, 2024 •

edited

Loading

matt2e left a comment •

edited

Loading

deniseli commented Apr 24, 2024 •

edited

Loading