Add strict checks to reference client to validate a server's connect error and end stream JSON #790
Conversation
jhump
force-pushed
the
jh/connect-checks
branch
from
4700764 to
2fc3ad9
Compare
jhump
force-pushed
the
jh/connect-checks
branch
from
2fc3ad9 to
b872a4d
Compare
|
@smaye81, this one's pretty big. Sorry about that. Though the test cases are a pretty big chunk of the lines changed. But there's also a non-trivial amount of logic to closely scrutinize the JSON data and complain about it. |
| var endStream connectEndStream | ||
| if err := json.Unmarshal(endStreamJSON, &endStream); err != nil { | ||
| printer.Printf("connect end stream JSON is malformed: %v", err) | ||
| return | ||
| } | ||
| // Extra checks, since encoding/json above is lenient. | ||
| if _, err := checkNoDuplicateKeys("", json.NewDecoder(bytes.NewReader(endStreamJSON))); err != nil { | ||
| printer.Printf("connect end stream JSON: %v", err) | ||
| return | ||
| } | ||
| var asAny map[string]any | ||
| if err := json.Unmarshal(endStreamJSON, &asAny); err != nil { | ||
| // note: since above unmarshal step succeeded, this should never fail | ||
| printer.Printf("connect end stream JSON is malformed: %v", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Is it worth moving out this logic into its own function? I noticed that examineConnectError and examineConnectErrorDetail perform very similar logic (albeit with some minor variations). All 3 seem to do a series of validation steps against a json.RawMessage and then use an internal.Printer to report an error. It may not even be worth it, but just an observation.
There was a problem hiding this comment.
Interesting point. I looked and was able to consolidate this into an examineJSON helper function. Just pushed a change with that.
This makes the checks significantly more strict than the base connect-go implementation, in order to report a number of possible issues in server JSON responses:
UnmarshalJSONbehavior of each type to explicitly check. 😦