Fix rfc2616 Section 4.2 compliance:

basic_headers no longer combines fields with the same name by appending
a comma and concatenating the two values together. This was breaking
certain header fields which expect each value to be distinct, such as
the "Set-Cookie" header.

Now the container behaves more like a multi set with respect to insertion
of multiple values with the same field name. Additional member functions
are provided to provide extra functionality.

CHANGELOG

@@ -1,6 +1,7 @@
 1.0.0-b8
 
 * Fix include in example code
+* Fix basic_headers rfc2616 Section 4.2 compliance
 
 --------------------------------------------------------------------------------
 

include/beast/http/basic_headers.hpp

@@ -105,7 +105,7 @@ protected:
     using list_t = typename boost::intrusive::make_list<
         element, boost::intrusive::constant_time_size<false>>::type;
 
-    using set_t = typename boost::intrusive::make_set<
+    using set_t = typename boost::intrusive::make_multiset<
         element, boost::intrusive::constant_time_size<true>,
             boost::intrusive::compare<less>>::type;
 
@@ -117,7 +117,6 @@ protected:
         : set_(std::move(set))
         , list_(std::move(list))
     {
-
     }
 
 public:
@@ -244,9 +243,10 @@ public:
     value.
 
     Field names are stored as-is, but comparison are case-insensitive.
-    The container preserves the order of insertion of fields with
+    When the container is iterated, the fields are presented in the order
-    different names. For fields with the same name, the implementation
+    of insertion. For fields with the same name, the container behaves
-    concatenates values inserted with duplicate names as per rfc7230.
+    as a std::multiset; there will be a separate value for each occurrence
+    of the field name.
 
     @note Meets the requirements of @b `FieldSequence`.
 */
@@ -359,27 +359,44 @@ public:
         return set_.size();
     }
 
-    /** Returns `true` if the specified field exists. */
+    /// Returns `true` if the specified field exists.
     bool
     exists(boost::string_ref const& name) const
     {
         return set_.find(name, less{}) != set_.end();
     }
 
-    /** Returns an iterator to the case-insensitive matching header. */
+    /// Returns the number of values for the specified field.
+    std::size_t
+    count(boost::string_ref const& name) const;
+
+    /** Returns an iterator to the case-insensitive matching field name.
+
+        If more than one field with the specified name exists, the
+        first field defined by insertion order is returned.
+    */
     iterator
     find(boost::string_ref const& name) const;
 
-    /** Returns the value for a case-insensitive matching header, or "" */
+    /** Returns the value for a case-insensitive matching header, or `""`.
+
+        If more than one field with the specified name exists, the
+        first field defined by insertion order is returned.
+    */
     boost::string_ref
     operator[](boost::string_ref const& name) const;
 
-    /** Clear the contents of the basic_headers. */
+    /// Clear the contents of the basic_headers.
     void
     clear() noexcept;
 
     /** Remove a field.
 
+        If more than one field with the specified name exists, all
+        matching fields will be removed.
+
+        @param name The name of the field(s) to remove.
+
         @return The number of fields removed.
     */
     std::size_t
@@ -387,39 +404,57 @@ public:
 
     /** Insert a field value.
 
-        If a field value already exists the new value will be
+        If a field with the same name already exists, the
-        extended as per RFC2616 Section 4.2.
+        existing field is untouched and a new field value pair
+        is inserted into the container.
+
+        @param name The name of the field.
+
+        @param value A string holding the value of the field.
     */
-    // VFALCO TODO Consider allowing rvalue references for std::move?
     void
     insert(boost::string_ref const& name, boost::string_ref value);
 
     /** Insert a field value.
 
-        If a field value already exists the new value will be
+        If a field with the same name already exists, the
-        extended as per RFC2616 Section 4.2.
+        existing field is untouched and a new field value pair
+        is inserted into the container.
+
+        @param name The name of the field
+
+        @param value The value of the field. The object will be
+        converted to a string using `boost::lexical_cast`.
     */
     template<class T>
     typename std::enable_if<
         ! std::is_constructible<boost::string_ref, T>::value>::type
     insert(boost::string_ref name, T const& value)
     {
-        insert(name,
+        insert(name, boost::lexical_cast<std::string>(value));
-            boost::lexical_cast<std::string>(value));
     }
 
     /** Replace a field value.
 
-        The current field value, if any, is removed. Then the
+        First removes any values with matching field names, then
-        specified value is inserted as if by `insert(field, value)`.
+        inserts the new field value.
+
+        @param name The name of the field.
+
+        @param value A string holding the value of the field.
     */
     void
     replace(boost::string_ref const& name, boost::string_ref value);
 
     /** Replace a field value.
 
-        The current field value, if any, is removed. Then the
+        First removes any values with matching field names, then
-        specified value is inserted as if by `insert(field, value)`.
+        inserts the new field value.
+
+        @param name The name of the field
+
+        @param value The value of the field. The object will be
+        converted to a string using `boost::lexical_cast`.
     */
     template<class T>
     typename std::enable_if<

include/beast/http/impl/basic_headers.ipp

@@ -9,6 +9,7 @@
 #define BEAST_HTTP_IMPL_BASIC_HEADERS_IPP
 
 #include <beast/http/detail/rfc7230.hpp>
+#include <algorithm>
 
 namespace beast {
 namespace http {
@@ -204,6 +205,18 @@ basic_headers(FwdIt first, FwdIt last)
         insert(first->name(), first->value());
 }
 
+template<class Allocator>
+std::size_t
+basic_headers<Allocator>::
+count(boost::string_ref const& name) const
+{
+    auto const it = set_.find(name, less{});
+    if(it == set_.end())
+        return 0;
+    auto const last = set_.upper_bound(name, less{});
+    return static_cast<std::size_t>(std::distance(it, last));
+}
+
 template<class Allocator>
 auto
 basic_headers<Allocator>::
@@ -221,11 +234,9 @@ boost::string_ref
 basic_headers<Allocator>::
 operator[](boost::string_ref const& name) const
 {
-    // VFALCO This none object looks sketchy
-    static boost::string_ref const none;
     auto const it = find(name);
     if(it == end())
-        return none;
+        return {};
     return it->second;
 }
 
@@ -244,15 +255,23 @@ std::size_t
 basic_headers<Allocator>::
 erase(boost::string_ref const& name)
 {
-    auto const it = set_.find(name, less{});
+    auto it = set_.find(name, less{});
     if(it == set_.end())
         return 0;
-    auto& e = *it;
+    auto const last = set_.upper_bound(name, less{});
-    set_.erase(set_.iterator_to(e));
+    std::size_t n = 1;
-    list_.erase(list_.iterator_to(e));
+    for(;;)
-    alloc_traits::destroy(this->member(), &e);
+    {
-    alloc_traits::deallocate(this->member(), &e, 1);
+        auto& e = *it++;
-    return 1;
+        set_.erase(set_.iterator_to(e));
+        list_.erase(list_.iterator_to(e));
+        alloc_traits::destroy(this->member(), &e);
+        alloc_traits::deallocate(this->member(), &e, 1);
+        if(it == last)
+            break;
+        ++n;
+    }
+    return n;
 }
 
 template<class Allocator>
@@ -262,25 +281,10 @@ insert(boost::string_ref const& name,
     boost::string_ref value)
 {
     value = detail::trim(value);
-    typename set_t::insert_commit_data d;
+    auto const p = alloc_traits::allocate(this->member(), 1);
-    auto const result =
+    alloc_traits::construct(this->member(), p, name, value);
-        set_.insert_check(name, less{}, d);
+    set_.insert_before(set_.upper_bound(name, less{}), *p);
-    if(result.second)
+    list_.push_back(*p);
-    {
-        auto const p = alloc_traits::allocate(
-            this->member(), 1);
-        alloc_traits::construct(
-            this->member(), p, name, value);
-        list_.push_back(*p);
-        set_.insert_commit(*p, d);
-        return;
-    }
-    // If field already exists, insert comma
-    // separated value as per RFC2616 section 4.2
-    auto& cur = result.first->data.second;
-    cur.reserve(cur.size() + 1 + value.size());
-    cur.append(1, ',');
-    cur.append(value.data(), value.size());
 }
 
 template<class Allocator>

test/http/basic_headers.cpp

@@ -63,9 +63,23 @@ public:
     void testRFC2616()
     {
         bh h;
+        h.insert("a", "w");
         h.insert("a", "x");
-        h.insert("a", "y");
+        h.insert("aa", "y");
-        expect(h["a"] == "x,y");
+        h.insert("b", "z");
+        expect(h.count("a") == 2);
+    }
+
+    void testErase()
+    {
+        bh h;
+        h.insert("a", "w");
+        h.insert("a", "x");
+        h.insert("aa", "y");
+        h.insert("b", "z");
+        expect(h.size() == 4);
+        h.erase("a");
+        expect(h.size() == 2);
     }
 
     void run() override