Repository navigation
Revert "Remove the legacy SParse JSON functions" - #2841
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@inimaga - you will need to make an auth PR before re-creating the reverted Bedrock PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 697025d4c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| STable data = SParseJSONObject(job["data"]); | ||
| data["mockRequest"] = "true"; | ||
| job["data"] = SComposeJSONObject(data); |
There was a problem hiding this comment.
For mocked jobs with non-empty data, converting the parsed object to STable and recomposing it loses JSON type information: for example, {"number":"123","literal":"null"} is stored as {"number":123,"literal":null,"mockRequest":true} because SComposeJSONObject infers scalar types from strings. This silently changes the worker payload for any mocked CreateJob/CreateJobs request; mutate the typed JSON object instead.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Verify that name is present for every job | ||
| if (!SContains(job, "name")) { |
There was a problem hiding this comment.
job is the raw JSON text here, so this only performs a substring search rather than checking for a top-level member. A request such as [{"data":{"name":"foo"}}] passes validation and later creates a job whose name is the default empty string, despite name being required; check jobObject instead.
Useful? React with 👍 / 👎.
| const char* parseEnd = _SParseJSONValue(ptr, end, ignore, false, "null"); | ||
| if (parseEnd == end) { // Parsed it all. | ||
| return value; |
There was a problem hiding this comment.
The restored parser accepts malformed containers such as [1,] because _SParseJSONValue makes no progress for the missing value after the comma and the array parser then accepts the closing bracket. Consequently SToJSON("[1,]") returns invalid JSON verbatim; any arbitrary string field with that shape, such as a job name, can make the enclosing SComposeJSONObject response unparsable.
Useful? React with 👍 / 👎.
| return "\"" + SEscape(value, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" | ||
| "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f\x7f\"\\/", | ||
| '\\') + |
There was a problem hiding this comment.
The string overload of SEscape delegates to the C-string overload without a length, so it stops at the first embedded NUL. Values obtained from valid JSON such as "abc\u0000def" therefore serialize as only "abc", silently dropping the suffix whenever they pass through SToJSON or SComposeJSONObject; use the length-aware overload and encode the NUL.
Useful? React with 👍 / 👎.
| auto array = SParseJSONArray(json); | ||
| if (array.empty()) { | ||
| // Nothing here. | ||
| return true; |
There was a problem hiding this comment.
SParseJSONArray returns an empty list both for [] and for parse failures, and this branch treats both as successful empty results. For example, SQLite JSON output for SELECT 1e999 AS value contains the non-JSON token Inf; deserialization now reports success with no columns, so the MySQL path sends a successful zero-column result instead of an error.
Useful? React with 👍 / 👎.
Noted @tylerkaraszewski. Thanks for handling while I was out. |
Reverts #2810
It broke auth tests. Auth tests will need to be updated to work with this.