From 43e0b344d55af50a3bad6c1d63a4d2b42c5deefb Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Mon, 13 Nov 2017 12:13:26 -0500 Subject: [PATCH 1/6] Throw an exception when unable to parse JSON. --- src/json.cc | 12 ++++++++---- src/json.h | 15 ++++++++++----- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/json.cc b/src/json.cc index be411dd0..b0cc03f2 100644 --- a/src/json.cc +++ b/src/json.cc @@ -424,7 +424,9 @@ yajl_callbacks callbacks = { } -std::unique_ptr Parser::FromStream(std::istream& stream) { +std::unique_ptr Parser::FromStream(std::istream& stream) + throw(Exception) +{ JSONBuilder builder; const int kMax = 65536; @@ -446,16 +448,18 @@ std::unique_ptr Parser::FromStream(std::istream& stream) { if (stat != yajl_status_ok) { unsigned char* str = yajl_get_error(handle, 1, data, kMax); - // TODO cerr << str << endl; + std::string error_str((const char*)str); yajl_free_error(handle, str); - return nullptr; + throw json::Exception(error_str); } yajl_free(handle); return builder.value(); } -std::unique_ptr Parser::FromString(const std::string& input) { +std::unique_ptr Parser::FromString(const std::string& input) + throw(Exception) +{ std::stringstream stream(input); return FromStream(stream); } diff --git a/src/json.h b/src/json.h index 8131b0b6..de654b91 100644 --- a/src/json.h +++ b/src/json.h @@ -267,7 +267,8 @@ class Object : public Value, public std::map struct FieldGetter { using return_type = const T*; static const T* GetField(const Object* obj, const std::string& field) - throw(Exception) { + throw(Exception) + { return obj->GetField(field); } }; @@ -277,7 +278,8 @@ class Object : public Value, public std::map using value_type = typename std::result_of::type; using return_type = typename std::remove_reference::type; static return_type GetField(const Object* obj, const std::string& field) - throw(Exception) { + throw(Exception) + { return obj->GetField(field)->value(); } }; @@ -288,7 +290,8 @@ class Object : public Value, public std::map // Field accessors. template typename FieldGetter::return_type Get(const std::string& field) const - throw(Exception) { + throw(Exception) + { return FieldGetter::GetField(this, field); } @@ -328,8 +331,10 @@ inline std::unique_ptr object( class Parser { public: - static std::unique_ptr FromStream(std::istream& stream); - static std::unique_ptr FromString(const std::string& input); + static std::unique_ptr FromStream(std::istream& stream) + throw(Exception); + static std::unique_ptr FromString(const std::string& input) + throw(Exception); }; } From bcd4796f9c38608f9a43c62053859e61115820bb Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Wed, 15 Nov 2017 19:27:52 -0500 Subject: [PATCH 2/6] Really support parsing JSON streams with multiple objects. --- src/json.cc | 49 ++++++++++++++++++++++++++++++++++--------------- src/json.h | 4 ++++ 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/json.cc b/src/json.cc index b0cc03f2..b0d8bb20 100644 --- a/src/json.cc +++ b/src/json.cc @@ -223,17 +223,13 @@ class TopLevelContext : public Context { public: TopLevelContext() : Context(nullptr) {} void AddValue(std::unique_ptr value) override { - if (value_ != nullptr) { - std::cerr << "Replacing " << *value_ - << " with " << *value << std::endl; - } - value_ = std::move(value); + values_.emplace_back(std::move(value)); } - std::unique_ptr value() { - return std::move(value_); + std::vector> values() { + return std::move(values_); } private: - std::unique_ptr value_; + std::vector> values_; }; class ArrayContext : public Context { @@ -320,13 +316,12 @@ class JSONBuilder { } // Top-level context only. - std::unique_ptr value() { + std::vector> values() throw(Exception) { TopLevelContext* top_level = dynamic_cast(context_); if (top_level == nullptr) { - std::cerr << "value() called for an inner context" << std::endl; - return nullptr; + throw json::Exception("values() called for an inner context"); } - return top_level->value(); + return top_level->values(); } private: @@ -424,7 +419,7 @@ yajl_callbacks callbacks = { } -std::unique_ptr Parser::FromStream(std::istream& stream) +std::vector> Parser::AllFromStream(std::istream& stream) throw(Exception) { JSONBuilder builder; @@ -433,6 +428,8 @@ std::unique_ptr Parser::FromStream(std::istream& stream) unsigned char data[kMax]; yajl_handle handle = yajl_alloc(&callbacks, NULL, (void*) &builder); yajl_config(handle, yajl_allow_comments, 1); + yajl_config(handle, yajl_allow_multiple_values, 1); + //yajl_config(handle, yajl_allow_trailing_garbage, 1); //yajl_config(handle, yajl_dont_validate_strings, 1); for (;;) { @@ -454,13 +451,35 @@ std::unique_ptr Parser::FromStream(std::istream& stream) } yajl_free(handle); - return builder.value(); + return builder.values(); +} + +std::unique_ptr Parser::FromStream(std::istream& stream) + throw(Exception) +{ + std::vector> all_values = AllFromStream(stream); + if (all_values.empty()) { + return nullptr; + } + if (all_values.size() > 1) { + std::ostringstream out; + out << "Getting a single value out of " << all_values.size(); + throw json::Exception(out.str()); + } + return std::move(all_values[0]); +} + +std::vector> Parser::AllFromString( + const std::string& input) throw(Exception) +{ + std::istringstream stream(input); + return AllFromStream(stream); } std::unique_ptr Parser::FromString(const std::string& input) throw(Exception) { - std::stringstream stream(input); + std::istringstream stream(input); return FromStream(stream); } diff --git a/src/json.h b/src/json.h index de654b91..bcb494cd 100644 --- a/src/json.h +++ b/src/json.h @@ -331,6 +331,10 @@ inline std::unique_ptr object( class Parser { public: + static std::vector> AllFromStream( + std::istream& stream) throw(Exception); + static std::vector> AllFromString( + const std::string& input) throw(Exception); static std::unique_ptr FromStream(std::istream& stream) throw(Exception); static std::unique_ptr FromString(const std::string& input) From 76811056f951a448ce02bb264937031d338328cd Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Fri, 17 Nov 2017 17:28:24 -0500 Subject: [PATCH 3/6] std::thread does an implicit bind. --- src/api_server.cc | 4 ++-- src/updater.cc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api_server.cc b/src/api_server.cc index 0de020f8..f8cf1502 100644 --- a/src/api_server.cc +++ b/src/api_server.cc @@ -155,7 +155,7 @@ MetadataApiServer::MetadataApiServer(const MetadataAgent& agent, server_pool_() { for (int i : boost::irange(0, server_threads)) { - server_pool_.emplace_back(std::bind(&HttpServer::run, &server_)); + server_pool_.emplace_back(&HttpServer::run, &server_); } } @@ -170,7 +170,7 @@ MetadataReporter::MetadataReporter(const MetadataAgent& agent, double period_s) environment_(agent.config_), auth_(environment_), period_(period_s), - reporter_thread_(std::bind(&MetadataReporter::ReportMetadata, this)) {} + reporter_thread_(&MetadataReporter::ReportMetadata, this) {} MetadataReporter::~MetadataReporter() { reporter_thread_.join(); diff --git a/src/updater.cc b/src/updater.cc index 6c9e5709..f3875d7d 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -43,7 +43,7 @@ void PollingMetadataUpdater::start() { LOG(INFO) << "Timer locked"; } reporter_thread_ = - std::thread(std::bind(&PollingMetadataUpdater::PollForMetadata, this)); + std::thread(&PollingMetadataUpdater::PollForMetadata, this); } void PollingMetadataUpdater::stop() { From 1edb94fdc0a9bde4892a6e34320481ef61347638 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Mon, 13 Nov 2017 15:21:09 -0500 Subject: [PATCH 4/6] Factor out streaming operators for request/response headers. --- src/api_server.cc | 17 +--------------- src/http_common.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++ src/oauth2.cc | 14 +------------ 3 files changed, 53 insertions(+), 29 deletions(-) create mode 100644 src/http_common.h diff --git a/src/api_server.cc b/src/api_server.cc index f8cf1502..aebf59c5 100644 --- a/src/api_server.cc +++ b/src/api_server.cc @@ -25,6 +25,7 @@ #include "environment.h" #include "format.h" +#include "http_common.h" #include "json.h" #include "logging.h" #include "oauth2.h" @@ -57,8 +58,6 @@ class MetadataApiServer { private: const MetadataAgent& agent_; }; - friend std::ostream& operator<<( - std::ostream& o, const HttpServer::request::headers_container_type& hv); Handler handler_; HttpServer server_; @@ -87,20 +86,6 @@ class MetadataReporter { std::thread reporter_thread_; }; - -// To allow logging headers. TODO: move to a common location. -std::ostream& operator<<( - std::ostream& o, - const MetadataApiServer::HttpServer::request::headers_container_type& hv) { - o << "["; - for (const auto& h : hv) { - o << " " << h.name << ": " << h.value; - } - o << " ]"; - return o; -} - - MetadataApiServer::Handler::Handler(const MetadataAgent& agent) : agent_(agent) {} diff --git a/src/http_common.h b/src/http_common.h new file mode 100644 index 00000000..b9f0d1b8 --- /dev/null +++ b/src/http_common.h @@ -0,0 +1,51 @@ +/* + * Copyright 2017 Google Inc. + * + * 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. + **/ +#ifndef HTTP_COMMON_H_ +#define HTTP_COMMON_H_ + +#include +#include +#include + +namespace google { + +// Allow logging request headers. +inline std::ostream& operator<<( + std::ostream& o, + const boost::network::http::basic_request::headers_container_type& hv) { + o << "["; + for (const auto& h : hv) { + o << " " << h.name << ": " << h.value; + } + o << " ]"; + return o; +} + +// Allow logging response headers. +inline std::ostream& operator<<( + std::ostream& o, + const boost::network::http::basic_response::headers_container_type& hv) { + o << "["; + for (const auto& h : hv) { + o << " " << h.first << ": " << h.second; + } + o << " ]"; + return o; +} + +} + +#endif // HTTP_COMMON_H_ diff --git a/src/oauth2.cc b/src/oauth2.cc index bdd67725..bcdcd4d4 100644 --- a/src/oauth2.cc +++ b/src/oauth2.cc @@ -28,6 +28,7 @@ #include #include "base64.h" +#include "http_common.h" #include "json.h" #include "logging.h" @@ -145,19 +146,6 @@ double SecondsSinceEpoch( } -// To allow logging headers. TODO: move to a common location. -std::ostream& operator<<( - std::ostream& o, - const http::client::request::headers_container_type& hv) { - o << "["; - for (const auto& h : hv) { - o << " " << h.first << ": " << h.second; - } - o << " ]"; - return o; -} - - json::value OAuth2::ComputeTokenFromCredentials() const { const std::string service_account_email = environment_.CredentialsClientEmail(); From dea4fd0e604d9fcb6e54478fec7bfb6eb1921904 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Tue, 19 Dec 2017 13:27:54 -0500 Subject: [PATCH 5/6] Remove redundant namespace qualifiers. --- src/json.cc | 6 +++--- src/json.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/json.cc b/src/json.cc index b0d8bb20..3c684c99 100644 --- a/src/json.cc +++ b/src/json.cc @@ -319,7 +319,7 @@ class JSONBuilder { std::vector> values() throw(Exception) { TopLevelContext* top_level = dynamic_cast(context_); if (top_level == nullptr) { - throw json::Exception("values() called for an inner context"); + throw Exception("values() called for an inner context"); } return top_level->values(); } @@ -447,7 +447,7 @@ std::vector> Parser::AllFromStream(std::istream& stream) unsigned char* str = yajl_get_error(handle, 1, data, kMax); std::string error_str((const char*)str); yajl_free_error(handle, str); - throw json::Exception(error_str); + throw Exception(error_str); } yajl_free(handle); @@ -464,7 +464,7 @@ std::unique_ptr Parser::FromStream(std::istream& stream) if (all_values.size() > 1) { std::ostringstream out; out << "Getting a single value out of " << all_values.size(); - throw json::Exception(out.str()); + throw Exception(out.str()); } return std::move(all_values[0]); } diff --git a/src/json.h b/src/json.h index bcb494cd..f1a5050e 100644 --- a/src/json.h +++ b/src/json.h @@ -247,13 +247,13 @@ class Object : public Value, public std::map const T* GetField(const std::string& field) const throw(Exception) { auto value_it = find(field); if (value_it == end()) { - throw json::Exception("There is no " + field + " in " + ToString()); + throw Exception("There is no " + field + " in " + ToString()); } if (value_it->second->type() != internal::TypeHelper::type) { constexpr const char* name = internal::TypeHelper::name; constexpr const char* article = internal::ArticleHelper::value(); - throw json::Exception(field + " " + value_it->second->ToString() + - " is not " + article + " " + name); + throw Exception(field + " " + value_it->second->ToString() + + " is not " + article + " " + name); } return value_it->second->As(); } From abd09a421b5d0c95cf870b9d81cd3d0dfc73c757 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Tue, 19 Dec 2017 13:41:15 -0500 Subject: [PATCH 6/6] Tweak error message. --- src/json.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/json.cc b/src/json.cc index 3c684c99..1c186ac5 100644 --- a/src/json.cc +++ b/src/json.cc @@ -463,7 +463,7 @@ std::unique_ptr Parser::FromStream(std::istream& stream) } if (all_values.size() > 1) { std::ostringstream out; - out << "Getting a single value out of " << all_values.size(); + out << "Single value expected, " << all_values.size() << " values seen"; throw Exception(out.str()); } return std::move(all_values[0]);