From 51e42ded446998dd6e6846de2ccea59342fff6d2 Mon Sep 17 00:00:00 2001 From: Dennis Luxen Date: Thu, 26 Feb 2015 10:11:33 +0100 Subject: [PATCH] - output only get escaped when actually output. Better seperation of functionality - refactor facade::GetEscapeName() into get_name_for_id() call that is implemented in subclasses - remove dead code - fix failing tests where names got double-escaped - fixes https://github.com/Project-OSRM/node-osrm/issues/83 --- algorithms/route_name_extraction.hpp | 12 +++-- descriptors/descriptor_base.hpp | 2 + descriptors/json_descriptor.hpp | 18 +++----- plugins/nearest.hpp | 10 ++--- server/data_structures/datafacade_base.hpp | 9 +--- .../data_structures/internal_datafacade.hpp | 15 ++++--- server/data_structures/shared_datafacade.hpp | 12 ++--- util/json_renderer.hpp | 45 +++---------------- util/string_util.hpp | 11 ++++- 9 files changed, 50 insertions(+), 84 deletions(-) diff --git a/algorithms/route_name_extraction.hpp b/algorithms/route_name_extraction.hpp index 8275de099..00dae89c4 100644 --- a/algorithms/route_name_extraction.hpp +++ b/algorithms/route_name_extraction.hpp @@ -1,6 +1,6 @@ /* -Copyright (c) 2013, Project OSRM contributors +Copyright (c) 2015, Project OSRM contributors All rights reserved. Redistribution and use in source and binary forms, with or without modification, @@ -147,15 +147,13 @@ template struct ExtractRouteNames } // fetching names for the selected segments - route_names.shortest_path_name_1 = - facade->GetEscapedNameForNameID(shortest_segment_1.name_id); - route_names.shortest_path_name_2 = - facade->GetEscapedNameForNameID(shortest_segment_2.name_id); + route_names.shortest_path_name_1 = facade->get_name_for_id(shortest_segment_1.name_id); + route_names.shortest_path_name_2 = facade->get_name_for_id(shortest_segment_2.name_id); route_names.alternative_path_name_1 = - facade->GetEscapedNameForNameID(alternative_segment_1.name_id); + facade->get_name_for_id(alternative_segment_1.name_id); route_names.alternative_path_name_2 = - facade->GetEscapedNameForNameID(alternative_segment_2.name_id); + facade->get_name_for_id(alternative_segment_2.name_id); return route_names; } diff --git a/descriptors/descriptor_base.hpp b/descriptors/descriptor_base.hpp index 70d8f1c04..497c2161d 100644 --- a/descriptors/descriptor_base.hpp +++ b/descriptors/descriptor_base.hpp @@ -33,6 +33,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "../data_structures/phantom_node.hpp" #include "../typedefs.h" +#include + #include #include diff --git a/descriptors/json_descriptor.hpp b/descriptors/json_descriptor.hpp index 35dd9c91e..352b55a94 100644 --- a/descriptors/json_descriptor.hpp +++ b/descriptors/json_descriptor.hpp @@ -113,9 +113,6 @@ template class JSONDescriptor final : public BaseDescriptor< } // check if first segment is non-zero - std::string road_name = facade->GetEscapedNameForNameID( - raw_route.segment_end_coordinates.front().source_phantom.name_id); - BOOST_ASSERT(raw_route.unpacked_path_segments.size() == raw_route.segment_end_coordinates.size()); @@ -157,9 +154,9 @@ template class JSONDescriptor final : public BaseDescriptor< json_route_summary.values["total_distance"] = description_factory.summary.distance; json_route_summary.values["total_time"] = description_factory.summary.duration; json_route_summary.values["start_point"] = - facade->GetEscapedNameForNameID(description_factory.summary.source_name_id); + facade->get_name_for_id(description_factory.summary.source_name_id); json_route_summary.values["end_point"] = - facade->GetEscapedNameForNameID(description_factory.summary.target_name_id); + facade->get_name_for_id(description_factory.summary.target_name_id); json_result.values["route_summary"] = json_route_summary; BOOST_ASSERT(!raw_route.segment_end_coordinates.empty()); @@ -241,10 +238,10 @@ template class JSONDescriptor final : public BaseDescriptor< alternate_description_factory.summary.distance; json_alternate_route_summary.values["total_time"] = alternate_description_factory.summary.duration; - json_alternate_route_summary.values["start_point"] = facade->GetEscapedNameForNameID( - alternate_description_factory.summary.source_name_id); - json_alternate_route_summary.values["end_point"] = facade->GetEscapedNameForNameID( - alternate_description_factory.summary.target_name_id); + json_alternate_route_summary.values["start_point"] = + facade->get_name_for_id(alternate_description_factory.summary.source_name_id); + json_alternate_route_summary.values["end_point"] = + facade->get_name_for_id(alternate_description_factory.summary.target_name_id); json_alternate_route_summary_array.values.push_back(json_alternate_route_summary); json_result.values["alternative_summaries"] = json_alternate_route_summary_array; @@ -349,8 +346,7 @@ template class JSONDescriptor final : public BaseDescriptor< } json_instruction_row.values.push_back(current_turn_instruction); - json_instruction_row.values.push_back( - facade->GetEscapedNameForNameID(segment.name_id)); + json_instruction_row.values.push_back(facade->get_name_for_id(segment.name_id)); json_instruction_row.values.push_back(std::round(segment.length)); json_instruction_row.values.push_back(necessary_segments_running_index); json_instruction_row.values.push_back(std::round(segment.duration / 10.)); diff --git a/plugins/nearest.hpp b/plugins/nearest.hpp index 81aa01e9c..bf4cff3a1 100644 --- a/plugins/nearest.hpp +++ b/plugins/nearest.hpp @@ -88,9 +88,8 @@ template class NearestPlugin final : public BasePlugin json_coordinate.values.push_back(phantom_node_vector.at(i).location.lon / COORDINATE_PRECISION); result.values["mapped coordinate"] = json_coordinate; - std::string temp_string; - facade->GetName(phantom_node_vector.at(i).name_id, temp_string); - result.values["name"] = temp_string; + result.values["name"] = + facade->get_name_for_id(phantom_node_vector.at(i).name_id); results.values.push_back(result); } json_result.values["results"] = results; @@ -103,9 +102,8 @@ template class NearestPlugin final : public BasePlugin json_coordinate.values.push_back(phantom_node_vector.front().location.lon / COORDINATE_PRECISION); json_result.values["mapped_coordinate"] = json_coordinate; - std::string temp_string; - facade->GetName(phantom_node_vector.front().name_id, temp_string); - json_result.values["name"] = temp_string; + json_result.values["name"] = + facade->get_name_for_id(phantom_node_vector.front().name_id); } } return 200; diff --git a/server/data_structures/datafacade_base.hpp b/server/data_structures/datafacade_base.hpp index 977f818a4..0a2d22025 100644 --- a/server/data_structures/datafacade_base.hpp +++ b/server/data_structures/datafacade_base.hpp @@ -109,14 +109,7 @@ template class BaseDataFacade virtual unsigned GetNameIndexFromEdgeID(const unsigned id) const = 0; - virtual void GetName(const unsigned name_id, std::string &result) const = 0; - - std::string GetEscapedNameForNameID(const unsigned name_id) const - { - std::string temporary_string; - GetName(name_id, temporary_string); - return EscapeJSONString(temporary_string); - } + virtual std::string get_name_for_id(const unsigned name_id) const = 0; virtual std::string GetTimestamp() const = 0; }; diff --git a/server/data_structures/internal_datafacade.hpp b/server/data_structures/internal_datafacade.hpp index 3c8a57c68..746402418 100644 --- a/server/data_structures/internal_datafacade.hpp +++ b/server/data_structures/internal_datafacade.hpp @@ -46,6 +46,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include +#include + template class InternalDataFacade final : public BaseDataFacade { @@ -418,24 +420,25 @@ template class InternalDataFacade final : public BaseDataFacad unsigned GetNameIndexFromEdgeID(const unsigned id) const override final { return m_name_ID_list.at(id); - }; + } - void GetName(const unsigned name_id, std::string &result) const override final + std::string get_name_for_id(const unsigned name_id) const override final { - if (UINT_MAX == name_id) + if (std::numeric_limits::max() == name_id) { - result = ""; - return; + return ""; } auto range = m_name_table.GetRange(name_id); - result.clear(); + std::string result; + result.reserve(range.size()); if (range.begin() != range.end()) { result.resize(range.back() - range.front() + 1); std::copy(m_names_char_list.begin() + range.front(), m_names_char_list.begin() + range.back() + 1, result.begin()); } + return result; } virtual unsigned GetGeometryIndexForEdgeID(const unsigned id) const override final diff --git a/server/data_structures/shared_datafacade.hpp b/server/data_structures/shared_datafacade.hpp index 468db30e2..fcec478b2 100644 --- a/server/data_structures/shared_datafacade.hpp +++ b/server/data_structures/shared_datafacade.hpp @@ -41,6 +41,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "../../util/simple_logger.hpp" #include +#include #include template class SharedDataFacade final : public BaseDataFacade @@ -409,22 +410,23 @@ template class SharedDataFacade final : public BaseDataFacade< return m_name_ID_list.at(id); }; - void GetName(const unsigned name_id, std::string &result) const override final + std::string get_name_for_id(const unsigned name_id) const override final { - if (UINT_MAX == name_id) + if (std::numeric_limits::max() == name_id) { - result = ""; - return; + return ""; } auto range = m_name_table->GetRange(name_id); - result.clear(); + std::string result; + result.reserve(range.size()); if (range.begin() != range.end()) { result.resize(range.back() - range.front() + 1); std::copy(m_names_char_list.begin() + range.front(), m_names_char_list.begin() + range.back() + 1, result.begin()); } + return result; } std::string GetTimestamp() const override final { return m_timestamp; } diff --git a/util/json_renderer.hpp b/util/json_renderer.hpp index 5a49d3c98..e5bbf6f92 100644 --- a/util/json_renderer.hpp +++ b/util/json_renderer.hpp @@ -32,6 +32,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #define JSON_RENDERER_HPP #include "cast.hpp" +#include "string_util.hpp" #include @@ -40,31 +41,14 @@ namespace osrm namespace json { - struct Renderer : mapbox::util::static_visitor<> { explicit Renderer(std::ostream &_out) : out(_out) {} - void operator()(const String &string) const { + void operator()(const String &string) const + { out << "\""; - - // check if we need escaping - std::size_t pos = string.value.find_first_of('\\'); - if (pos != std::string::npos) - { - std::string escapedString(string.value); - do - { - escapedString.insert(pos, 1, '\\'); - pos = escapedString.find_first_of('\\', pos); - } while (pos != std::string::npos); - } - // no need to escape - else - { - out << string.value; - } - + out << escape_JSON(string.value); out << "\""; } @@ -123,25 +107,8 @@ struct ArrayRenderer : mapbox::util::static_visitor<> void operator()(const String &string) const { out.push_back('\"'); - - // check if we need escaping - std::size_t pos = string.value.find_first_of('\\'); - if (pos != std::string::npos) - { - std::string escapedString(string.value); - do - { - escapedString.insert(pos, 1, '\\'); - pos = escapedString.find_first_of('\\', pos+2); - } while (pos != std::string::npos); - out.insert(out.end(), escapedString.begin(), escapedString.end()); - } - // no need to escape - else - { - out.insert(out.end(), string.value.begin(), string.value.end()); - } - + const auto string_to_insert = escape_JSON(string.value); + out.insert(std::end(out), std::begin(string_to_insert), std::end(string_to_insert)); out.push_back('\"'); } diff --git a/util/string_util.hpp b/util/string_util.hpp index 46b6a5a64..31be0444b 100644 --- a/util/string_util.hpp +++ b/util/string_util.hpp @@ -77,10 +77,17 @@ inline void replaceAll(std::string &s, const std::string &sub, const std::string boost::replace_all(s, sub, other); } -inline std::string EscapeJSONString(const std::string &input) +inline std::string escape_JSON(const std::string &input) { + // return the input if no backslash can be found -> no allocations + if (input.find_first_of('\\') == std::string::npos) + { + return input; + } + + // escape and skip reallocations if possible std::string output; - output.reserve(input.size()); + output.reserve(input.size() + 4); // +4 assumes two backslashes on avg for (auto iter = input.begin(); iter != input.end(); ++iter) { switch (iter[0])