From 49ffee34a1d8bd424d053520a318c4231c1a06ba Mon Sep 17 00:00:00 2001 From: cdesouza-chromium Date: Wed, 26 Feb 2025 13:51:52 +0100 Subject: [PATCH] [CodeHealth] Simplifying json parsing - pt.4 (#27805) This PR simplifies JSON reading in several places by: - Using `base::JSONReader::ReadDict` - Using `base::JSONReader::ReadList` - Using `base::test::ParseJson*` This changes aim to make the checks around the parsing and its expectations simpler to read. This PR also introduces patch for `base::JSONReader::ReadList` which is only available in M135. Chromium change: https://chromium.googlesource.com/chromium/src/+/cb3669146301529c0c364a0478372fc46ddf514a commit cb3669146301529c0c364a0478372fc46ddf514a Author: Claudio DeSouza Date: Wed Feb 19 16:51:29 2025 -0800 [base] Adding `JSONReader::ReadList` This class already provides `ReadDict`, which has proven its usefulness in general. This change adds the corresponding implementation for `base::Value::Dict`. Bug: 40912723 --- .../brave_wallet/external_wallets_importer.cc | 6 +- .../browser/named_third_party_registry.cc | 7 +- .../engine/credentials/credentials_util.cc | 20 ++---- .../post_balance/post_balance_gemini.cc | 7 +- .../get_capabilities/get_capabilities.cc | 6 +- .../endpoint/uphold/get_cards/get_cards.cc | 7 +- .../engine/endpoints/brave/get_ui_cards.cc | 10 +-- .../gemini/get_recipient_id_gemini.cc | 6 +- .../core/browser/filter_list_catalog_entry.cc | 8 +-- .../brave_vpn/browser/brave_vpn_service.cc | 7 +- .../brave_vpn_region_data_manager.cc | 22 ++++--- .../browser/connection/connection_api_impl.cc | 7 +- .../browser/ethereum_provider_impl.cc | 6 +- .../brave_wallet/browser/fil_requests.cc | 2 +- .../browser/fil_requests_unittest.cc | 64 ++++++++----------- .../brave_wallet/browser/fil_transaction.cc | 4 +- .../brave_wallet/browser/fil_transaction.h | 2 +- .../browser/fil_transaction_unittest.cc | 53 ++++++--------- .../browser/fil_tx_manager_unittest.cc | 13 +--- .../brave_wallet/browser/filecoin_keyring.cc | 6 +- .../json_rpc_service_test_utils_unittest.cc | 35 ++++------ .../browser/json_rpc_service_unittest.cc | 41 +++++------- patches/base-json-json_reader.cc.patch | 22 +++++++ patches/base-json-json_reader.h.patch | 18 ++++++ 24 files changed, 181 insertions(+), 198 deletions(-) create mode 100644 patches/base-json-json_reader.cc.patch create mode 100644 patches/base-json-json_reader.h.patch diff --git a/browser/brave_wallet/external_wallets_importer.cc b/browser/brave_wallet/external_wallets_importer.cc index c4bf2163e660..b5499589f880 100644 --- a/browser/brave_wallet/external_wallets_importer.cc +++ b/browser/brave_wallet/external_wallets_importer.cc @@ -400,10 +400,10 @@ void ExternalWalletsImporter::GetMnemonic(bool is_legacy_crypto_wallets, const std::string decrypted_keyrings_str = std::string(decrypted_keyrings->begin(), decrypted_keyrings->end()); - auto keyrings = base::JSONReader::Read( + auto keyrings = base::JSONReader::ReadList( decrypted_keyrings_str, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSON_ALLOW_TRAILING_COMMAS); - if (!keyrings || !keyrings->is_list()) { + if (!keyrings) { VLOG(1) << "not a valid JSON: " << decrypted_keyrings_str; std::move(callback).Run(base::unexpected(ImportError::kJsonError)); return; @@ -411,7 +411,7 @@ void ExternalWalletsImporter::GetMnemonic(bool is_legacy_crypto_wallets, std::optional mnemonic = std::nullopt; std::optional number_of_accounts = std::nullopt; - for (const auto& keyring_listed : keyrings->GetList()) { + for (const auto& keyring_listed : *keyrings) { DCHECK(keyring_listed.is_dict()); const auto& keyring = *keyring_listed.GetIfDict(); const auto* type = keyring.FindString("type"); diff --git a/components/brave_perf_predictor/browser/named_third_party_registry.cc b/components/brave_perf_predictor/browser/named_third_party_registry.cc index 0c2fe3356405..830d51ef774f 100644 --- a/components/brave_perf_predictor/browser/named_third_party_registry.cc +++ b/components/brave_perf_predictor/browser/named_third_party_registry.cc @@ -37,14 +37,15 @@ ParseMappings(std::string_view entities, bool discard_irrelevant) { auto& [entity_by_domain, entity_by_root_domain] = result; // Parse the JSON - std::optional document = base::JSONReader::Read(entities); - if (!document || !document->is_list()) { + std::optional document = + base::JSONReader::ReadList(entities); + if (!document) { LOG(ERROR) << "Cannot parse the third-party entities list"; return {}; } // Collect the mappings - for (auto& item : document->GetList()) { + for (auto& item : *document) { const auto& entity = item.GetDict(); const std::string* entity_name = entity.FindString("name"); diff --git a/components/brave_rewards/core/engine/credentials/credentials_util.cc b/components/brave_rewards/core/engine/credentials/credentials_util.cc index 1a269e2a22eb..b0a39da57b49 100644 --- a/components/brave_rewards/core/engine/credentials/credentials_util.cc +++ b/components/brave_rewards/core/engine/credentials/credentials_util.cc @@ -75,16 +75,6 @@ std::string GetBlindedCredsJSON( return json; } -std::optional ParseStringToBaseList( - const std::string& string_list) { - std::optional value = base::JSONReader::Read(string_list); - if (!value || !value->is_list()) { - return std::nullopt; - } - - return std::move(value).value().TakeList(); -} - base::expected, std::string> UnBlindCreds( const mojom::CredsBatch& creds_batch) { std::vector unblinded_encoded_creds; @@ -94,7 +84,7 @@ base::expected, std::string> UnBlindCreds( return base::unexpected(std::move(batch_proof).error()); } - auto creds_base64 = ParseStringToBaseList(creds_batch.creds); + auto creds_base64 = base::JSONReader::ReadList(creds_batch.creds); DCHECK(creds_base64.has_value()); std::vector creds; for (auto& item : creds_base64.value()) { @@ -105,7 +95,8 @@ base::expected, std::string> UnBlindCreds( } } - auto blinded_creds_base64 = ParseStringToBaseList(creds_batch.blinded_creds); + auto blinded_creds_base64 = + base::JSONReader::ReadList(creds_batch.blinded_creds); DCHECK(blinded_creds_base64.has_value()); std::vector blinded_creds; for (auto& item : blinded_creds_base64.value()) { @@ -117,7 +108,8 @@ base::expected, std::string> UnBlindCreds( } } - auto signed_creds_base64 = ParseStringToBaseList(creds_batch.signed_creds); + auto signed_creds_base64 = + base::JSONReader::ReadList(creds_batch.signed_creds); DCHECK(signed_creds_base64.has_value()); std::vector signed_creds; for (auto& item : signed_creds_base64.value()) { @@ -156,7 +148,7 @@ base::expected, std::string> UnBlindCreds( std::vector UnBlindCredsMock(const mojom::CredsBatch& creds) { std::vector unblinded_encoded_creds; - auto signed_creds_base64 = ParseStringToBaseList(creds.signed_creds); + auto signed_creds_base64 = base::JSONReader::ReadList(creds.signed_creds); DCHECK(signed_creds_base64.has_value()); for (auto& item : signed_creds_base64.value()) { diff --git a/components/brave_rewards/core/engine/endpoint/gemini/post_balance/post_balance_gemini.cc b/components/brave_rewards/core/engine/endpoint/gemini/post_balance/post_balance_gemini.cc index 4ab16fd19cc4..b1c2af6e9e78 100644 --- a/components/brave_rewards/core/engine/endpoint/gemini/post_balance/post_balance_gemini.cc +++ b/components/brave_rewards/core/engine/endpoint/gemini/post_balance/post_balance_gemini.cc @@ -33,14 +33,13 @@ mojom::Result PostBalance::ParseBody(const std::string& body, double* available) { DCHECK(available); - std::optional value = base::JSONReader::Read(body); - if (!value || !value->is_list()) { + std::optional value = base::JSONReader::ReadList(body); + if (!value) { engine_->LogError(FROM_HERE) << "Invalid JSON"; return mojom::Result::FAILED; } - auto& balances = value->GetList(); - for (auto& item : balances) { + for (auto& item : *value) { DCHECK(item.is_dict()); auto& balance = item.GetDict(); const auto* currency_code = balance.FindString("currency"); diff --git a/components/brave_rewards/core/engine/endpoint/uphold/get_capabilities/get_capabilities.cc b/components/brave_rewards/core/engine/endpoint/uphold/get_capabilities/get_capabilities.cc index 70937e751830..2f916d0a9f50 100644 --- a/components/brave_rewards/core/engine/endpoint/uphold/get_capabilities/get_capabilities.cc +++ b/components/brave_rewards/core/engine/endpoint/uphold/get_capabilities/get_capabilities.cc @@ -80,14 +80,14 @@ GetCapabilities::ProcessResponse(const mojom::UrlResponse& response) const { GetCapabilities::CapabilityMap GetCapabilities::ParseBody( const std::string& body) const { - const auto value = base::JSONReader::Read(body); - if (!value || !value->is_list()) { + const auto value = base::JSONReader::ReadList(body); + if (!value) { engine_->LogError(FROM_HERE) << "Invalid body format"; return {}; } std::map capability_map; - for (const auto& item : value->GetList()) { + for (const auto& item : *value) { DCHECK(item.is_dict()); const auto& dict = item.GetDict(); const auto* key = dict.FindString("key"); diff --git a/components/brave_rewards/core/engine/endpoint/uphold/get_cards/get_cards.cc b/components/brave_rewards/core/engine/endpoint/uphold/get_cards/get_cards.cc index f0c7b3126fdf..857b672c24d1 100644 --- a/components/brave_rewards/core/engine/endpoint/uphold/get_cards/get_cards.cc +++ b/components/brave_rewards/core/engine/endpoint/uphold/get_cards/get_cards.cc @@ -46,14 +46,13 @@ mojom::Result GetCards::ParseBody(const std::string& body, std::string* id) const { DCHECK(id); - std::optional value = base::JSONReader::Read(body); - if (!value || !value->is_list()) { + std::optional value = base::JSONReader::ReadList(body); + if (!value) { engine_->LogError(FROM_HERE) << "Invalid JSON"; return mojom::Result::FAILED; } - auto& list = value->GetList(); - for (const auto& it : list) { + for (const auto& it : *value) { DCHECK(it.is_dict()); const auto& dict = it.GetDict(); const auto* label = dict.FindString("label"); diff --git a/components/brave_rewards/core/engine/endpoints/brave/get_ui_cards.cc b/components/brave_rewards/core/engine/endpoints/brave/get_ui_cards.cc index 7fb69ee328e6..32f5297668bc 100644 --- a/components/brave_rewards/core/engine/endpoints/brave/get_ui_cards.cc +++ b/components/brave_rewards/core/engine/endpoints/brave/get_ui_cards.cc @@ -42,14 +42,10 @@ mojom::UICardItemPtr ReadItem(const base::Value& value) { } std::optional> ReadResponseBody( - const base::Value& body) { - if (!body.is_dict()) { - return std::nullopt; - } - + const base::Value::Dict& body) { std::vector cards; - for (auto [key, value] : body.GetDict()) { + for (auto [key, value] : body) { auto card = mojom::UICard::New(); card->name = key; @@ -95,7 +91,7 @@ GetUICards::Result GetUICards::MapResponse(const mojom::UrlResponse& response) { return std::nullopt; } - auto value = base::JSONReader::Read(response.body); + auto value = base::JSONReader::ReadDict(response.body); if (!value) { LogError(FROM_HERE) << "Failed to parse body: invalid JSON"; return std::nullopt; diff --git a/components/brave_rewards/core/engine/endpoints/gemini/get_recipient_id_gemini.cc b/components/brave_rewards/core/engine/endpoints/gemini/get_recipient_id_gemini.cc index aeb714ed14ab..5985df7983b0 100644 --- a/components/brave_rewards/core/engine/endpoints/gemini/get_recipient_id_gemini.cc +++ b/components/brave_rewards/core/engine/endpoints/gemini/get_recipient_id_gemini.cc @@ -22,13 +22,13 @@ using Result = GetRecipientIDGemini::Result; namespace { Result ParseBody(RewardsEngine& engine, const std::string& body) { - auto value = base::JSONReader::Read(body); - if (!value || !value->is_list()) { + auto value = base::JSONReader::ReadList(body); + if (!value) { engine.LogError(FROM_HERE) << "Failed to parse body"; return base::unexpected(Error::kFailedToParseBody); } - for (auto& item : value->GetList()) { + for (auto& item : *value) { auto* pair = item.GetIfDict(); if (!pair) { engine.LogError(FROM_HERE) << "Failed to parse body"; diff --git a/components/brave_shields/core/browser/filter_list_catalog_entry.cc b/components/brave_shields/core/browser/filter_list_catalog_entry.cc index 973def899bb4..83bfd992af1e 100644 --- a/components/brave_shields/core/browser/filter_list_catalog_entry.cc +++ b/components/brave_shields/core/browser/filter_list_catalog_entry.cc @@ -211,16 +211,16 @@ std::vector FilterListCatalogFromJSON( std::vector catalog = std::vector(); - std::optional parsed_json = base::JSONReader::Read(catalog_json); - if (!parsed_json || !parsed_json->is_list()) { + std::optional parsed_json = + base::JSONReader::ReadList(catalog_json); + if (!parsed_json) { LOG(ERROR) << "Could not load regional adblock catalog"; return catalog; } base::JSONValueConverter converter; - base::Value::List& regional_lists = parsed_json->GetList(); - for (const auto& item : regional_lists) { + for (const auto& item : *parsed_json) { DCHECK(item.is_dict()); FilterListCatalogEntry entry; converter.Convert(item, &entry); diff --git a/components/brave_vpn/browser/brave_vpn_service.cc b/components/brave_vpn/browser/brave_vpn_service.cc index 16481a05ba78..6f0afdae0aac 100644 --- a/components/brave_vpn/browser/brave_vpn_service.cc +++ b/components/brave_vpn/browser/brave_vpn_service.cc @@ -456,9 +456,10 @@ void BraveVpnService::GetAllRegions(GetAllRegionsCallback callback) { void BraveVpnService::OnFetchRegionList(GetAllRegionsCallback callback, const std::string& region_list, bool success) { - std::optional value = base::JSONReader::Read(region_list); - if (value && value->is_list()) { - auto new_regions = ParseRegionList(value->GetList()); + std::optional value = + base::JSONReader::ReadList(region_list); + if (value) { + auto new_regions = ParseRegionList(*value); std::vector regions; for (const auto& region : new_regions) { regions.push_back(region.Clone()); diff --git a/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc b/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc index 817f1904b10d..8f4b3d2bf7de 100644 --- a/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc +++ b/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc @@ -227,9 +227,9 @@ void BraveVPNRegionDataManager::OnFetchRegionList( CHECK_IS_TEST(); } api_request_.reset(); - std::optional value = base::JSONReader::Read(region_list); - if (value && value->is_list() && - ParseAndCacheRegionList(value->GetList(), true)) { + std::optional value = + base::JSONReader::ReadList(region_list); + if (value && ParseAndCacheRegionList(*value, true)) { VLOG(2) << "Got valid region list"; // Set default device region and it'll be updated when received valid // timezone info. @@ -270,11 +270,17 @@ void BraveVPNRegionDataManager::OnFetchTimezones( bool success) { api_request_.reset(); - std::optional value = base::JSONReader::Read(timezones_list); - if (success && value && value->is_list()) { - VLOG(2) << "Got valid timezones list"; - SetDeviceRegionWithTimezone(value->GetList()); - } else { + if (success) { + std::optional value = + base::JSONReader::ReadList(timezones_list); + success = value.has_value(); + if (success) { + SetDeviceRegionWithTimezone(*value); + VLOG(2) << "Got valid timezones list"; + } + } + + if (!success) { VLOG(2) << "Failed to get invalid timezones list"; } diff --git a/components/brave_vpn/browser/connection/connection_api_impl.cc b/components/brave_vpn/browser/connection/connection_api_impl.cc index 6bf2d20217ca..a3445abface0 100644 --- a/components/brave_vpn/browser/connection/connection_api_impl.cc +++ b/components/brave_vpn/browser/connection/connection_api_impl.cc @@ -120,9 +120,10 @@ void ConnectionAPIImpl::OnFetchHostnames(const std::string& region, ResetAPIRequestInstance(); - std::optional value = base::JSONReader::Read(hostnames); - if (value && value->is_list()) { - ParseAndCacheHostnames(region, value->GetList()); + std::optional value = + base::JSONReader::ReadList(hostnames); + if (value) { + ParseAndCacheHostnames(region, *value); return; } diff --git a/components/brave_wallet/browser/ethereum_provider_impl.cc b/components/brave_wallet/browser/ethereum_provider_impl.cc index 824e1e7a4bab..2817cc6b6cff 100644 --- a/components/brave_wallet/browser/ethereum_provider_impl.cc +++ b/components/brave_wallet/browser/ethereum_provider_impl.cc @@ -168,13 +168,13 @@ void EthereumProviderImpl::AddEthereumChain(const std::string& json_payload, return RejectInvalidParams(std::move(id), std::move(callback)); } - auto json_value = base::JSONReader::Read( + auto json_value = base::JSONReader::ReadDict( json_payload, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSON_ALLOW_TRAILING_COMMAS); - if (!json_value || !json_value->is_dict()) { + if (!json_value) { return RejectInvalidParams(std::move(id), std::move(callback)); } - const auto& root = json_value->GetDict(); + const auto& root = *json_value; const auto* params = root.FindList(brave_wallet::kParams); if (!params || params->empty()) { diff --git a/components/brave_wallet/browser/fil_requests.cc b/components/brave_wallet/browser/fil_requests.cc index 0778e0fae6e8..41060a60d44f 100644 --- a/components/brave_wallet/browser/fil_requests.cc +++ b/components/brave_wallet/browser/fil_requests.cc @@ -94,7 +94,7 @@ std::string getStateSearchMsgLimited(const std::string& cid, uint64_t period) { std::optional getSendTransaction(const std::string& signed_tx) { base::Value::List params; auto signed_tx_value = FilTransaction::DeserializeSignedTx(signed_tx); - if (!signed_tx_value || !signed_tx_value->is_dict()) { + if (!signed_tx_value) { return std::nullopt; } params.Append(std::move(signed_tx_value.value())); diff --git a/components/brave_wallet/browser/fil_requests_unittest.cc b/components/brave_wallet/browser/fil_requests_unittest.cc index ace3b2ad8142..8202b0732fa2 100644 --- a/components/brave_wallet/browser/fil_requests_unittest.cc +++ b/components/brave_wallet/browser/fil_requests_unittest.cc @@ -3,57 +3,48 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ +#include "brave/components/brave_wallet/browser/fil_requests.h" + #include #include #include -#include "base/json/json_reader.h" -#include "brave/components/brave_wallet/browser/fil_requests.h" +#include "base/test/values_test_util.h" #include "testing/gtest/include/gtest/gtest.h" namespace brave_wallet { -namespace { -void CompareJSONs(const std::string& request_string, - const std::string& expected_request) { - auto request_json = base::JSONReader::Read(request_string); - ASSERT_TRUE(request_json); - auto expected_request_json = base::JSONReader::Read(expected_request); - ASSERT_TRUE(expected_request_json); - EXPECT_EQ(*request_json, *expected_request_json); -} -} // namespace - TEST(FilRequestUnitTest, getBalance) { - CompareJSONs(fil::getBalance("t1jdlfl73voaiblrvn2yfivvn5ifucwwv5f26nfza"), - R"({ + EXPECT_EQ(base::test::ParseJson( + fil::getBalance("t1jdlfl73voaiblrvn2yfivvn5ifucwwv5f26nfza")), + base::test::ParseJson(R"({ "id": 1, "jsonrpc": "2.0", "method": "Filecoin.WalletBalance", "params": [ "t1jdlfl73voaiblrvn2yfivvn5ifucwwv5f26nfza" ] - })"); + })")); } TEST(FilRequestUnitTest, getTransactionCount) { - CompareJSONs( - fil::getTransactionCount("t1jdlfl73voaiblrvn2yfivvn5ifucwwv5f26nfza"), - R"({ + EXPECT_EQ(base::test::ParseJson(fil::getTransactionCount( + "t1jdlfl73voaiblrvn2yfivvn5ifucwwv5f26nfza")), + base::test::ParseJson(R"({ "id": 1, "jsonrpc": "2.0", "method": "Filecoin.MpoolGetNonce", "params":[ "t1jdlfl73voaiblrvn2yfivvn5ifucwwv5f26nfza" ] - })"); + })")); } TEST(FilRequestUnitTest, estimateGas) { - CompareJSONs(fil::getEstimateGas("from_address", "to_address", "gas_premium", - "gas_fee_cap", INT64_MAX, UINT64_MAX, - "max_fee", "value"), - R"({ + EXPECT_EQ(base::test::ParseJson(fil::getEstimateGas( + "from_address", "to_address", "gas_premium", "gas_fee_cap", + INT64_MAX, UINT64_MAX, "max_fee", "value")), + base::test::ParseJson(R"({ "id": 1, "jsonrpc": "2.0", "method": "Filecoin.GasEstimateMessageGas", @@ -75,16 +66,15 @@ TEST(FilRequestUnitTest, estimateGas) { }, [] ] - })"); + })")); } TEST(FilRequestUnitTest, estimateGas_WhenSendToFEVM) { - CompareJSONs( - fil::getEstimateGas("from_address", - "t410frrqkhkktbxosf5cmboocdhsv42jtgw2rddjac2y", - "gas_premium", "gas_fee_cap", INT64_MAX, UINT64_MAX, - "max_fee", "value"), - R"({ + EXPECT_EQ(base::test::ParseJson(fil::getEstimateGas( + "from_address", "t410frrqkhkktbxosf5cmboocdhsv42jtgw2rddjac2y", + "gas_premium", "gas_fee_cap", INT64_MAX, UINT64_MAX, "max_fee", + "value")), + base::test::ParseJson(R"({ "id": 1, "jsonrpc": "2.0", "method": "Filecoin.GasEstimateMessageGas", @@ -106,7 +96,7 @@ TEST(FilRequestUnitTest, estimateGas_WhenSendToFEVM) { }, [] ] - })"); + })")); } TEST(FilRequestUnitTest, getChainHead) { @@ -141,8 +131,7 @@ TEST(FilRequestUnitTest, getSendTransaction_WhenSendToFEVM) { } })"); ASSERT_TRUE(send); - CompareJSONs(*send, - R"({ + EXPECT_EQ(base::test::ParseJson(*send), base::test::ParseJson(R"({ "id": 1, "jsonrpc": "2.0", "method": "Filecoin.MpoolPush", @@ -165,7 +154,7 @@ TEST(FilRequestUnitTest, getSendTransaction_WhenSendToFEVM) { } } ] - })"); + })")); } TEST(FilRequestUnitTest, getSendTransaction) { @@ -188,8 +177,7 @@ TEST(FilRequestUnitTest, getSendTransaction) { } })"); ASSERT_TRUE(send); - CompareJSONs(*send, - R"({ + EXPECT_EQ(base::test::ParseJson(*send), base::test::ParseJson(R"({ "id": 1, "jsonrpc": "2.0", "method": "Filecoin.MpoolPush", @@ -212,7 +200,7 @@ TEST(FilRequestUnitTest, getSendTransaction) { } } ] - })"); + })")); // broken json EXPECT_FALSE(fil::getSendTransaction("broken")); // empty json diff --git a/components/brave_wallet/browser/fil_transaction.cc b/components/brave_wallet/browser/fil_transaction.cc index dba6b3b312e8..e08f5e4df8ec 100644 --- a/components/brave_wallet/browser/fil_transaction.cc +++ b/components/brave_wallet/browser/fil_transaction.cc @@ -245,13 +245,13 @@ std::optional FilTransaction::ConvertSignedTxStringFieldsToInt64( } // static -std::optional FilTransaction::DeserializeSignedTx( +std::optional FilTransaction::DeserializeSignedTx( const std::string& signed_tx) { std::string json = json::convert_int64_value_to_string("/Message/GasLimit", signed_tx, true); json = json::convert_int64_value_to_string("/Message/Nonce", json, true); json = json::convert_int64_value_to_string("/Message/Method", json, true); - return base::JSONReader::Read(json); + return base::JSONReader::ReadDict(json); } // https://spec.filecoin.io/algorithms/crypto/signatures/#section-algorithms.crypto.signatures diff --git a/components/brave_wallet/browser/fil_transaction.h b/components/brave_wallet/browser/fil_transaction.h index c176d8421806..e953f0e70518 100644 --- a/components/brave_wallet/browser/fil_transaction.h +++ b/components/brave_wallet/browser/fil_transaction.h @@ -62,7 +62,7 @@ class FilTransaction { // Deserializes JSON which contains value, provided by SignTransaction // Wraps uint64_t fields to string - static std::optional DeserializeSignedTx( + static std::optional DeserializeSignedTx( const std::string& signed_tx); // Finds signed tx JSON by path and converts some string fields to uint64 form static std::optional ConvertMesssageStringFieldsToInt64( diff --git a/components/brave_wallet/browser/fil_transaction_unittest.cc b/components/brave_wallet/browser/fil_transaction_unittest.cc index 2e0bbf73b378..66a6912d43d6 100644 --- a/components/brave_wallet/browser/fil_transaction_unittest.cc +++ b/components/brave_wallet/browser/fil_transaction_unittest.cc @@ -6,8 +6,8 @@ #include "brave/components/brave_wallet/browser/fil_transaction.h" #include "base/base64.h" -#include "base/json/json_reader.h" #include "base/json/json_writer.h" +#include "base/test/values_test_util.h" #include "brave/components/brave_wallet/common/fil_address.h" #include "brave/components/filecoin/rs/src/lib.rs.h" #include "testing/gtest/include/gtest/gtest.h" @@ -15,14 +15,6 @@ namespace brave_wallet { namespace { -void CompareJSONs(const std::string& current_string, - const std::string& expected_string) { - auto current_json = base::JSONReader::Read(current_string); - ASSERT_TRUE(current_json); - auto expected_string_json = base::JSONReader::Read(expected_string); - ASSERT_TRUE(expected_string_json); - EXPECT_EQ(*current_json, *expected_string_json); -} std::string DecodePrivateKey(const std::string& private_key_base64) { std::string private_key_decoded; EXPECT_TRUE(base::Base64Decode(private_key_base64, &private_key_decoded)); @@ -193,8 +185,7 @@ TEST(FilTransactionUnitTest, GetMessageToSignSecp) { "t1h4n7rphclbmwyjcp6jrdiwlfcuwbroxy3jvg33q", "6")); auto message_to_sign = transaction->GetMessageToSignJson(from); ASSERT_TRUE(message_to_sign); - CompareJSONs(*message_to_sign, - R"({ + EXPECT_EQ(base::test::ParseJson(*message_to_sign), base::test::ParseJson(R"({ "From": "t1h5tg3bhp5r56uzgjae2373znti6ygq4agkx4hzq", "GasFeeCap": "3", "GasLimit": 1, @@ -205,12 +196,11 @@ TEST(FilTransactionUnitTest, GetMessageToSignSecp) { "To": "t1h4n7rphclbmwyjcp6jrdiwlfcuwbroxy3jvg33q", "Value": "6", "Version": 0 - })"); + })")); transaction->set_nonce(1); message_to_sign = transaction->GetMessageToSignJson(from); ASSERT_TRUE(message_to_sign); - CompareJSONs(*message_to_sign, - R"({ + EXPECT_EQ(base::test::ParseJson(*message_to_sign), base::test::ParseJson(R"({ "From": "t1h5tg3bhp5r56uzgjae2373znti6ygq4agkx4hzq", "GasFeeCap": "3", "GasLimit": 1, @@ -221,7 +211,7 @@ TEST(FilTransactionUnitTest, GetMessageToSignSecp) { "To": "t1h4n7rphclbmwyjcp6jrdiwlfcuwbroxy3jvg33q", "Value": "6", "Version": 0 - })"); + })")); std::string private_key_decoded = DecodePrivateKey("8VcW07ADswS4BV2cxi5rnIadVsyTDDhY1NfDH19T8Uo="); @@ -229,21 +219,18 @@ TEST(FilTransactionUnitTest, GetMessageToSignSecp) { private_key_decoded.end()); auto signature = transaction->GetSignedTransaction(from, private_key); ASSERT_TRUE(signature.has_value()); - auto signature_value = base::JSONReader::Read(*signature); - EXPECT_TRUE(signature_value); - auto* message = signature_value->GetDict().Find("Message"); + auto signature_value = base::test::ParseJsonDict(*signature); + auto* message = signature_value.Find("Message"); auto* signature_data = - signature_value->GetDict().FindStringByDottedPath("Signature.Data"); + signature_value.FindStringByDottedPath("Signature.Data"); EXPECT_TRUE(message); EXPECT_TRUE(signature_data); - auto message_as_value = base::JSONReader::Read(*message_to_sign); - EXPECT_TRUE(message_as_value); + auto message_as_value = base::test::ParseJson(*message_to_sign); EXPECT_EQ(*signature_data, "SozNIZGNAvALCWtc38OUhO9wdFl82qESGhjnVVhI6CYNN0gP5qa+hZtyFh+" "j9K0wIVVU10ZJPgaV0yM6a+xwKgA="); - EXPECT_EQ(*message, *message_as_value); - auto signature_type = - signature_value->GetDict().FindIntByDottedPath("Signature.Type"); + EXPECT_EQ(*message, message_as_value); + auto signature_type = signature_value.FindIntByDottedPath("Signature.Type"); ASSERT_TRUE(signature_type); EXPECT_EQ(signature_type, 1); } @@ -277,7 +264,8 @@ TEST(FilTransactionUnitTest, GetMessageToSignBLS) { from_account); base::ReplaceFirstSubstringAfterOffset(&expected_message, 0, "{to_account}", to_account); - CompareJSONs(*message_to_sign, expected_message); + EXPECT_EQ(base::test::ParseJson(*message_to_sign), + base::test::ParseJson(expected_message)); std::string private_key_decoded = DecodePrivateKey("7ug8i7Q6xddnBpvjbHe8zm+UekV+EVtOUxpNXr+PpCc="); @@ -286,23 +274,20 @@ TEST(FilTransactionUnitTest, GetMessageToSignBLS) { auto signature = transaction->GetSignedTransaction( FilAddress::FromAddress(from_account), private_key); ASSERT_TRUE(signature.has_value()); - auto signature_value = base::JSONReader::Read(*signature); - EXPECT_TRUE(signature_value); - auto* message = signature_value->GetDict().Find("Message"); + auto signature_value = base::test::ParseJsonDict(*signature); + auto* message = signature_value.Find("Message"); auto* signature_data = - signature_value->GetDict().FindStringByDottedPath("Signature.Data"); + signature_value.FindStringByDottedPath("Signature.Data"); EXPECT_TRUE(message); EXPECT_TRUE(signature_data); - auto message_as_value = base::JSONReader::Read(*message_to_sign); - EXPECT_TRUE(message_as_value); + auto message_as_value = base::test::ParseJson(*message_to_sign); EXPECT_EQ( *signature_data, "lsMyTOOAaW9/FxIKupqypmUl1hXLOKrbcJdQs+bHMPNF6aaCu2MaIRQKjS/" "Hi6pMB84syUMuxRPC5JdpFvMl7gy5J2kvOEuDclSvc1ALQf2wOalPUOH022DNgLVATD36"); - EXPECT_EQ(*message, *message_as_value); + EXPECT_EQ(*message, message_as_value); - auto signature_type = - signature_value->GetDict().FindIntByDottedPath("Signature.Type"); + auto signature_type = signature_value.FindIntByDottedPath("Signature.Type"); ASSERT_TRUE(signature_type); EXPECT_EQ(signature_type, 2); } diff --git a/components/brave_wallet/browser/fil_tx_manager_unittest.cc b/components/brave_wallet/browser/fil_tx_manager_unittest.cc index f6693130fcc6..992e75ed7482 100644 --- a/components/brave_wallet/browser/fil_tx_manager_unittest.cc +++ b/components/brave_wallet/browser/fil_tx_manager_unittest.cc @@ -11,11 +11,11 @@ #include #include "base/files/scoped_temp_dir.h" -#include "base/json/json_reader.h" #include "base/json/json_writer.h" #include "base/task/sequenced_task_runner.h" #include "base/test/bind.h" #include "base/test/task_environment.h" +#include "base/test/values_test_util.h" #include "brave/components/brave_wallet/browser/brave_wallet_prefs.h" #include "brave/components/brave_wallet/browser/brave_wallet_utils.h" #include "brave/components/brave_wallet/browser/fil_transaction.h" @@ -39,14 +39,6 @@ namespace brave_wallet { namespace { -void EqualJSONs(const std::string& current_string, - const std::string& expected_string) { - auto current_json = base::JSONReader::Read(current_string); - ASSERT_TRUE(current_json); - auto expected_string_json = base::JSONReader::Read(expected_string); - ASSERT_TRUE(expected_string_json); - EXPECT_EQ(*current_json, *expected_string_json); -} std::string GetSignedMessage(const std::string& message, const std::string& data) { base::Value::Dict dict; @@ -136,7 +128,8 @@ class FilTxManagerUnitTest : public testing::Test { EXPECT_EQ(!!json_message, expected_message.has_value()); if (expected_message.has_value()) { - EqualJSONs(*json_message, *expected_message); + EXPECT_EQ(base::test::ParseJson(*json_message), + base::test::ParseJson(*expected_message)); } run_loop.Quit(); })); diff --git a/components/brave_wallet/browser/filecoin_keyring.cc b/components/brave_wallet/browser/filecoin_keyring.cc index 5d6b42d45923..90216814090c 100644 --- a/components/brave_wallet/browser/filecoin_keyring.cc +++ b/components/brave_wallet/browser/filecoin_keyring.cc @@ -107,16 +107,16 @@ bool FilecoinKeyring::DecodeImportPayload( if (!base::HexStringToString(payload_hex, &key_payload)) { return false; } - std::optional records_v = base::JSONReader::Read( + std::optional records_v = base::JSONReader::ReadDict( key_payload, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); - if (!records_v || !records_v->is_dict()) { + if (!records_v) { VLOG(1) << "Invalid payload, could not parse JSON, JSON is: " << key_payload; return false; } - const auto& dict = records_v->GetDict(); + const auto& dict = *records_v; const std::string* type = dict.FindString("Type"); if (!type || (*type != "secp256k1" && *type != "bls")) { return false; diff --git a/components/brave_wallet/browser/json_rpc_service_test_utils_unittest.cc b/components/brave_wallet/browser/json_rpc_service_test_utils_unittest.cc index b0d8e6f52f5a..db0e111c05bf 100644 --- a/components/brave_wallet/browser/json_rpc_service_test_utils_unittest.cc +++ b/components/brave_wallet/browser/json_rpc_service_test_utils_unittest.cc @@ -8,7 +8,6 @@ #include #include -#include "base/json/json_reader.h" #include "base/test/values_test_util.h" #include "brave/components/brave_wallet/browser/brave_wallet_utils.h" #include "brave/components/brave_wallet/browser/eth_response_parser.h" @@ -21,14 +20,11 @@ namespace brave_wallet { namespace { std::string ParseRpcJsonResult(const std::string& json) { - std::optional value = base::JSONReader::Read(json); - EXPECT_TRUE(value); - EXPECT_TRUE(value->is_dict()); - EXPECT_THAT(value->GetDict(), + base::Value::Dict value = base::test::ParseJsonDict(json); + EXPECT_THAT(value, base::test::DictionaryHasValue("jsonrpc", base::Value("2.0"))); - EXPECT_THAT(value->GetDict(), - base::test::DictionaryHasValue("id", base::Value(1))); - auto* result = value->GetDict().FindString("result"); + EXPECT_THAT(value, base::test::DictionaryHasValue("id", base::Value(1))); + auto* result = value.FindString("result"); EXPECT_TRUE(result); EXPECT_THAT(*result, testing::StartsWith("0x")); return result->substr(2); @@ -193,26 +189,21 @@ TEST(JsonRpcServiceTestUtils, MakeJsonRpcStringResponse_Empty) { TEST(JsonRpcServiceTestUtils, MakeJsonRpcErrorResponse) { auto json = MakeJsonRpcErrorResponse(123, "Error!"); - std::optional value = base::JSONReader::Read(json); - ASSERT_TRUE(value); - ASSERT_TRUE(value->is_dict()); - EXPECT_EQ("2.0", *value->GetDict().FindString("jsonrpc")); - EXPECT_EQ(1, *value->GetDict().FindInt("id")); - EXPECT_EQ(123, *value->GetDict().FindIntByDottedPath("error.code")); - EXPECT_EQ("Error!", - *value->GetDict().FindStringByDottedPath("error.message")); + base::Value::Dict value = base::test::ParseJsonDict(json); + EXPECT_EQ("2.0", *value.FindString("jsonrpc")); + EXPECT_EQ(1, *value.FindInt("id")); + EXPECT_EQ(123, *value.FindIntByDottedPath("error.code")); + EXPECT_EQ("Error!", *value.FindStringByDottedPath("error.message")); } TEST(JsonRpcServiceTestUtils, MakeJsonRpcValueResponse) { base::Value::Dict payload; payload.Set("test", 555); auto json = MakeJsonRpcValueResponse(base::Value(std::move(payload))); - std::optional value = base::JSONReader::Read(json); - ASSERT_TRUE(value); - ASSERT_TRUE(value->is_dict()); - EXPECT_EQ("2.0", *value->GetDict().FindString("jsonrpc")); - EXPECT_EQ(1, *value->GetDict().FindInt("id")); - EXPECT_EQ(555, *value->GetDict().FindIntByDottedPath("result.value.test")); + base::Value::Dict value = base::test::ParseJsonDict(json); + EXPECT_EQ("2.0", *value.FindString("jsonrpc")); + EXPECT_EQ(1, *value.FindInt("id")); + EXPECT_EQ(555, *value.FindIntByDottedPath("result.value.test")); } } // namespace brave_wallet diff --git a/components/brave_wallet/browser/json_rpc_service_unittest.cc b/components/brave_wallet/browser/json_rpc_service_unittest.cc index 11c595547231..25dcdafff426 100644 --- a/components/brave_wallet/browser/json_rpc_service_unittest.cc +++ b/components/brave_wallet/browser/json_rpc_service_unittest.cc @@ -30,6 +30,7 @@ #include "base/test/mock_callback.h" #include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" +#include "base/test/values_test_util.h" #include "base/values.h" #include "brave/components/brave_wallet/browser/blockchain_list_parser.h" #include "brave/components/brave_wallet/browser/blockchain_registry.h" @@ -88,22 +89,6 @@ namespace brave_wallet { namespace { -// Compare two JSON strings, ignoring the order of the keys and other -// insignificant whitespace differences. -void CompareJSON(const std::string& response, - const std::string& expected_response) { - auto response_val = base::JSONReader::Read(response); - auto expected_response_val = base::JSONReader::Read(expected_response); - EXPECT_EQ(response_val, expected_response_val); - if (response_val) { - // If the JSON is valid, compare the parsed values. - EXPECT_EQ(*response_val, *expected_response_val); - } else { - // If the JSON is invalid, compare the raw strings. - EXPECT_EQ(response, expected_response); - } -} - void GetErrorCodeMessage(base::Value formed_response, mojom::ProviderError* error, std::string* error_message) { @@ -304,13 +289,14 @@ class TestJsonRpcServiceObserver constexpr char https_metadata_response[] = R"({"attributes":[{"trait_type":"Feet","value":"Green Shoes"},{"trait_type":"Legs","value":"Tan Pants"},{"trait_type":"Suspenders","value":"White Suspenders"},{"trait_type":"Upper Body","value":"Indigo Turtleneck"},{"trait_type":"Sleeves","value":"Long Sleeves"},{"trait_type":"Hat","value":"Yellow / Blue Pointy Beanie"},{"trait_type":"Eyes","value":"White Nerd Glasses"},{"trait_type":"Mouth","value":"Toothpick"},{"trait_type":"Ears","value":"Bing Bong Stick"},{"trait_type":"Right Arm","value":"Swinging"},{"trait_type":"Left Arm","value":"Diamond Hand"},{"trait_type":"Background","value":"Blue"}],"description":"5,000 animated Invisible Friends hiding in the metaverse. A collection by Markus Magnusson & Random Character Collective.","image":"https://rcc.mypinata.cloud/ipfs/QmXmuSenZRnofhGMz2NyT3Yc4Zrty1TypuiBKDcaBsNw9V/1817.gif","name":"Invisible Friends #1817"})"; -std::optional ToValue(const network::ResourceRequest& request) { +std::optional ToValue( + const network::ResourceRequest& request) { std::string_view request_string(request.request_body->elements() ->at(0) .As() .AsStringPiece()); - return base::JSONReader::Read(request_string, - base::JSONParserOptions::JSON_PARSE_RFC); + return base::JSONReader::ReadDict(request_string, + base::JSONParserOptions::JSON_PARSE_RFC); } class EthCallHandler { @@ -680,8 +666,8 @@ class JsonRpcEndpointHandler { } auto value = ToValue(request); - if (value && value->is_dict()) { - auto response = HandleCall(value->GetDict()); + if (value) { + auto response = HandleCall(*value); if (response) { return response; } @@ -1897,7 +1883,12 @@ class JsonRpcServiceUnitTest : public testing::Test { const std::string& response, mojom::SolanaProviderError error, const std::string& error_message) { - CompareJSON(response, expected_response); + if (response.empty()) { + EXPECT_EQ(response, expected_response); + } else { + EXPECT_EQ(base::test::ParseJson(response), + base::test::ParseJson(expected_response)); + } EXPECT_EQ(error, expected_error); EXPECT_EQ(error_message, expected_error_message); loop.Quit(); @@ -5801,13 +5792,13 @@ class OffchainGatewayHandler { } auto payload = ToValue(request); - if (!payload || !payload->is_dict()) { + if (!payload) { return std::nullopt; } - auto* sender = payload->GetDict().FindString("sender"); + auto* sender = payload->FindString("sender"); EXPECT_EQ(EthAddress::FromHex(*sender), resolver_address_); - auto* data = payload->GetDict().FindString("data"); + auto* data = payload->FindString("data"); auto bytes = PrefixedHexStringToBytes(*data); if (!bytes) { NOTREACHED(); diff --git a/patches/base-json-json_reader.cc.patch b/patches/base-json-json_reader.cc.patch new file mode 100644 index 000000000000..3302fcff149d --- /dev/null +++ b/patches/base-json-json_reader.cc.patch @@ -0,0 +1,22 @@ +diff --git a/base/json/json_reader.cc b/base/json/json_reader.cc +index af1d4f46d02c97b46352cdaa4834732ce645e145..32d8707d3ad5d55e5479ab87352edcd911770e21 100644 +--- a/base/json/json_reader.cc ++++ b/base/json/json_reader.cc +@@ -164,6 +164,17 @@ std::optional JSONReader::ReadDict(std::string_view json, + return std::move(*value).TakeDict(); + } + ++// static ++std::optional JSONReader::ReadList(std::string_view json, ++ int options, ++ size_t max_depth) { ++ std::optional value = Read(json, options, max_depth); ++ if (!value || !value->is_list()) { ++ return std::nullopt; ++ } ++ return std::move(*value).TakeList(); ++} ++ + // static + JSONReader::Result JSONReader::ReadAndReturnValueWithError( + std::string_view json, diff --git a/patches/base-json-json_reader.h.patch b/patches/base-json-json_reader.h.patch new file mode 100644 index 000000000000..5e134f21edd3 --- /dev/null +++ b/patches/base-json-json_reader.h.patch @@ -0,0 +1,18 @@ +diff --git a/base/json/json_reader.h b/base/json/json_reader.h +index b0b6f1949471a8ec7f86777d5b3fbdcb8e323de1..469546e8fcb2f0b60e7a3a36b09aab4cfa02b062 100644 +--- a/base/json/json_reader.h ++++ b/base/json/json_reader.h +@@ -123,6 +123,13 @@ class BASE_EXPORT JSONReader { + int options = JSON_PARSE_CHROMIUM_EXTENSIONS, + size_t max_depth = internal::kAbsoluteMaxDepth); + ++ // Reads and parses |json|, returning a Value::List. ++ // If |json| is not a properly formed JSON list string, returns std::nullopt. ++ static std::optional ReadList( ++ std::string_view json, ++ int options = JSON_PARSE_CHROMIUM_EXTENSIONS, ++ size_t max_depth = internal::kAbsoluteMaxDepth); ++ + // Reads and parses |json| like Read(). On success returns a Value as the + // expected value. Otherwise, it returns an Error instance, populated with a + // formatted error message, an error code, and the error location if