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: add array support to http ingress #915

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Conversation

wesbillman
Copy link
Collaborator

No description provided.

Comment on lines 70 to 77
fmt.Printf("resp: %v\n", resp)

respdata, err := encoding.Marshal(resp)
if err != nil {
return nil, err
}

fmt.Printf("respdata: %v\n", string(respdata))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alecthomas I might need to pick your brain on this one. It seems like when we encode arrays of strings, they are getting double quoted here. Not sure if that's what we want, or if I need to make a change to the encoder. It also causes code after this to fail since it can't probably json encode the response for recordCall in observability.

This is what is printed out when I run the integration tests for HTTP Ingress.

resp: {0 map[] ["hello" "world"] None}
respdata: {"status":0,"headers":{},"body":[""hello"",""world""],"error":null}

Copy link
Collaborator

@alecthomas alecthomas Feb 9, 2024

Choose a reason for hiding this comment

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

Use repr.Println() as it will tell you what the values actually are. %v is useless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yeah!

resp:

builtin.HttpRequest[[]string]{
  Method: "GET",
  Path: "/array/string",
  PathParameters: map[string]string{  },
  Query: map[string][]string{  },
  Headers: map[string][]string{
    "Accept-Encoding": []string{
      "gzip",
    },
    "Content-Length": []string{
      "17",
    },
    "Content-Type": []string{
      "application/json",
    },
    "User-Agent": []string{
      "Go-http-client/1.1",
    },
  },
  Body: []string{
    "\"hello\"",
    "\"world\"",
  },
}

respdata:

[]byte("{\"status\":0,\"headers\":{},\"body\":[\"\"hello\"\",\"\"world\"\"],\"error\":null}")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that before or after? Whatever it is, it should tell you that either the strings are coming in already with extra quotes, or they're getting added by encode. That should point you closer to where the problem lies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the heads up on this one! The issue was that the data in the req was already encoded incorrectly and since it's just returned in the Body: req.Body the response was also encoded incorrectly. Garbage in, garbage out. 🤦

@wesbillman wesbillman force-pushed the http-ingress-array-support branch 4 times, most recently from 551e814 to 9a4d3e0 Compare February 12, 2024 20:33
Comment on lines 157 to 161
// httpCall(rd, http.MethodPost, "/array/data", jsonData(t, []obj{{"item": "a"}, {"item": "b"}}), func(t testing.TB, resp *httpResponse) {
// assert.Equal(t, 200, resp.status)
// assert.Equal(t, []string{"application/json; charset=utf-8"}, resp.headers["Content-Type"])
// assert.Equal(t, jsonData(t, []obj{{"item": "a"}, {"item": "b"}}), resp.bodyBytes)
// }),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alecthomas I have this one commented out because it seems like Arrays of DataRefs fail to monomorphize correctly. I took a stab at fixing it in data.go, but I think I was causing more harm than good 😂

Maybe we can take a peek together on this one.

It seems to happen when monomorphising to get the body of the request object.

func getBodyField(dataRef *schema.DataRef, sch *schema.Schema) (*schema.Field, error) {
	data, err := sch.ResolveDataRefMonomorphised(dataRef)
	if err != nil {
		return nil, err
	}

@wesbillman wesbillman force-pushed the http-ingress-array-support branch from 9a4d3e0 to c2cfcec Compare February 12, 2024 22:13
@wesbillman wesbillman marked this pull request as ready for review February 12, 2024 22:14
}

// Transform the array element to its aliased form.
if dataRef, ok := t.Element.(*schema.DataRef); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this not call bodyForType recursively? What if you have an array of strings? Or an array of arrays?

@wesbillman wesbillman force-pushed the http-ingress-array-support branch from fcd04dc to 54c2a4c Compare February 13, 2024 16:05
@wesbillman wesbillman force-pushed the http-ingress-array-support branch from 54c2a4c to 321d49c Compare February 13, 2024 16:06
@wesbillman wesbillman force-pushed the http-ingress-array-support branch from 321d49c to 6a2ec0c Compare February 13, 2024 16:10
Comment on lines +44 to +63
// Since the query and header parameters are a `map[string][]string`
// we need to convert them before they go through the `transformFromAliasedFields` call
// otherwise they will fail the type check.
queryMap := make(map[string]any)
for key, values := range r.URL.Query() {
valuesAny := make([]any, len(values))
for i, v := range values {
valuesAny[i] = v
}
queryMap[key] = valuesAny
}

headerMap := make(map[string]any)
for key, values := range r.Header {
valuesAny := make([]any, len(values))
for i, v := range values {
valuesAny[i] = v
}
headerMap[key] = valuesAny
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if there's a better way to handle this that will keep the type checker happy. But since they query and headers types don't match map[string]any and their values don't pass []any the transformFromAliasedFields will fail.

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 that's about all you can do here yeah.

@wesbillman wesbillman merged commit 2c5f3fe into main Feb 13, 2024
11 checks passed
@wesbillman wesbillman deleted the http-ingress-array-support branch February 13, 2024 16:27
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.

2 participants