feat: add support for ingress path params #652

wesbillman · 2023-11-24T23:48:11Z

Given an ingress path defined as

//ftl:verb
//ftl:ingress GET /productcatalog/{id}/date/{year}/{month}/{day}

We get the following from the controller

controller | info: GET /productcatalog/1234/date/2022/12/12
controller | info: Path params: map[day:12 id:1234 month:12 year:2022]

With this curl request:

curl -i http://localhost:8892/ingress/productcatalog/1234/date/2022/12/12

We can locate the record in the ingress_routes table and route to the correct verb. This needs more work and testing, but we are starting to build out the basics here to get feedback.

wesbillman · 2023-11-24T23:49:34Z

backend/controller/sql/queries.sql

 FROM ingress_routes ir
         INNER JOIN runners r ON ir.deployment_id = r.deployment_id
 WHERE r.state = 'assigned'
  AND ir.method = $1
-  AND ir.path = $2;
+  AND ir.path LIKE CONCAT(sqlc.arg('path')::TEXT, '%');


Not sure if this is good, or a bit too hacky. Open to other ideas here. Basically, I need to find routes that start with the path I'm looking for and then filter it down later based on the actual request that was made on the client.

I don't think there's a good solution that uses the DB (or at least, not one I can think of). I think we'll just need to load the entire ingress table into memory periodically, and do the matching in code.

wesbillman · 2023-11-24T23:50:24Z

backend/controller/controller.go

+	pathParams := getPathParams(route.Path, r.URL.Path)
+	logger.Infof("Path params: %v", pathParams)


Not sure where best to put these so that they can be used/retrieved from the verb code. I could add it to the ctx and have a PathParamsWithContext(ctx) or something.

wesbillman · 2023-11-24T23:50:51Z

backend/controller/controller.go

@@ -201,8 +204,9 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	creq := connect.NewRequest(&ftlv1.CallRequest{
-		Verb: &schemapb.VerbRef{Module: route.Module, Name: route.Verb},
-		Body: body,
+		Metadata: &ftlv1.Metadata{},


I also might be able to add it to the Metadata here or something.

I think we'll combine the path parameters with query parameters, and map them onto the request structure. That's the approach gRPC uses, and I think it makes sense.

coolio! Maybe we can chat today on this one. Want to make sure we have request validation in the right place. For example, if we add a RequestParams arg that has either the path params or query params here, when we create the actual verb request, we'll need a way to know if we should use the RequestParams for the <verb>Request.

It seems like we currently put the query params into the body of the payload after we json.Marshal it. So maybe the path parameters will always be in the RequestParams struct and query params will always end up in the body? I'm guessing we can't specify query params in the ingress annotation, yeah?

Off the top of my head, I think it should be in the schema validator.

alecthomas

Looking good!

alecthomas · 2023-11-27T02:57:29Z

backend/controller/controller.go

@@ -201,8 +204,9 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	creq := connect.NewRequest(&ftlv1.CallRequest{
-		Verb: &schemapb.VerbRef{Module: route.Module, Name: route.Verb},
-		Body: body,
+		Metadata: &ftlv1.Metadata{},


I think we'll combine the path parameters with query parameters, and map them onto the request structure. That's the approach gRPC uses, and I think it makes sense.

alecthomas · 2023-11-27T03:01:31Z

backend/controller/ingress.go

+)
+
+func (s *Service) getIngressRoute(ctx context.Context, method string, path string) (*dal.IngressRoute, error) {
+	pathStart := strings.Split(path, "/")[1]


Won't this be invalid if the user has something like /{foo}, or does path include the /ingress prefix?

alecthomas · 2023-11-27T03:11:15Z

backend/controller/ingress.go

+
+	params := make(map[string]string)
+	for i, segment := range formatSegments {
+		if strings.HasPrefix(segment, "{") && strings.HasSuffix(segment, "}") {


I think this should be checking that the non-parameter segments match too yeah?

alecthomas · 2023-11-27T03:14:30Z

backend/controller/ingress.go

+	return params, nil
+}
+
+func matchURL(pattern, urlPath string) bool {


It would be good to not have two functions doing almost the exact same thing. Perhaps just have one that takes a closure with any matching path variables, eg.

func matchURL(pattern, urlPath string, matched func(key, value)) bool

alecthomas · 2023-11-27T03:14:47Z

backend/controller/ingress_test.go

+	"github.com/alecthomas/errors"
+)
+
+func TestMatchURL(t *testing.T) {


alecthomas · 2023-11-27T03:15:28Z

backend/controller/ingress_test.go

+
+	for _, test := range tests {
+		actual, err := getPathParams(test.pattern, test.urlPath)
+		if !areMapsEqual(actual, test.expected) {


Use assert.Equal()

alecthomas · 2023-11-27T03:16:05Z

backend/controller/ingress_test.go

+		if !areMapsEqual(actual, test.expected) {
+			t.Errorf("getPathParams(%q, %q) = %v, expected %v", test.pattern, test.urlPath, actual, test.expected)
+		}
+		if (err != nil && test.err == nil) || (err == nil && test.err != nil) || (err != nil && test.err != nil && err.Error() != test.err.Error()) {


Use:

if test.err != nil { assert.EqualError(t, test.err, err) } else { assert.NoError(t, err) }

Also usually put the error check before the value check.

alecthomas · 2023-11-27T03:18:25Z

backend/controller/ingress_test.go

+
+	for _, test := range tests {
+		actual := matchURL(test.pattern, test.urlPath)
+		if actual != test.expected {


Use assert.Equal()

There's optional printf-style formatting at the end that you can use to display a nice error eg.

assert.Equal(t, test.expected, actual, "pattern = %s, urlPath = %s", test.pattern, test.urlPath)

alecthomas · 2023-11-27T03:22:34Z

backend/controller/sql/queries.sql

 FROM ingress_routes ir
         INNER JOIN runners r ON ir.deployment_id = r.deployment_id
 WHERE r.state = 'assigned'
  AND ir.method = $1
-  AND ir.path = $2;
+  AND ir.path LIKE CONCAT(sqlc.arg('path')::TEXT, '%');


I don't think there's a good solution that uses the DB (or at least, not one I can think of). I think we'll just need to load the entire ingress table into memory periodically, and do the matching in code.

backend/controller/sql/queries.sql

alecthomas · 2023-12-05T00:29:56Z

backend/controller/controller.go

-	routes, err := s.dal.GetIngressRoutes(r.Context(), r.Method, r.URL.Path)
+	routes, err := s.dal.GetIngressRoutes(r.Context(), r.Method)
+	if err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)


I think you also want the if errors.Is(err, dal.ErrNotFound) check here too.

alecthomas · 2023-12-05T03:09:16Z

backend/controller/ingress/ingress.go

+	return &route, nil
+}
+
+func matchAndExtractAllSegments(pattern, urlPath string, onMatch func(segment, value string)) bool {


Should this be called something different? Like just matchSegments()?

I like that!

alecthomas · 2023-12-05T03:09:50Z

backend/controller/ingress/ingress.go

+	return true
+}
+
+func ValidateAndExtractBody(route *dal.IngressRoute, r *http.Request, sch *schema.Schema) ([]byte, error) {


A doc comment here would be good.

alecthomas · 2023-12-05T03:11:25Z

backend/controller/ingress/ingress.go

+		var bodyMap map[string]any
+		err := json.NewDecoder(r.Body).Decode(&bodyMap)
+		if err != nil {
+			return nil, err


I think add an fmt.Errorf("HTTP request body is not valid JSON: %w", err) so the returned error is a bit more useful.

alecthomas · 2023-12-05T03:11:38Z

backend/controller/ingress/ingress.go

+			requestMap[k] = v
+		}
+	default:
+		// TODO: Support query params correctly for map and array


This pained me 😂

wesbillman commented Nov 24, 2023

View reviewed changes

wesbillman force-pushed the handle-ingress-path-parameters branch from 54cf08b to 02135eb Compare November 25, 2023 01:10

alecthomas reviewed Nov 27, 2023

View reviewed changes

wesbillman force-pushed the handle-ingress-path-parameters branch 3 times, most recently from 094e145 to 64d57e1 Compare December 1, 2023 21:59

wesbillman marked this pull request as ready for review December 4, 2023 18:53

wesbillman requested a review from dhanji as a code owner December 4, 2023 18:53

wesbillman force-pushed the handle-ingress-path-parameters branch 2 times, most recently from 40f11eb to 859f2f8 Compare December 5, 2023 00:28

alecthomas approved these changes Dec 5, 2023

View reviewed changes

feat: add support for ingress path params

47f5aab

wesbillman force-pushed the handle-ingress-path-parameters branch from 2854cd5 to 47f5aab Compare December 5, 2023 03:48

chore(autofmt): Automated formatting

63f98de

wesbillman merged commit d9404b4 into main Dec 5, 2023

wesbillman deleted the handle-ingress-path-parameters branch December 5, 2023 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for ingress path params #652

feat: add support for ingress path params #652

wesbillman commented Nov 24, 2023

wesbillman Nov 24, 2023

alecthomas Nov 27, 2023

wesbillman Nov 24, 2023

wesbillman Nov 24, 2023

alecthomas Nov 27, 2023

wesbillman Nov 29, 2023 •

edited

Loading

alecthomas Nov 29, 2023

alecthomas left a comment

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Nov 27, 2023

alecthomas Dec 5, 2023

alecthomas Dec 5, 2023

wesbillman Dec 5, 2023

alecthomas Dec 5, 2023

alecthomas Dec 5, 2023

alecthomas Dec 5, 2023

wesbillman Dec 5, 2023

		pathParams := getPathParams(route.Path, r.URL.Path)
		logger.Infof("Path params: %v", pathParams)

feat: add support for ingress path params #652

feat: add support for ingress path params #652

Conversation

wesbillman commented Nov 24, 2023

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 Nov 29, 2023 • 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

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 Nov 29, 2023 •

edited

Loading