-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Improve custom/complex argument JSON parsing for chip-tool. (#19530)
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 #19529- Loading branch information
1 parent
8b8f695
commit fb7de2a
Showing
3 changed files
with
190 additions
and
52 deletions.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| /* | ||
| * Copyright (c) 2022 Project CHIP Authors | ||
| * All rights reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <json/json.h> | ||
| #include <lib/core/Optional.h> | ||
|
|
||
| #include <memory> | ||
| #include <sstream> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| class JsonParser | ||
| { | ||
| public: | ||
| // Returns whether the parse succeeded. | ||
| static bool ParseComplexArgument(const char * label, const char * json, Json::Value & value) | ||
| { | ||
| return Parse(label, json, /* strictRoot = */ true, value); | ||
| } | ||
|
|
||
| // Returns whether the parse succeeded. | ||
| static bool ParseCustomArgument(const char * label, const char * json, Json::Value & value) | ||
| { | ||
| return Parse(label, json, /* strictRoot = */ false, value); | ||
| } | ||
|
|
||
| private: | ||
| static bool Parse(const char * label, const char * json, bool strictRoot, Json::Value & value) | ||
| { | ||
| Json::CharReaderBuilder readerBuilder; | ||
| readerBuilder.settings_["strictRoot"] = strictRoot; | ||
| readerBuilder.settings_["allowSingleQuotes"] = true; | ||
| readerBuilder.settings_["failIfExtra"] = true; | ||
| readerBuilder.settings_["rejectDupKeys"] = true; | ||
|
|
||
| auto reader = std::unique_ptr<Json::CharReader>(readerBuilder.newCharReader()); | ||
| std::string errors; | ||
| if (reader->parse(json, json + strlen(json), &value, &errors)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // The CharReader API allows us to set failIfExtra, unlike Reader, but does | ||
| // not allow us to get structured errors. We get to try to manually undo | ||
| // the work it did to create a string from the structured errors it had. | ||
| ChipLogError(chipTool, "Error parsing JSON for %s:", label); | ||
|
|
||
| // For each error "errors" has the following: | ||
| // | ||
| // 1) A line starting with "* " that has line/column info | ||
| // 2) A line with the error message. | ||
| // 3) An optional line with some extra info. | ||
| // | ||
| // We keep track of the last error column, in case the error message | ||
| // reporting needs it. | ||
| std::istringstream stream(errors); | ||
| std::string error; | ||
| chip::Optional<unsigned> errorColumn; | ||
| while (getline(stream, error)) | ||
| { | ||
| if (error.rfind("* ", 0) == 0) | ||
| { | ||
| // Flush out any pending error location. | ||
| LogErrorLocation(errorColumn, json); | ||
|
|
||
| // The format of this line is: | ||
| // | ||
| // * Line N, Column M | ||
| // | ||
| // Unfortunately it does not indicate end of error, so we can only | ||
| // show its start. | ||
| unsigned errorLine; // ignored in practice | ||
| if (sscanf(error.c_str(), "* Line %u, Column %u", &errorLine, &errorColumn.Emplace()) != 2) | ||
| { | ||
| ChipLogError(chipTool, "Unexpected location string: %s\n", error.c_str()); | ||
| // We don't know how to make sense of this thing anymore. | ||
| break; | ||
| } | ||
| if (errorColumn.Value() == 0) | ||
| { | ||
| ChipLogError(chipTool, "Expected error column to be at least 1"); | ||
| // We don't know how to make sense of this thing anymore. | ||
| break; | ||
| } | ||
| // We are using our column numbers as offsets, so want them to be | ||
| // 0-based. | ||
| --errorColumn.Value(); | ||
| } | ||
| else | ||
| { | ||
| ChipLogError(chipTool, " %s", error.c_str()); | ||
| if (error == " Missing ',' or '}' in object declaration" && errorColumn.HasValue() && errorColumn.Value() > 0 && | ||
| json[errorColumn.Value() - 1] == '0' && (json[errorColumn.Value()] == 'x' || json[errorColumn.Value()] == 'X')) | ||
| { | ||
| // Log the error location marker before showing the NOTE | ||
| // message. | ||
| LogErrorLocation(errorColumn, json); | ||
| ChipLogError(chipTool, | ||
| "NOTE: JSON does not allow hex syntax beginning with 0x for numbers. Try putting the hex number " | ||
| "in quotes (like {\"name\": \"0x100\"})."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Write out the marker for our last error. | ||
| LogErrorLocation(errorColumn, json); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private: | ||
| static void LogErrorLocation(chip::Optional<unsigned> & errorColumn, const char * json) | ||
| { | ||
| if (!errorColumn.HasValue()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const char * sourceText = json; | ||
| unsigned error_start = errorColumn.Value(); | ||
| // The whole JSON string might be too long to fit in our log | ||
| // messages. Just include 30 chars before the error. | ||
| constexpr ptrdiff_t kMaxContext = 30; | ||
| std::string errorMarker; | ||
| if (error_start > kMaxContext) | ||
| { | ||
| sourceText += (error_start - kMaxContext); | ||
| error_start = kMaxContext; | ||
| ChipLogError(chipTool, "... %s", sourceText); | ||
| // Add markers corresponding to the "... " above. | ||
| errorMarker += "----"; | ||
| } | ||
| else | ||
| { | ||
| ChipLogError(chipTool, "%s", sourceText); | ||
| } | ||
| for (unsigned i = 0; i < error_start; ++i) | ||
| { | ||
| errorMarker += "-"; | ||
| } | ||
| errorMarker += "^"; | ||
| ChipLogError(chipTool, "%s", errorMarker.c_str()); | ||
| errorColumn.ClearValue(); | ||
| } | ||
| }; |