Applied changes to percent encoding of query parameters in the uri_builder (#126)

* Applied changes to percent encoding of query parameters in the uri_builder

* Applied feedback from review

* Updated API of uri_builder and encoding functions

Author: glynos

include/network/uri/detail/encode.hpp

@@ -105,17 +105,6 @@ OutputIterator encode_path(InputIterator first, InputIterator last,
   return out;
 }
 
-template <typename InputIterator, typename OutputIterator>
-OutputIterator encode_query(InputIterator first, InputIterator last,
-                            OutputIterator out) {
-  auto it = first;
-  while (it != last) {
-    detail::encode_char(*it, out, "/.@&%;=");
-    ++it;
-  }
-  return out;
-}
-
 template <typename InputIterator, typename OutputIterator>
 OutputIterator encode_query_component(InputIterator first, InputIterator last,
                                       OutputIterator out) {
@@ -167,13 +156,6 @@ String encode_path(const String &path) {
   return encoded;
 }
 
-template <class String>
-String encode_query(const String &query) {
-  String encoded;
-  encode_query(std::begin(query), std::end(query), std::back_inserter(encoded));
-  return encoded;
-}
-
 template <class String>
 String encode_fragment(const String &fragment) {
   String encoded;

include/network/uri/uri.hpp

@@ -547,20 +547,60 @@ class uri {
   }
 
   /**
-   * \brief Encodes a sequence according to the rules for encoding a
-   *        query part.
+   * \deprecated Avoid using this function
+   * \brief Equivalent to \c encode_query_component
    * \param first The iterator at first element in the input
    *        sequence.
    * \param last The iterator at end + 1th element in the input
    *        sequence.
    * \param out The iterator at the first element in the output
    *        sequence.
    * \returns The iterator at the end + 1th in the output sequence.
+   * \sa encode_query_commponent
+   * \sa encode_query_key_value_pair
    */
   template <typename InputIter, typename OutputIter>
   static OutputIter encode_query(InputIter first, InputIter last,
                                  OutputIter out) {
-    return detail::encode_query(first, last, out);
+    return encode_query_component(first, last, out);
+  }
+
+  /**
+   * \brief Encodes a sequence according to the rules for encoding a
+   *        query component, including the '=' character.
+   * \param first The iterator at first element in the input
+   *              sequence.
+   * \param last The iterator at end + 1th element in the input
+   *             sequence.
+   * \param out The iterator at the first element in the output
+   *            sequence.
+   * \returns The iterator at the end + 1th in the output sequence.
+   */
+  template <typename InputIter, typename OutputIter>
+  static OutputIter encode_query_component(
+      InputIter first, InputIter last, OutputIter out) {
+    return detail::encode_query_component(first, last, out);
+  }
+
+  /**
+   * \brief Encodes a sequence according to the rules for encoding a
+   *        query key value pair.
+   * \param key_first The iterator at first element in the input
+   *                   sequence.
+   * \param key_last The iterator at end + 1th element in the input
+   *                  sequence.
+   * \param out The iterator at the first element in the output
+   *            sequence.
+   * \returns The iterator at the end + 1th in the output sequence.
+   */
+  template <typename InputIter, typename OutputIter>
+  static OutputIter encode_query_key_value_pair(
+      InputIter key_first, InputIter key_last,
+      InputIter value_first, InputIter value_last,
+      OutputIter out) {
+    out = detail::encode_query_component(key_first, key_last, out);
+    out++ = '=';
+    return detail::encode_query_component(value_first, value_last, out);
   }
 
   /**

include/network/uri/uri_builder.hpp

@@ -182,26 +182,36 @@ class uri_builder {
   uri_builder &clear_path();
 
   /**
+   * \deprecated Use append_query_component
+   * \warning This function's behaviour has changed and percent encoding
+   *          of the '=' character is not ignored.
    * \brief Adds a new query to the uri_builder.
    * \param query The query.
    * \returns \c *this
+   * \sa append_query_parameter
    */
   template <typename Source>
   uri_builder &append_query(const Source &query) {
-    append_query(detail::translate(query));
-    return *this;
+    return append_query_component(query);
   }
 
   /**
-   * \brief Clears the URI query part.
+   * \brief Appends a new query component to the uri_builder. The
+   *        '=' symbol is percent encoded.
+   * \param component The query component.
    * \returns \c *this
+   * \sa append_query_key_value_pair
    */
-  uri_builder &clear_query();
+  template <typename Source>
+  uri_builder &append_query_component(const Source &component) {
+    append_query_component(detail::translate(component));
+    return *this;
+  }
 
   /**
    * \brief Adds a new query key/value pair to the uri_builder.
-   * \param key The query key.
-   * \param value The query value.
+   * \param key The query parameter key.
+   * \param value The query parameter value.
    * \returns \c *this
    */
   template <typename Key, typename Value>
@@ -211,6 +221,12 @@ class uri_builder {
     return *this;
   }
 
+  /**
+   * \brief Clears the URI query part.
+   * \returns \c *this
+   */
+  uri_builder &clear_query();
+
   /**
    * \brief Adds a new fragment to the uri_builder.
    * \param fragment The fragment.
@@ -239,15 +255,15 @@ class uri_builder {
   network::uri uri() const;
 
  private:
-  void set_scheme(string_type scheme);
-  void set_user_info(string_type user_info);
-  void set_host(string_type host);
-  void set_port(string_type port);
-  void set_authority(string_type authority);
-  void set_path(string_type path);
-  void append_query(string_type query);
-  void append_query_key_value_pair(string_type key, string_type value);
-  void set_fragment(string_type fragment);
+  void set_scheme(string_type &&scheme);
+  void set_user_info(string_type &&user_info);
+  void set_host(string_type &&host);
+  void set_port(string_type &&port);
+  void set_authority(string_type &&authority);
+  void set_path(string_type &&path);
+  void append_query_component(string_type &&name);
+  void append_query_key_value_pair(string_type &&key, string_type &&value);
+  void set_fragment(string_type &&fragment);
 
   optional<string_type> scheme_, user_info_, host_, port_, path_, query_,
       fragment_;

src/uri.cpp

@@ -275,9 +275,7 @@ uri::query_iterator::query_iterator(optional<detail::uri_part> query)
 
 uri::query_iterator::query_iterator(const query_iterator &other)
   : query_(other.query_)
-  , kvp_(other.kvp_) {
-
-}
+  , kvp_(other.kvp_) {}
 
 uri::query_iterator &uri::query_iterator::operator = (const query_iterator &other) {
   auto tmp(other);

src/uri_builder.cpp

@@ -12,46 +12,46 @@
 namespace network {
 uri_builder::uri_builder(const network::uri &base_uri) {
   if (base_uri.has_scheme()) {
-    set_scheme(base_uri.scheme().to_string());
+    scheme_ = base_uri.scheme().to_string();
   }
 
   if (base_uri.has_user_info()) {
-    set_user_info(base_uri.user_info().to_string());
+    user_info_ = base_uri.user_info().to_string();
   }
 
   if (base_uri.has_host()) {
-    set_host(base_uri.host().to_string());
+    host_ = base_uri.host().to_string();
   }
 
   if (base_uri.has_port()) {
-    set_port(base_uri.port().to_string());
+    port_ = base_uri.port().to_string();
   }
 
   if (base_uri.has_path()) {
-    set_path(base_uri.path().to_string());
+    path_ = base_uri.path().to_string();
   }
 
   if (base_uri.has_query()) {
-    append_query(base_uri.query().to_string());
+    query_ = base_uri.query().to_string();
   }
 
   if (base_uri.has_fragment()) {
-    set_fragment(base_uri.fragment().to_string());
+    fragment_ = base_uri.fragment().to_string();
   }
 }
 
 uri_builder::~uri_builder() noexcept {}
 
 network::uri uri_builder::uri() const { return network::uri(*this); }
 
-void uri_builder::set_scheme(string_type scheme) {
+void uri_builder::set_scheme(string_type &&scheme) {
   // validate scheme is valid and normalize
   scheme_ = scheme;
   detail::transform(*scheme_, std::begin(*scheme_),
                     [] (char ch) { return std::tolower(ch, std::locale()); });
 }
 
-void uri_builder::set_user_info(string_type user_info) {
+void uri_builder::set_user_info(string_type &&user_info) {
   user_info_ = string_type();
   network::uri::encode_user_info(std::begin(user_info), std::end(user_info),
                                  std::back_inserter(*user_info_));
@@ -62,15 +62,15 @@ uri_builder &uri_builder::clear_user_info() {
   return *this;
 }
 
-void uri_builder::set_host(string_type host) {
+void uri_builder::set_host(string_type &&host) {
   host_ = string_type();
   network::uri::encode_host(std::begin(host), std::end(host),
                             std::back_inserter(*host_));
   detail::transform(*host_, std::begin(*host_),
                     [](char ch) { return std::tolower(ch, std::locale()); });
 }
 
-void uri_builder::set_port(string_type port) {
+void uri_builder::set_port(string_type &&port) {
   port_ = string_type();
   network::uri::encode_port(std::begin(port), std::end(port),
                             std::back_inserter(*port_));
@@ -81,7 +81,7 @@ uri_builder &uri_builder::clear_port() {
   return *this;
 }
 
-void uri_builder::set_authority(string_type authority) {
+void uri_builder::set_authority(string_type &&authority) {
   optional<detail::uri_part> user_info, host, port;
   uri::string_view view(authority);
   uri::const_iterator it = std::begin(view), last = std::end(view);
@@ -100,7 +100,7 @@ void uri_builder::set_authority(string_type authority) {
   }
 }
 
-void uri_builder::set_path(string_type path) {
+void uri_builder::set_path(string_type &&path) {
   path_ = string_type();
   network::uri::encode_path(std::begin(path), std::end(path),
                             std::back_inserter(*path_));
@@ -111,36 +111,35 @@ uri_builder &uri_builder::clear_path() {
   return *this;
 }
 
-void uri_builder::append_query(string_type query) {
+void uri_builder::append_query_component(string_type &&name) {
   if (!query_) {
     query_ = string_type();
   }
   else {
     query_->append("&");
   }
-  network::uri::encode_query(std::begin(query), std::end(query),
-                             std::back_inserter(*query_));
+  network::uri::encode_query_component(
+      std::begin(name), std::end(name), std::back_inserter(*query_));
 }
 
-void uri_builder::append_query_key_value_pair(string_type key, string_type value) {
+void uri_builder::append_query_key_value_pair(string_type &&key, string_type &&value) {
   if (!query_) {
     query_ = string_type();
   } else {
     query_->push_back('&');
   }
-  detail::encode_query_component(std::begin(key), std::end(key),
-                                 std::back_inserter(*query_));
-  query_->push_back('=');
-  detail::encode_query_component(std::begin(value), std::end(value),
-                                 std::back_inserter(*query_));
+  network::uri::encode_query_key_value_pair(
+      std::begin(key), std::end(key),
+      std::begin(value), std::end(value),
+      std::back_inserter(*query_));
 }
 
 uri_builder &uri_builder::clear_query() {
   query_ = network::nullopt;
   return *this;
 }
 
-void uri_builder::set_fragment(string_type fragment) {
+void uri_builder::set_fragment(string_type &&fragment) {
   fragment_ = string_type();
   network::uri::encode_fragment(std::begin(fragment), std::end(fragment),
                                 std::back_inserter(*fragment_));