feat: HttpRequest path and query mapping #2422

stuartwdouglas · 2024-08-19T00:35:53Z

alecthomas

Awesome!

alecthomas · 2024-08-19T03:53:19Z

backend/controller/console/testdata/go/console/console.go

@@ -17,10 +17,10 @@ type Response struct {
 }

 //ftl:ingress http GET /test
-func Get(ctx context.Context, req builtin.HttpRequest[External]) (builtin.HttpResponse[Response, string], error) {
+func Get(ctx context.Context, req builtin.HttpRequest[ftl.Unit, ftl.Unit, External]) (builtin.HttpResponse[Response, string], error) {


Maybe later we should code-gen some type aliases to eradicate some of the boilerplate, it's pretty verbose.

Something like

type HttpRequestBody[T any] HttpRequest[Unit, Unit, T]

I had the same thought, we just need to figure out which ones we want to provide and what to name them.

alecthomas · 2024-08-19T03:53:53Z

backend/controller/ingress/ingress.go

 			bodyField = field
 			break
 		}
 	}

 	if bodyField == nil {
-		return nil, fmt.Errorf("verb %s must have a 'body' field", ref.Name)
+		return nil, fmt.Errorf("verb %s must have a '%s' field", ref.Name, name)


tip: use %q for quoted strings rather than '%s'

alecthomas · 2024-08-19T03:54:29Z

backend/controller/ingress/ingress_integration_test.go

@@ -118,7 +118,7 @@ func TestHttpIngress(t *testing.T) {
 			assert.Equal(t, []string{"application/json; charset=utf-8"}, resp.Headers["Content-Type"])
 			assert.Equal(t, in.JsonData(t, []in.Obj{{"item": "a"}, {"item": "b"}}), resp.BodyBytes)
 		}),
-		in.HttpCall(http.MethodGet, "/typeenum", nil, in.JsonData(t, in.Obj{"name": "A", "value": "hello"}), func(t testing.TB, resp *in.HTTPResponse) {
+		in.HttpCall(http.MethodPost, "/typeenum", nil, in.JsonData(t, in.Obj{"name": "A", "value": "hello"}), func(t testing.TB, resp *in.HTTPResponse) {


Why the method changes?

Ah because of the request body in the test.

alecthomas · 2024-08-19T05:06:55Z

backend/controller/ingress/request_test.go

+				Path:   "/multiMapQuery",
+				Query:  map[string][]string{"alias": []string{"value"}},
+			},
+		},


Is there a test for mapping query and path parameters onto structs?

Yes, there were existing tests that were mapping them to a body struct, these have been changed so now tests are mapping them onto the path or query param struct with a Unit body.

backend/schema/validate.go

alecthomas · 2024-08-19T05:28:14Z

backend/schema/validate.go

+	case *String, *Data, *Unit, *Float, *Int, *Bool, *Map: // Valid HTTP param payload types.
+	case *TypeAlias:
+		// allow aliases of external types
+		if len(t.Metadata) > 0 {


This should check that the metadata is of the expected type, MetadataTypeMap. There's a helper function (that I cannot for the life of me find at the moment) that can extract a concrete type from a list of sum types if it exists.

This was part of the original validation logic, I have updated it.

Aaah, thanks @stuartwdouglas

alecthomas · 2024-08-19T05:28:24Z

backend/schema/validate.go

 	switch t := n.(type) {
 	case *Bytes, *String, *Data, *Unit, *Float, *Int, *Bool, *Map, *Array: // Valid HTTP response payload types.
 	case *TypeAlias:
 		// allow aliases of external types
 		if len(t.Metadata) > 0 {
 			return nil
 		}
-		return validatePayloadType(t.Type, r, v, reqOrResp)
+		return validateBodyPayloadType(t.Type, r, v, reqOrResp)


alecthomas · 2024-08-19T05:36:59Z

backend/schema/validate.go

+	case *Data, *Unit, *Map: // Valid HTTP query payload types.
+	case *TypeAlias:
+		// allow aliases of external types
+		if len(t.Metadata) > 0 {


backend/schema/validate_test.go

alecthomas · 2024-08-19T05:38:47Z

docs/content/docs/reference/ingress.md

+```go
+
+//ftl:ingress GET /http/users/{userId}/posts
+func Get(ctx context.Context, req builtin.HttpRequest[ftl.Unit, int64, GetRequestQueryParams]) (builtin.HttpResponse[GetResponse, ErrorResponse], error) {


gak · 2024-08-19T05:45:25Z

backend/controller/ingress/request.go

+			return float, nil
+		case *schema.Bool:
+			// TODO: is anything else considered truthy?
+			return val == "true", nil


Should we use an LLM here to decide?

backend/controller/ingress/request.go

gak · 2024-08-19T05:52:25Z

backend/schema/validate.go

+	queryParamSymbol Symbol
+}
+
+func validateIngressResponse(scopes Scopes, module *Module, n *Verb, reqOrResp string, r Type) (merr []error) {


Could reqOrResp be a bool or enum?

It's mostly used for constructing error messages, it was part of the existing code.

fixes: #2230

wesbillman · 2024-08-19T15:19:21Z

This is awesome! Thanks @stuartwdouglas!

wesbillman · 2024-08-19T15:15:23Z

backend/controller/ingress/testdata/go/httpingress/httpingress.go

 	return builtin.HttpResponse[ftl.Unit, string]{Body: ftl.Some(ftl.Unit{})}, nil
 }

-//ftl:ingress http GET /string
-func String(ctx context.Context, req builtin.HttpRequest[string]) (builtin.HttpResponse[string, string], error) {
+//ftl:ingress http POST /string


Why did these change to POST?

They have a body, and GET requests with a body don't make sense.

fixes: #2230

stuartwdouglas added the run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue label Aug 19, 2024

stuartwdouglas requested a review from alecthomas as a code owner August 19, 2024 00:35

stuartwdouglas requested review from a team and gak and removed request for a team August 19, 2024 00:35

ftl-robot mentioned this pull request Aug 19, 2024

Dashboard #728

Open

stuartwdouglas force-pushed the stuartwdouglas/2230 branch 9 times, most recently from 1fbd7de to 668d0b0 Compare August 19, 2024 05:25

alecthomas approved these changes Aug 19, 2024

View reviewed changes

gak reviewed Aug 19, 2024

View reviewed changes

backend/controller/ingress/request.go Show resolved Hide resolved

gak reviewed Aug 19, 2024

View reviewed changes

gak approved these changes Aug 19, 2024

View reviewed changes

feat: HttpRequest path and query mapping

d422542

fixes: #2230

stuartwdouglas force-pushed the stuartwdouglas/2230 branch from 668d0b0 to d422542 Compare August 19, 2024 07:18

stuartwdouglas enabled auto-merge August 19, 2024 07:19

stuartwdouglas added this pull request to the merge queue Aug 19, 2024

Merged via the queue into main with commit 826dd3f Aug 19, 2024
74 checks passed

stuartwdouglas deleted the stuartwdouglas/2230 branch August 19, 2024 07:30

wesbillman reviewed Aug 19, 2024

View reviewed changes

safeer pushed a commit that referenced this pull request Aug 19, 2024

feat: HttpRequest path and query mapping (#2422)

0e5ef15

fixes: #2230

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: HttpRequest path and query mapping #2422

feat: HttpRequest path and query mapping #2422

stuartwdouglas commented Aug 19, 2024 •

edited

Loading

alecthomas left a comment

alecthomas Aug 19, 2024

stuartwdouglas Aug 19, 2024

alecthomas Aug 19, 2024

alecthomas Aug 19, 2024

alecthomas Aug 19, 2024

alecthomas Aug 19, 2024

stuartwdouglas Aug 19, 2024

alecthomas Aug 19, 2024

stuartwdouglas Aug 19, 2024

alecthomas Aug 19, 2024

alecthomas Aug 19, 2024

alecthomas Aug 19, 2024

alecthomas Aug 19, 2024

gak Aug 19, 2024

gak Aug 19, 2024

gak Aug 19, 2024

stuartwdouglas Aug 19, 2024

wesbillman commented Aug 19, 2024

wesbillman Aug 19, 2024

stuartwdouglas Aug 19, 2024

feat: HttpRequest path and query mapping #2422

feat: HttpRequest path and query mapping #2422

Conversation

stuartwdouglas commented Aug 19, 2024 • edited Loading

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

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

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

wesbillman commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartwdouglas commented Aug 19, 2024 •

edited

Loading