Allow ClientContext.Custom unmarshaling for non-string (JSON) values#620
Allow ClientContext.Custom unmarshaling for non-string (JSON) values#620
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
==========================================
- Coverage 74.94% 73.99% -0.96%
==========================================
Files 36 36
Lines 1401 1419 +18
==========================================
Hits 1050 1050
- Misses 273 291 +18
Partials 78 78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if err := json.Unmarshal(v, &s); err == nil { | ||
| cc.Custom[k] = s | ||
| } else { | ||
| cc.Custom[k] = string(v) |
There was a problem hiding this comment.
Maybe you can add a test when JSON parsing is failing? (I think a non-valid JSON will trigger that code path?)
| // This handles the case where values in the "custom" map are not strings | ||
| // (e.g. nested JSON objects), by serializing non-string values back to | ||
| // their JSON string representation. | ||
| func (cc *ClientContext) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
Thinking there may need to be a MarshalJSON too, so that a re-serialization produces the same output as what was input. (number, bool values. quote escaping in nested json)
There was a problem hiding this comment.
see: https://go.dev/play/p/8q6bgWa-Rcw where re marshaling results in a type change
// You can edit this code!
// Click here and start typing.
package main
import (
"encoding/json"
"fmt"
"log"
"log/slog"
"os"
)
type X struct {
Custom map[string]string
}
func (x *X) UnmarshalJSON(b []byte) error {
var m struct{ Custom map[string]json.RawMessage }
if err := json.Unmarshal(b, &m); err != nil {
return err
}
x.Custom = make(map[string]string, len(m.Custom))
for k, v := range m.Custom {
var s string
if err := json.Unmarshal(v, &s); err != nil {
slog.Warn("uhoh", "err", err, "key", k, "val", v)
s = string(v)
}
x.Custom[k] = s
}
return nil
}
func main() {
input := []byte(`{"Custom":{"hello":9001}}`)
fmt.Println("Input:")
os.Stdout.Write(input)
fmt.Println("")
var x X
if err := json.Unmarshal(input, &x); err != nil {
log.Fatal(err)
}
output, err := json.Marshal(x)
if err != nil {
log.Fatal(err)
}
fmt.Println("Output:")
os.Stdout.Write(output)
}
Input:
{"Custom":{"hello":9001}}
2009/11/10 23:00:00 WARN uhoh err="json: cannot unmarshal number into Go value of type string" key=hello val="9001"
Output:
{"Custom":{"hello":"9001"}}
Program exited.
There was a problem hiding this comment.
interesting. Java and .NET already be mangling this with string conversions for numbers and bools. And .NET only will stringify nested values.
Client Context custom — Return Value Test Matrix
All functions return context.clientContext.custom as JSON.
Test 1: {"custom": {"number": 9001, "bool": false}}
| Runtime | Result |
|---|---|
| Java | ✅ {"number":"9001","bool":"false"} |
| .NET | ✅ {"number":"9001","bool":"False"} |
| Rust | ❌ Runtime crash (logs: invalid type: integer '9001', expected a string) |
| Go | ❌ cannot unmarshal number into Go struct field ClientContext.custom of type string |
Test 2: {"custom": {"nested": {"hello": "world"}}}
| Runtime | Result |
|---|---|
| Java | ❌ Expected a string but was BEGIN_OBJECT |
| .NET | ✅ {"nested":"{\"hello\":\"world\"}"} |
| Rust | ❌ Runtime crash (logs: invalid type: map, expected a string) |
| Go | ❌ cannot unmarshal object into Go struct field ClientContext.custom of type string |
|
|
|
mm yeah java don't like nested either |
| var s string | ||
| if err := json.Unmarshal(v, &s); err == nil { | ||
| cc.Custom[k] = s | ||
| } else { |
There was a problem hiding this comment.
Is the nested json in the Client Context like, critical to all use of the Bedrock Agentcore Gateway service? Rather than stringifying, another option is to drop non-string values.
var raw struct { Custom map[string]any }
json.Unmarshal(b, &raw)
for k, v := range raw.Custom {
if s, ok := v.(string); ok {
cc.Custom[k] = s
}
}
Bringing this option up, so as not to be hasty in type coercion decisions that could introduce inconsistencies with other runtimes.
There was a problem hiding this comment.
Can you elaborate on why you would drop types other than a string? It seems possible that this would bite you down the road if another service was enabled that needed this but had values other than strings.
There was a problem hiding this comment.
Balancing urgency in (partially?) unblocking a use-case, and introducing warts that'd be hard to undo without semver bumps. Other runtimes have the same blocker, and it'd be prudent to line up a similar strategy for supporting the usecase.
For Go, other options that solve for making the context parse more lenient, without committing to inconsistent type coercion for this novel use of client context:
- Assign
LambdaContext.ClientContext.Customtonilif parse fails,slog.Warnthe error. - Move the
LambdaContextparse step from pre-invoke to being done lazily only when handlers fetch it usinglambdacontext.FromContext(ctx)(and logging errors, returningnil, false)
An option to make the novel use of Client Context readable in a way consistent with node, python, (eg: no type coercion) could be something like stuffing the []byte from the client context header into the context.Context pre-invoke. Combine with swallowing the parse error (set Custom to nil), and a deprecation comment on the ClientContext.Custom field // Deprecated: Custom is 'nil' when client sends something other than a map[string]string. Use `lambdacontext.ClientContextCustomFromContext` instead
and adding new function
ClientContextCustomFromContext[T](ctx context.Context) (*T, error) {
b, ok := ctx.Value("lambda-client-context-header-value").([]byte)
if !ok {
return nil errors.New("ClientContext not found in context)
}
var t T
if err := json.Unmarshal(lc.ClientContext.CustomRaw, &t); err != nil {
return nil, err
}
return &t, nil
}
^ is my general recommendation to keep back compat while also unblocking the new use-case. But given that .NET and Java are already inconsistent this isn't necessarily the only solution.
| // their JSON string representation. | ||
| func (cc *ClientContext) UnmarshalJSON(data []byte) error { | ||
| var raw struct { | ||
| Client ClientApplication `json:"Client"` |
There was a problem hiding this comment.
typo, should be "client". Well actually, no struct tag. without the json: struct tag, the current ClientContext is lenient to both "Custom" and "custom". The mobile SDKs I belive all strictly send "custom", and .NET, java, Rust, all drop "Custom", so IDK why this was left lenient 🤷♂️ - but I guess should also stay lenient for back compat
nevermind
When parsing a JSON object into a Go struct, keys are considered in a case-insensitive fashion.
5 participants
Issue #, if available: -
Description of changes:
When AWS services like Bedrock Agentcore Gateway send nested JSON objects as values in
ClientContext.custom(e.g. propagated headers), the Go Lambda runtime fails with:cannot unmarshal object into Go struct field ClientContext.custom of type string. This blocks all Go Lambda functions used with Agentcore Gateway propagated headers.Root cause
ClientContext.Customismap[string]string, which rejects non-string JSON values duringjson.UnmarshalinparseClientContext()before the handler is ever invoked.How this Fixes that
Add a custom
UnmarshalJSONtoClientContextthat parses Custom values viajson.RawMessage. String values are stored directly (backward compatible) while Non-string values (objects, arrays) are serialized to their JSON string representation instead of failing.Fully backward compatible - no struct signature changes. All existing tests pass.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.