Skip to content

Commit

Permalink
[CodeHealth] Simplifying json parsing - pt.4 (#27805)
Browse files Browse the repository at this point in the history
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 <[email protected]>
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
  • Loading branch information
cdesouza-chromium authored Feb 26, 2025
1 parent 9f33c95 commit 49ffee3
Show file tree
Hide file tree
Showing 24 changed files with 181 additions and 198 deletions.
6 changes: 3 additions & 3 deletions browser/brave_wallet/external_wallets_importer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -400,18 +400,18 @@ 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;
}

std::optional<std::string> mnemonic = std::nullopt;
std::optional<int> 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<base::Value> document = base::JSONReader::Read(entities);
if (!document || !document->is_list()) {
std::optional<base::Value::List> 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,6 @@ std::string GetBlindedCredsJSON(
return json;
}

std::optional<base::Value::List> ParseStringToBaseList(
const std::string& string_list) {
std::optional<base::Value> value = base::JSONReader::Read(string_list);
if (!value || !value->is_list()) {
return std::nullopt;
}

return std::move(value).value().TakeList();
}

base::expected<std::vector<std::string>, std::string> UnBlindCreds(
const mojom::CredsBatch& creds_batch) {
std::vector<std::string> unblinded_encoded_creds;
Expand All @@ -94,7 +84,7 @@ base::expected<std::vector<std::string>, 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<Token> creds;
for (auto& item : creds_base64.value()) {
Expand All @@ -105,7 +95,8 @@ base::expected<std::vector<std::string>, 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<BlindedToken> blinded_creds;
for (auto& item : blinded_creds_base64.value()) {
Expand All @@ -117,7 +108,8 @@ base::expected<std::vector<std::string>, 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<SignedToken> signed_creds;
for (auto& item : signed_creds_base64.value()) {
Expand Down Expand Up @@ -156,7 +148,7 @@ base::expected<std::vector<std::string>, std::string> UnBlindCreds(
std::vector<std::string> UnBlindCredsMock(const mojom::CredsBatch& creds) {
std::vector<std::string> 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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,13 @@ mojom::Result PostBalance::ParseBody(const std::string& body,
double* available) {
DCHECK(available);

std::optional<base::Value> value = base::JSONReader::Read(body);
if (!value || !value->is_list()) {
std::optional<base::Value::List> 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, Capability> 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,13 @@ mojom::Result GetCards::ParseBody(const std::string& body,
std::string* id) const {
DCHECK(id);

std::optional<base::Value> value = base::JSONReader::Read(body);
if (!value || !value->is_list()) {
std::optional<base::Value::List> 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,10 @@ mojom::UICardItemPtr ReadItem(const base::Value& value) {
}

std::optional<std::vector<mojom::UICardPtr>> ReadResponseBody(
const base::Value& body) {
if (!body.is_dict()) {
return std::nullopt;
}

const base::Value::Dict& body) {
std::vector<mojom::UICardPtr> cards;

for (auto [key, value] : body.GetDict()) {
for (auto [key, value] : body) {
auto card = mojom::UICard::New();
card->name = key;

Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,16 +211,16 @@ std::vector<FilterListCatalogEntry> FilterListCatalogFromJSON(
std::vector<FilterListCatalogEntry> catalog =
std::vector<FilterListCatalogEntry>();

std::optional<base::Value> parsed_json = base::JSONReader::Read(catalog_json);
if (!parsed_json || !parsed_json->is_list()) {
std::optional<base::Value::List> parsed_json =
base::JSONReader::ReadList(catalog_json);
if (!parsed_json) {
LOG(ERROR) << "Could not load regional adblock catalog";
return catalog;
}

base::JSONValueConverter<FilterListCatalogEntry> 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);
Expand Down
7 changes: 4 additions & 3 deletions components/brave_vpn/browser/brave_vpn_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,10 @@ void BraveVpnService::GetAllRegions(GetAllRegionsCallback callback) {
void BraveVpnService::OnFetchRegionList(GetAllRegionsCallback callback,
const std::string& region_list,
bool success) {
std::optional<base::Value> value = base::JSONReader::Read(region_list);
if (value && value->is_list()) {
auto new_regions = ParseRegionList(value->GetList());
std::optional<base::Value::List> value =
base::JSONReader::ReadList(region_list);
if (value) {
auto new_regions = ParseRegionList(*value);
std::vector<mojom::RegionPtr> regions;
for (const auto& region : new_regions) {
regions.push_back(region.Clone());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ void BraveVPNRegionDataManager::OnFetchRegionList(
CHECK_IS_TEST();
}
api_request_.reset();
std::optional<base::Value> value = base::JSONReader::Read(region_list);
if (value && value->is_list() &&
ParseAndCacheRegionList(value->GetList(), true)) {
std::optional<base::Value::List> 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.
Expand Down Expand Up @@ -270,11 +270,17 @@ void BraveVPNRegionDataManager::OnFetchTimezones(
bool success) {
api_request_.reset();

std::optional<base::Value> 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<base::Value::List> 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";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,10 @@ void ConnectionAPIImpl::OnFetchHostnames(const std::string& region,

ResetAPIRequestInstance();

std::optional<base::Value> value = base::JSONReader::Read(hostnames);
if (value && value->is_list()) {
ParseAndCacheHostnames(region, value->GetList());
std::optional<base::Value::List> value =
base::JSONReader::ReadList(hostnames);
if (value) {
ParseAndCacheHostnames(region, *value);
return;
}

Expand Down
6 changes: 3 additions & 3 deletions components/brave_wallet/browser/ethereum_provider_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
2 changes: 1 addition & 1 deletion components/brave_wallet/browser/fil_requests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ std::string getStateSearchMsgLimited(const std::string& cid, uint64_t period) {
std::optional<std::string> 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()));
Expand Down
Loading

0 comments on commit 49ffee3

Please sign in to comment.