[json_parser] Enable json_parser target and tests for host, tidy up
This library contains json parsing utilities that are useful on host
and/or very similar to other library utilities that are useful on host.
This fixes up the build rules and code to build on all supported host
targets (avoiding strlcpy) and reuses directory enumeration / file
testing logic from src/lib/files/ and also tidies up the build rules
and namespace.
This also removes code in the appmgr integration test that attempted to
read configuration data from the path "/system/data/sysmgr" which no
longer exists. Previously, JSONParser::ParseFromDirectory would return
no error if it was unable to open a directory. This commit changes the
behavior to report an error in this case.
Bug: 36759
Change-Id: Ic7af0ac23181a9bff678204c68ed96beb9784f5b
diff --git a/garnet/packages/tests/BUILD.gn b/garnet/packages/tests/BUILD.gn
index f149c10..5edff76 100644
--- a/garnet/packages/tests/BUILD.gn
+++ b/garnet/packages/tests/BUILD.gn
@@ -912,7 +912,6 @@
testonly = true
public_deps = [
"//build/tools/json_merge:json_merge_test($host_toolchain)",
- "//garnet/public/lib/json:json_unittests",
]
}
diff --git a/garnet/public/lib/json/BUILD.gn b/garnet/public/lib/json/BUILD.gn
index f213a5a..dc19816 100644
--- a/garnet/public/lib/json/BUILD.gn
+++ b/garnet/public/lib/json/BUILD.gn
@@ -2,18 +2,9 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
-# Do not use this target directly, instead depend on //src/lib/json_parser:json_unittests.
-group("json_unittests") {
- public_deps = [
- "//src/lib/json_parser:json_unittests",
- ]
-
- testonly = true
-}
-
-# Do not use this target directly, instead depend on //src/lib/json_parser:json.
+# Do not use this target directly, instead depend on //src/lib/json_parser.
group("json") {
public_deps = [
- "//src/lib/json_parser:json",
+ "//src/lib/json_parser",
]
}
diff --git a/src/lib/BUILD.gn b/src/lib/BUILD.gn
index 22461d55..0d3997e 100644
--- a/src/lib/BUILD.gn
+++ b/src/lib/BUILD.gn
@@ -39,6 +39,7 @@
"fuzzing:tests",
"icu:tests",
"isolated_devmgr:tests",
+ "json_parser:tests",
"line_input:tests",
"mpmc:tests",
"network_wrapper:tests",
diff --git a/src/lib/json_parser/BUILD.gn b/src/lib/json_parser/BUILD.gn
index fba23af..062d998 100644
--- a/src/lib/json_parser/BUILD.gn
+++ b/src/lib/json_parser/BUILD.gn
@@ -2,10 +2,7 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
-import("//build/test/test_package.gni")
-import("//build/testing/environments.gni")
-
-source_set("json") {
+source_set("json_parser") {
sources = [
"json_parser.cc",
"json_parser.h",
@@ -14,24 +11,24 @@
deps = [
"//src/lib/fxl",
"//third_party/rapidjson",
+ "//zircon/public/lib/fbl",
"//zircon/public/lib/fit",
]
public_configs = [ "//garnet/public:config" ]
}
-executable("tests") {
+executable("json_parser_unittests_bin") {
testonly = true
- output_name = "json_unittests"
+ output_name = "json_parser_unittests"
sources = [
"json_parser_unittest.cc",
]
deps = [
- ":json",
- "//garnet/public/lib/gtest",
+ ":json_parser",
"//src/lib/files",
"//src/lib/fxl",
"//src/lib/fxl/test:gtest_main",
@@ -39,14 +36,30 @@
]
}
-test_package("json_unittests") {
+if (is_fuchsia) {
+ import("//build/test/test_package.gni")
+ import("//build/testing/environments.gni")
+
+ test_package("json_parser_unittests") {
+ deps = [
+ ":json_parser_unittests_bin",
+ ]
+ tests = [
+ {
+ name = "json_parser_unittests"
+ environments = basic_envs
+ },
+ ]
+ }
+}
+
+group("tests") {
+ testonly = true
+
deps = [
- ":tests",
+ ":json_parser_unittests_bin($host_toolchain)",
]
- tests = [
- {
- name = "json_unittests"
- environments = basic_envs
- },
- ]
+ if (is_fuchsia) {
+ deps += [ ":json_parser_unittests" ]
+ }
}
diff --git a/src/lib/json_parser/json_parser.cc b/src/lib/json_parser/json_parser.cc
index 3dbb355..ae280bf 100644
--- a/src/lib/json_parser/json_parser.cc
+++ b/src/lib/json_parser/json_parser.cc
@@ -2,25 +2,27 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "lib/json/json_parser.h"
+#include "src/lib/json_parser/json_parser.h"
-#include <dirent.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/types.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
#include <functional>
#include <string>
+#include <fbl/unique_fd.h>
+#include <rapidjson/document.h>
+#include <rapidjson/error/en.h>
+
#include "lib/fit/function.h"
-#include "rapidjson/document.h"
-#include "rapidjson/error/en.h"
+#include "src/lib/files/directory.h"
#include "src/lib/files/file.h"
#include "src/lib/fxl/macros.h"
#include "src/lib/fxl/strings/join_strings.h"
#include "src/lib/fxl/strings/string_printf.h"
-namespace json {
+namespace json_parser {
namespace {
using ErrorCallback = fit::function<void(size_t, const std::string&)>;
@@ -80,39 +82,27 @@
void JSONParser::ParseFromDirectory(const std::string& path,
fit::function<void(rapidjson::Document)> cb) {
- static constexpr char kPathTooLong[] = "Config directory path is too long: %s";
- char buf[PATH_MAX];
- buf[0] = '\0';
- if (strlcpy(buf, path.c_str(), PATH_MAX) >= PATH_MAX) {
- file_ = path;
- ReportError(fxl::StringPrintf(kPathTooLong, buf));
+ fbl::unique_fd dir_fd(open(path.c_str(), O_RDONLY | O_DIRECTORY));
+ if (!dir_fd.is_valid()) {
+ ReportError(
+ fxl::StringPrintf("Could not open directory %s error %s", path.c_str(), strerror(errno)));
return;
}
- if (buf[strlen(buf) - 2] != '/' && strlcat(buf, "/", PATH_MAX) >= PATH_MAX) {
- file_ = path;
- ReportError(fxl::StringPrintf(kPathTooLong, buf));
+
+ std::vector<std::string> dir_entries;
+ if (!files::ReadDirContentsAt(dir_fd.get(), ".", &dir_entries)) {
+ ReportError(fxl::StringPrintf("Could not read directory contents from path %s error %s",
+ path.c_str(), strerror(errno)));
return;
}
- const size_t dir_len = strlen(buf);
- DIR* cfg_dir = opendir(path.c_str());
- if (cfg_dir != nullptr) {
- for (dirent* cfg = readdir(cfg_dir); cfg != nullptr; cfg = readdir(cfg_dir)) {
- if (strcmp(".", cfg->d_name) == 0 || strcmp("..", cfg->d_name) == 0) {
- continue;
- }
- if (strlcat(buf, cfg->d_name, PATH_MAX) >= PATH_MAX) {
- file_ = path;
- ReportError(fxl::StringPrintf(kPathTooLong, buf));
- continue;
- }
- rapidjson::Document document = ParseFromFile(buf);
- if (!document.HasParseError() && !document.IsNull()) {
- cb(std::move(document));
- }
- // Reset buf to directory path.
- buf[dir_len] = '\0';
+ for (const auto& entry : dir_entries) {
+ if (!files::IsFileAt(dir_fd.get(), entry))
+ continue;
+
+ rapidjson::Document document = ParseFromFileAt(dir_fd.get(), entry);
+ if (!document.HasParseError() && !document.IsNull()) {
+ cb(std::move(document));
}
- closedir(cfg_dir);
}
}
@@ -150,4 +140,4 @@
std::string JSONParser::error_str() const { return fxl::JoinStrings(errors_, "\n"); }
-} // namespace json
+} // namespace json_parser
diff --git a/src/lib/json_parser/json_parser.h b/src/lib/json_parser/json_parser.h
index f959c34..527b829 100644
--- a/src/lib/json_parser/json_parser.h
+++ b/src/lib/json_parser/json_parser.h
@@ -10,11 +10,12 @@
#include <string>
#include <vector>
+#include <rapidjson/document.h>
+
#include "lib/fit/function.h"
-#include "rapidjson/document.h"
#include "src/lib/fxl/macros.h"
-namespace json {
+namespace json_parser {
// A JSON parser utility class.
//
@@ -81,6 +82,14 @@
FXL_DISALLOW_COPY_AND_ASSIGN(JSONParser);
};
+} // namespace json_parser
+
+namespace json {
+// The JSONParser should be used from the json_parser namespace to be consistent with the rest of
+// this library but currently many callsites expect this to live in the json namespace. This alias
+// allows both names to work.
+// TODO(36759): Update callers to new name and remove this alias.
+using JSONParser = ::json_parser::JSONParser;
} // namespace json
#endif // SRC_LIB_JSON_PARSER_JSON_PARSER_H_
diff --git a/src/lib/json_parser/json_parser_unittest.cc b/src/lib/json_parser/json_parser_unittest.cc
index 28d3938..e438970 100644
--- a/src/lib/json_parser/json_parser_unittest.cc
+++ b/src/lib/json_parser/json_parser_unittest.cc
@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "lib/json/json_parser.h"
+#include "src/lib/json_parser/json_parser.h"
#include <fcntl.h>
#include <stdio.h>
@@ -10,16 +10,17 @@
#include <functional>
#include <string>
-#include "gtest/gtest.h"
+#include <gtest/gtest.h>
+#include <rapidjson/document.h>
+
#include "lib/fit/function.h"
-#include "rapidjson/document.h"
#include "src/lib/files/file.h"
#include "src/lib/files/path.h"
#include "src/lib/files/scoped_temp_dir.h"
#include "src/lib/fxl/strings/concatenate.h"
#include "src/lib/fxl/strings/substitute.h"
-namespace json {
+namespace json_parser {
namespace {
class JSONParserTest : public ::testing::Test {
@@ -69,9 +70,8 @@
}
std::string NewJSONFileInDir(const std::string& dir, const std::string& json) {
- const std::string json_file =
- fxl::Concatenate({dir, "/json_file", std::to_string(unique_id_++)});
- if (!files::WriteFile(json_file, json.data(), json.size())) {
+ const std::string json_file = "json_file" + std::to_string(unique_id_++);
+ if (!files::WriteFile(dir + "/" + json_file, json.data(), json.size())) {
return "";
}
return json_file;
@@ -240,6 +240,15 @@
EXPECT_EQ(1, props_found_);
}
+TEST_F(JSONParserTest, ParseFromDirectoryDoesNotExist) {
+ std::string dir = "do_not_exist";
+
+ JSONParser parser;
+ std::string error;
+ EXPECT_FALSE(ParseFromDirectory(&parser, dir, &error));
+ EXPECT_NE(std::string::npos, error.find("Could not open directory"));
+}
+
TEST_F(JSONParserTest, CopyArrayToVectorNonArray) {
const std::string json = R"JSON({
"foo": 0
@@ -297,4 +306,4 @@
}
} // namespace
-} // namespace json
+} // namespace json_parser
diff --git a/src/lib/json_parser/meta/json_unittests.cmx b/src/lib/json_parser/meta/json_parser_unittests.cmx
similarity index 70%
rename from src/lib/json_parser/meta/json_unittests.cmx
rename to src/lib/json_parser/meta/json_parser_unittests.cmx
index 0bd49c5..a1637f0 100644
--- a/src/lib/json_parser/meta/json_unittests.cmx
+++ b/src/lib/json_parser/meta/json_parser_unittests.cmx
@@ -1,6 +1,6 @@
{
"program": {
- "binary": "test/json_unittests"
+ "binary": "test/json_parser_unittests"
},
"sandbox": {
"features": [
diff --git a/src/sys/appmgr/integration_tests/hub_integration_test.cc b/src/sys/appmgr/integration_tests/hub_integration_test.cc
index 05f9487..8bed412 100644
--- a/src/sys/appmgr/integration_tests/hub_integration_test.cc
+++ b/src/sys/appmgr/integration_tests/hub_integration_test.cc
@@ -112,7 +112,6 @@
"fuchsia.sys.test.CacheControl",
"fuchsia.virtualconsole.SessionManager"};
sysmgr::Config config;
- ASSERT_TRUE(config.ParseFromDirectory("/system/data/sysmgr"));
// The following path is deprecated, and because config-data is component
// name isolated, it will be impossible to continue to do this in future:
ASSERT_TRUE(config.ParseFromDirectory("/pkgfs/packages/config-data/0/data/sysmgr"));
