Improve custom/complex argument JSON parsing for chip-tool. #19530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns and
msandstedt
pullapprove
bot
requested review from
mlepage-google,
mrjerryjohns,
msandstedt,
mspang,
rgoliver,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tecimovic,
tehampson,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
bzbarsky-apple
force-pushed
the
better-json-parsing
branch
from
bba82ca to
c2e50d7
Compare
Specific changes:
1) Use CharReaderBuilder/CharReader, because that allows us to set 'failIfExtra'
to true, so we fail out on trailing garbage.
2) Since now we can, set 'allowSingleQuotes' to true, so people can use single
quotes in their JSON if that's more convenient.
3) Add checking for duplicated keys by setting rejectDupKeys to true.
4) For complex arguments, enforce that the value is in fact an array or object,
not a primitive value.
5) Factour out the parsing logic from ComplexArgument so we can use it in
CustomArgument too.
6) Make CustomArgument fail out on JSON parse errors instead of silently doing
unexpected things.
7) For CustomArgument, detect an argument starting with one of our type prefixes
("hex:", "i:", "u:", "f:", "d:") and if so just treat it as a string value
instead of trying to do a full JSON parse (which would fail anyway).
8) For CustomArgument, detect an argument starting with "0x" and treat it as a
string starting "u:0x", so it will be parsed as a hex number instead of just
failing to parse.
This leads to the following behavior:
./chip-tool any write-by-id 0x130AFC01 0x130A0001 123 17 1
Before: sends 123 as signed int.
After: same.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 0x123 17 1
Before: sends 0.
After: sends 291 as unsigned int.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 u:123 17 1
Before: sends null.
After: sends 123 as unsigned int.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 hex:121314 17 1
Before: sends null.
After: sends byte string with bytes 0x12, 0x13, 0x14.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 "[1, 2, 3], [4, 5, 6]" 17 1
Before: sends list [1, 2, 3].
After: errors out on trailing garbage.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 "[1 2, 3]" 17 1
Before: sends list [1].
After: errors out with "Missing ',' or ']' in array declaration".
./chip-tool accesscontrol write acl '[{"privilege": 5, "authMode": 2, "subjects": [112233], "targets": [{"cluster": null, "endpoint": null, "deviceType": 0x100}]}]' 17 0
Before: Errors out on the invalid hex number not in quotes, shows the error location.
After: same.
./chip-tool accesscontrol write acl '[{"privilege": 5, "authMode": 2, "subjects": [112233], "targets": []}], [{"targets": [{"cluster": null, "endpoint": null, "deviceType": 0x100}]}]' 17 0
Before: sends the first array and ignores the second one.
After: errors out on trailing garbage.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 '{"1": 0x100 }' 17 1
Before: Sends {"1": 0}
After: Parse error on invalid hex.
./chip-tool any write-by-id 0x130AFC01 0x130A0001 '{"1": "u:0x100" }' 17 1
Before: sends {"1": 256}
After: same.
Fixes project-chip#19529
bzbarsky-apple
force-pushed
the
better-json-parsing
branch
from
c2e50d7 to
c77c66e
Compare
|
PR #19530: Size comparison from 0e9b2cc to c77c66e Increases above 0.2%:
Increases (2 builds for linux)
Decreases (3 builds for cyw30739, linux)
Full report (34 builds for cc13x2_26x2, cyw30739, k32w, linux, mbed, nrfconnect, p6, telink)
|
tcarmelveilleux
approved these changes
Jun 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specific changes:
to true, so we fail out on trailing garbage.
quotes in their JSON if that's more convenient.
not a primitive value.
CustomArgument too.
unexpected things.
("hex:", "i:", "u:", "f:", "d:") and if so just treat it as a string value
instead of trying to do a full JSON parse (which would fail anyway).
string starting "u:0x", so it will be parsed as a hex number instead of just
failing to parse.
This leads to the following behavior:
Before: sends 123 as signed int.
After: same.
Before: sends 0.
After: sends 291 as unsigned int.
Before: sends null.
After: sends 123 as unsigned int.
Before: sends null.
After: sends byte string with bytes 0x12, 0x13, 0x14.
Before: sends list [1, 2, 3].
After: errors out on trailing garbage.
Before: sends list [1].
After: errors out with "Missing ',' or ']' in array declaration".
Before: Errors out on the invalid hex number not in quotes, shows the error location.
After: same.
Before: sends the first array and ignores the second one.
After: errors out on trailing garbage.
Before: Sends {"1": 0}
After: Parse error on invalid hex.
Before: sends {"1": 256}
After: same.
Fixes #19529
Problem
Sending garbage instead of failing out on invalid JSON for custom arguments. Not reporting trailing garbage for complex arguments.
Change overview
See above.
Testing
See above for list of things tested.