diff --git a/CHANGELOG b/CHANGELOG index 4c3c0318..0d4aa446 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ 1.0.0-b8 * Fix include in example code +* Fix basic_headers rfc2616 Section 4.2 compliance -------------------------------------------------------------------------------- diff --git a/include/beast/http/basic_headers.hpp b/include/beast/http/basic_headers.hpp index ee6b3b0f..8972ae34 100644 --- a/include/beast/http/basic_headers.hpp +++ b/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>::type; - using set_t = typename boost::intrusive::make_set< + using set_t = typename boost::intrusive::make_multiset< element, boost::intrusive::constant_time_size, boost::intrusive::compare>::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 - different names. For fields with the same name, the implementation - concatenates values inserted with duplicate names as per rfc7230. + When the container is iterated, the fields are presented in the order + of insertion. For fields with the same name, the container behaves + 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 - extended as per RFC2616 Section 4.2. + If a field with the same name already exists, the + 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 - extended as per RFC2616 Section 4.2. + If a field with the same name already exists, the + 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 typename std::enable_if< ! std::is_constructible::value>::type insert(boost::string_ref name, T const& value) { - insert(name, - boost::lexical_cast(value)); + insert(name, boost::lexical_cast(value)); } /** Replace a field value. - The current field value, if any, is removed. Then the - specified value is inserted as if by `insert(field, value)`. + First removes any values with matching field names, then + 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 - specified value is inserted as if by `insert(field, value)`. + First removes any values with matching field names, then + 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 typename std::enable_if< diff --git a/include/beast/http/impl/basic_headers.ipp b/include/beast/http/impl/basic_headers.ipp index 27b9d416..206ed575 100644 --- a/include/beast/http/impl/basic_headers.ipp +++ b/include/beast/http/impl/basic_headers.ipp @@ -9,6 +9,7 @@ #define BEAST_HTTP_IMPL_BASIC_HEADERS_IPP #include +#include namespace beast { namespace http { @@ -204,6 +205,18 @@ basic_headers(FwdIt first, FwdIt last) insert(first->name(), first->value()); } +template +std::size_t +basic_headers:: +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::distance(it, last)); +} + template auto basic_headers:: @@ -221,11 +234,9 @@ boost::string_ref basic_headers:: 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:: 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; - 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); - return 1; + auto const last = set_.upper_bound(name, less{}); + std::size_t n = 1; + for(;;) + { + auto& e = *it++; + 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 @@ -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 result = - set_.insert_check(name, less{}, d); - if(result.second) - { - 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()); + auto const p = alloc_traits::allocate(this->member(), 1); + alloc_traits::construct(this->member(), p, name, value); + set_.insert_before(set_.upper_bound(name, less{}), *p); + list_.push_back(*p); } template diff --git a/test/http/basic_headers.cpp b/test/http/basic_headers.cpp index 2508c4d4..8407b884 100644 --- a/test/http/basic_headers.cpp +++ b/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"); - expect(h["a"] == "x,y"); + h.insert("aa", "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