NetServices: Rewrite BHttpFields to use raw strings as underlying data storage

This change also drops the principle that fields with the same keys would be
grouped together. This was initially inspired by Boost::Beast, but it means a
lot of extra copying of data when adding/organizing the list, as well as
inefficient querying on each add. Now that the design choice is to fully go
for the raw string as underlying data storage, that choice is not necessary.
In the future it may be able to emulate the grouping or retrieving of lists
of values in the API, rather than as a fundamental principle of the data
storage.

Change-Id: I2667cfa38eb3b7b75393ee71fb038231a40b4193

docs/user/netservices/HttpFields.dox

@@ -42,9 +42,8 @@ namespace Network {
 	to valid data for the lifetime of this object, which in case of a HTTP response, will be
 	bound to the lifetime of the object that contains the HTTP response.
 
-	When adding headers, the fields are stored in the order in which they were added. However,
-	when you add additional values with an existing name, the new field will be added below the
-	existing field.
+	When adding headers, the fields are stored in the order in which they were added. You can use
+	\ref AddField() to add more than one field with the same key.
 
 	The HTTP protocol does not prohibit multiple fields with the same name, but it does note that
 	semantically this is only allowed for a limited set of explicitly named headers, like the
@@ -163,6 +162,27 @@ namespace Network {
 */
 
 
+/*!
+	\fn BHttpFields::Field::Field(BString& field)
+	\brief Construct a field from the raw \a field value.
+
+	The raw header field is checked to determine whether it corresponds to the the HTTP
+	specification. Note that the raw field should not include any newline characters at the end
+	of the string.
+
+	If succesful, the string is moved into the fields object, and the original input value will be
+	empty.
+
+	\param field The raw header field to move into the list of headers 
+
+	\exception std::bad_alloc Error in case memory cannot be allocated.
+	\exception BHttpFields::InvalidInput This error indicates that the \a name or the \a value
+		is empty or contains invalid characters.
+
+	\since Haiku R1
+*/
+
+
 /*!
 	\fn BHttpFields::Field::Field(const Field &other)
 	\brief Copy constructor.
@@ -221,7 +241,18 @@ namespace Network {
 	\fn std::string_view BHttpFields::Field::Value() const noexcept
 	\brief Get a const reference to the field value.
 
-	\return The contents of the field value as a \a std::string_view.
+	\return The contents of the field value as a \c std::string_view.
+
+	\since Haiku R1
+*/
+
+
+/*!
+	\fn std::string_view BHttpFields::Field::RawField() const noexcept
+	\brief Get a view to the field value.
+
+	\return The raw field value as a \c string_view. The raw value does not include the line
+		ending (\\r\\n).
 
 	\since Haiku R1
 */
@@ -254,16 +285,6 @@ namespace Network {
 */
 
 
-/*!
-	\fn BHttpFields::FieldName::operator BString() const
-	\brief Return a copy of the header name as a string.
-
-	\return The header name as a \ref BString object.
-
-	\since Haiku R1
-*/
-
-
 /*!
 	\fn BHttpFields::FieldName::operator std::string_view() const
 	\brief Return a \c std::string_view over the header name.
@@ -336,11 +357,12 @@ namespace Network {
 
 	This enables you to initialize the fields with a list of \ref BHttpFields::Field objects. Any
 	empty fields will be skipped. Like \ref AddField(), this constructor keeps the fields in the
-	original order, though duplicate keys will be grouped together in sequence.
+	original order.
 
 	The example below will create an object with four fields, even though five fields have been
-	passed in the initializer. The last header will be reorderd to follow the other
-	\c Accept-Encoding header.
+	passed in the initializer. The two \c Accept-Encoding will be added in this order, even though
+	the HTTP specification does not explicitly allow this.
+
 	\code
 	const BHttpFields defaultFields = {
 		{"Host"sv, "haiku-os.org"sv},
@@ -476,14 +498,33 @@ namespace Network {
 */
 
 
+/*!
+	\fn void BHttpFields::AddField(BString &field)
+	\brief Append a field from the raw \a field line
+
+	The raw header field is checked to determine whether it corresponds to the the HTTP
+	specification. Note that the raw field should not include any newline characters at the end
+	of the string.
+
+	If succesful, the string is moved into the fields object, and the original input value will be
+	empty.
+
+	\param field The raw header field to move into the list of headers.
+
+	\exception std::bad_alloc Error in case memory cannot be allocated.
+	\exception BHttpFields::InvalidInput This error indicates that the \a name or the \a value
+		is empty or contains invalid characters.
+
+	\since Haiku R1
+*/
+
+
 /*!
 	\fn void BHttpFields::AddFields(std::initializer_list< Field > fields)
 	\brief Add a list of fields.
 
 	This enables you to add a list of \ref BHttpFields::Field objects. Like \ref AddField(), the
-	fields are added in the the original order, though if there are duplicate keys within the
-	\a fields list, or there are existing keys in the object, they will be grouped together in
-	sequence.
+	fields are added in the the original order.
 
 	\exception std::bad_alloc Error in case memory cannot be allocated.
 	\exception BHttpFields::InvalidInput This error indicates that some of the names or values in
@@ -541,9 +582,8 @@ namespace Network {
 	\fn ConstIterator BHttpFields::FindField(const std::string_view &name) const noexcept
 	\brief Find a field with \a name.
 
-	In case there are more than one fields with the same name, this container will make sure that
-	these are grouped together. That means that you can use the properties of the iterator to find
-	the other fields.
+	In case there are more than one fields with the same name, you cannot use this method to find
+	all instances, and you should iterate through the fields instead.
 
 	\param name The name of the field to be found.
 

headers/private/netservices2/HttpFields.h

@@ -7,6 +7,7 @@
 #define _B_HTTP_FIELDS_H_
 
 #include <list>
+#include <optional>
 #include <string_view>
 #include <variant>
 #include <vector>
@@ -42,19 +43,17 @@ public:
 				bool			operator==(const FieldName& other) const noexcept;
 
 		// Conversion
-								operator BString() const;
 								operator std::string_view() const;
 	private:
 		friend	class			BHttpFields;
-								FieldName(const std::string_view& name);
-								FieldName(BString name);
-								FieldName(const FieldName& other);
+								FieldName() noexcept;
+								FieldName(const std::string_view& name) noexcept;
+								FieldName(const FieldName& other) noexcept;
 								FieldName(FieldName&&) noexcept;
-		FieldName&				operator=(const FieldName& other);
+		FieldName&				operator=(const FieldName& other) noexcept;
 		FieldName&				operator=(FieldName&&) noexcept;
-		FieldName&				operator=(BString name);
 
-		std::variant<std::string_view, BString>	fName;
+		std::string_view		fName;
 	};
 
 	class Field {
@@ -62,6 +61,7 @@ public:
 		// Constructors
 								Field() noexcept;
 								Field(const std::string_view& name, const std::string_view& value);
+								Field(BString& field);
 								Field(const Field& other);
 								Field(Field&&) noexcept;
 
@@ -72,54 +72,56 @@ public:
 		// Access Operators
 		const FieldName&		Name() const noexcept;
 		std::string_view		Value() const noexcept;
+		std::string_view		RawField() const noexcept;
 		bool					IsEmpty() const noexcept;
 
 	private:
 		friend	class			BHttpFields;
 
-								Field(const std::string_view& name, const std::string_view& value, bool borrowed);
+								Field(BString&& rawField);
+
+		std::optional<BString>	fRawField;
 
 		FieldName				fName;
-		std::variant<std::string_view, BString>
-						fValue;
+		std::string_view		fValue;
 	};
 
 	// Type Aliases
 	using ConstIterator = std::list<Field>::const_iterator;
 
 	// Constructors & Destructor
-						BHttpFields();
-						BHttpFields(std::initializer_list<Field> fields);
-						BHttpFields(const BHttpFields& other);
-						BHttpFields(BHttpFields&& other);
-						~BHttpFields() noexcept;
+								BHttpFields();
+								BHttpFields(std::initializer_list<Field> fields);
+								BHttpFields(const BHttpFields& other);
+								BHttpFields(BHttpFields&& other);
+								~BHttpFields() noexcept;
 
 	// Assignment operators
-	BHttpFields&		operator=(const BHttpFields&);
-	BHttpFields&		operator=(BHttpFields&&) noexcept;
+			BHttpFields&		operator=(const BHttpFields&);
+			BHttpFields&		operator=(BHttpFields&&) noexcept;
 
 	// Access list
-	const	Field&		operator[](size_t index) const;
+	const	Field&				operator[](size_t index) const;
 
 	// Modifiers
-	void				AddField(const std::string_view& name, const std::string_view& value);
-	void				AddFields(std::initializer_list<Field> fields);
-	void				RemoveField(const std::string_view& name) noexcept; 
-	void				RemoveField(ConstIterator it) noexcept;
-	void				MakeEmpty() noexcept;
+			void				AddField(const std::string_view& name,
+									const std::string_view& value);
+			void				AddField(BString& field);
+			void				AddFields(std::initializer_list<Field> fields);
+			void				RemoveField(const std::string_view& name) noexcept; 
+			void				RemoveField(ConstIterator it) noexcept;
+			void				MakeEmpty() noexcept;
 
 	// Querying
-	ConstIterator		FindField(const std::string_view& name) const noexcept;
-	size_t				CountFields() const noexcept;
+			ConstIterator		FindField(const std::string_view& name) const noexcept;
+			size_t				CountFields() const noexcept;
 
 	// Range-based iteration
-	ConstIterator		begin() const noexcept;
-	ConstIterator		end() const noexcept;
+			ConstIterator		begin() const noexcept;
+			ConstIterator		end() const noexcept;
 
 private:
-	void				_AddField(Field&& field);
-
-	std::list<Field>	fFields;
+			std::list<Field>	fFields;
 };
 
 

src/kits/network/libnetservices2/HttpFields.cpp

@@ -44,7 +44,7 @@ validate_value_string(const std::string_view& string)
 }
 
 
-/*
+/*!
 	\brief Case insensitively compare two string_views.
 
 	Inspired by https://stackoverflow.com/a/4119881
@@ -61,6 +61,30 @@ iequals(const std::string_view& a, const std::string_view& b)
 }
 
 
+/*!
+	\brief Trim whitespace from the beginning and end of a string_view
+
+	Inspired by:
+		https://terrislinenbach.medium.com/trimming-whitespace-from-a-string-view-6795e18b108f
+*/
+static inline std::string_view
+trim(std::string_view in)
+{
+	auto left = in.begin();
+	for (;; ++left) {
+		if (left == in.end())
+			return std::string_view();
+		if (!isspace(*left))
+			break;
+	}
+
+	auto right = in.end() - 1;
+	for (; right > left && isspace(*right); --right);
+
+	return std::string_view(left, std::distance(left, right) + 1);
+}
+
+
 // #pragma mark -- BHttpFields::InvalidHeader
 
 
@@ -92,28 +116,24 @@ BHttpFields::InvalidInput::DebugMessage() const
 // #pragma mark -- BHttpFields::Name
 
 
-BHttpFields::FieldName::FieldName(const std::string_view& name)
+BHttpFields::FieldName::FieldName() noexcept
+	: fName(std::string_view())
+{
+
+}
+
+
+BHttpFields::FieldName::FieldName(const std::string_view& name) noexcept
 	: fName(name)
 {
 
 }
 
 
-BHttpFields::FieldName::FieldName(BString name)
-	: fName(std::move(name))
-{
-
-}
-
-
 /*!
-	\brief Copy constructor; any borrowed field is copied into an owned field
+	\brief Copy constructor;
 */
-BHttpFields::FieldName::FieldName(const FieldName& other)
-{
-	BString otherName = other;
-	fName = std::move(otherName);
-}
+BHttpFields::FieldName::FieldName(const FieldName& other) noexcept = default;
 
 
 /*!
@@ -131,15 +151,10 @@ BHttpFields::FieldName::FieldName(FieldName&& other) noexcept
 
 
 /*!
-	\brief Copy assignment; the copy is always owned
+	\brief Copy assignment; 
 */
 BHttpFields::FieldName&
-BHttpFields::FieldName::operator=(const BHttpFields::FieldName& other)
-{
-	BString otherName = other;
-	fName = std::move(otherName);
-	return *this;
-}
+BHttpFields::FieldName::operator=(const BHttpFields::FieldName& other) noexcept = default;
 
 
 /*!
@@ -158,71 +173,30 @@ BHttpFields::FieldName::operator=(BHttpFields::FieldName&& other) noexcept
 }
 
 
-/*!
-	\brief Unchecked assignment of owned name
-
-	This should only be used interally when the name is known to be valid!
-*/
-BHttpFields::FieldName&
-BHttpFields::FieldName::operator=(BString name)
-{
-	fName = std::move(name);
-	return *this;
-}
-
-
 bool
 BHttpFields::FieldName::operator==(const BString& other) const noexcept
 {
-	if (std::holds_alternative<std::string_view>(fName)) {
-		return iequals(std::get<std::string_view>(fName), std::string_view(other.String()));
-	} else {
-		return std::get<BString>(fName).ICompare(other) == 0;
-	}
+	return iequals(fName, std::string_view(other.String()));
 }
 
 
 bool
 BHttpFields::FieldName::operator==(const std::string_view& other) const noexcept
 {
-	if (std::holds_alternative<std::string_view>(fName)) {
-		return iequals(std::get<std::string_view>(fName), other);
-	} else {
-		return std::get<BString>(fName).ICompare(other.data(), other.size()) == 0;
-	}
+	return iequals(fName, other);
 }
 
 
 bool
 BHttpFields::FieldName::operator==(const BHttpFields::FieldName& other) const noexcept
 {
-	if (std::holds_alternative<std::string_view>(other.fName)) {
-		return *this == std::get<std::string_view>(other.fName);
-	} else {
-		return *this == std::get<BString>(other.fName);
-	}
-}
-
-
-
-BHttpFields::FieldName::operator BString() const
-{
-	if (std::holds_alternative<std::string_view>(fName)) {
-		const auto& name = std::get<std::string_view>(fName);
-		return BString(name.data(), name.size());
-	} else {
-		return std::get<BString>(fName);
-	}
+	return iequals(fName, other.fName);
 }
 
 
 BHttpFields::FieldName::operator std::string_view() const
 {
-	if (std::holds_alternative<std::string_view>(fName)) {
-		return std::get<std::string_view>(fName);
-	} else {
-		return std::string_view(std::get<BString>(fName).String());
-	}
+	return fName;
 }
 
 
@@ -237,45 +211,63 @@ BHttpFields::Field::Field() noexcept
 
 
 BHttpFields::Field::Field(const std::string_view& name, const std::string_view& value)
-	: Field(name, value, false)
-{
-
-}
-
-
-/*!
-	\brief Internal constructor that has the option to create an instance of a 'borrowed' item.
-*/
-BHttpFields::Field::Field(const std::string_view& name, const std::string_view& value, bool borrowed)
-	: fName(name), fValue(value)
 {
 	if (name.length() == 0 || !validate_http_token_string(name))
-		throw BHttpFields::InvalidInput(__PRETTY_FUNCTION__, fName);
+		throw BHttpFields::InvalidInput(__PRETTY_FUNCTION__, BString(name.data(), name.size()));
 	if (value.length() == 0 || !validate_value_string(value))
 		throw BHttpFields::InvalidInput(__PRETTY_FUNCTION__, BString(value.data(), value.length()));
 
-	if (!borrowed) {
-		// set as owned
-		fName = BHttpFields::FieldName(BString(name.data(), name.length()));
-		fValue = BString(value.data(), value.length());
-	}
+	BString rawField(name.data(), name.size());
+	rawField << ": ";
+	rawField.Append(value.data(), value.size());
+
+	fName = std::string_view(rawField.String(), name.size());
+	fValue = std::string_view(rawField.String() + name.size() + 2, value.size());
+	fRawField = std::move(rawField);
+}
+
+
+BHttpFields::Field::Field(BString& field)
+{
+	// Check if the input contains a key, a separator and a value.
+	auto separatorIndex = field.FindFirst(':');
+	if (separatorIndex <= 0)
+		throw BHttpFields::InvalidInput(__PRETTY_FUNCTION__, field);
+
+	// Get the name and the value. Remove whitespace around the value.
+	auto name = std::string_view(field.String(), separatorIndex);
+	auto value = trim(std::string_view(field.String() + separatorIndex + 1));
+
+	if (name.length() == 0 || !validate_http_token_string(name))
+		throw BHttpFields::InvalidInput(__PRETTY_FUNCTION__, BString(name.data(), name.size()));
+	if (value.length() == 0 || !validate_value_string(value))
+		throw BHttpFields::InvalidInput(__PRETTY_FUNCTION__, BString(value.data(), value.length()));
+
+	fRawField = std::move(field);
+	fName = name;
+	fValue = value;
 }
 
 
 BHttpFields::Field::Field(const BHttpFields::Field& other)
 	: fName(std::string_view()), fValue(std::string_view())
 {
-	if (!other.IsEmpty()) {
-		BString name = other.fName;
-		fName = std::move(name);
-		std::string_view otherValue = other.Value();
-		fValue = BString(otherValue.data(), otherValue.length());
+	if (other.IsEmpty()) {
+		fRawField = BString();
+		fName = std::string_view();
+		fValue = std::string_view();
+	} else {
+		fRawField = other.fRawField;
+		auto nameSize = other.Name().fName.size();
+		auto valueOffset = other.fValue.data() - other.fRawField.value().String();
+		fName = std::string_view((*fRawField).String(), nameSize);
+		fValue = std::string_view((*fRawField).String() + valueOffset, other.fValue.size());
 	}
 }
 
 
 BHttpFields::Field::Field(BHttpFields::Field&& other) noexcept
-	: fName(std::move(other.fName)), fValue(std::move(other.fValue))
+	: fRawField(std::move(other.fRawField)), fName(std::move(other.fName)), fValue(std::move(other.fValue))
 {
 	other.fName.fName = std::string_view();
 	other.fValue = std::string_view();
@@ -286,13 +278,15 @@ BHttpFields::Field&
 BHttpFields::Field::operator=(const BHttpFields::Field& other)
 {
 	if (other.IsEmpty()) {
+		fRawField = BString();
 		fName = std::string_view();
 		fValue = std::string_view();
 	} else {
-		BString name = other.fName;
-		fName = std::move(name);
-		std::string_view otherValue = other.Value();
-		fValue = BString(otherValue.data(), otherValue.length());
+		fRawField = other.fRawField;
+		auto nameSize = other.Name().fName.size();
+		auto valueOffset = other.fValue.data() - other.fRawField.value().String();
+		fName = std::string_view((*fRawField).String(), nameSize);
+		fValue = std::string_view((*fRawField).String() + valueOffset, other.fValue.size());
 	}
 	return *this;
 }
@@ -301,6 +295,7 @@ BHttpFields::Field::operator=(const BHttpFields::Field& other)
 BHttpFields::Field&
 BHttpFields::Field::operator=(BHttpFields::Field&& other) noexcept
 {
+	fRawField = std::move(other.fRawField);
 	fName = std::move(other.fName);
 	other.fName.fName = std::string_view();
 	fValue = std::move(other.fValue);
@@ -319,11 +314,17 @@ BHttpFields::Field::Name() const noexcept
 std::string_view
 BHttpFields::Field::Value() const noexcept
 {
-	if (std::holds_alternative<std::string_view>(fValue)) {
-		return std::get<std::string_view>(fValue);
-	} else {
-		return std::string_view(std::get<BString>(fValue).String());
-	}
+	return fValue;
+}
+
+
+std::string_view
+BHttpFields::Field::RawField() const noexcept
+{
+	if (fRawField)
+		return std::string_view((*fRawField).String(), (*fRawField).Length());
+	else
+		return std::string_view();
 }
 
 
@@ -331,9 +332,7 @@ bool
 BHttpFields::Field::IsEmpty() const noexcept
 {
 	// The object is either fully empty, or it has data, so we only have to check fValue.
-	if (std::holds_alternative<std::string_view>(fValue))
-		return std::get<std::string_view>(fValue).length() == 0;
-	return false;
+	return !fRawField.has_value();
 }
 
 
@@ -348,10 +347,7 @@ BHttpFields::BHttpFields()
 
 BHttpFields::BHttpFields(std::initializer_list<BHttpFields::Field> fields)
 {
-	for (auto& field: fields) {
-		if (!field.IsEmpty())
-			_AddField(Field(field));
-	}
+	AddFields(fields);
 }
 
 
@@ -403,7 +399,14 @@ BHttpFields::operator[](size_t index) const
 void
 BHttpFields::AddField(const std::string_view& name, const std::string_view& value)
 {
-	_AddField(BHttpFields::Field(name, value));
+	fFields.emplace_back(name, value);
+}
+
+
+void
+BHttpFields::AddField(BString& field)
+{
+	fFields.emplace_back(field);
 }
 
 
@@ -412,7 +415,7 @@ BHttpFields::AddFields(std::initializer_list<Field> fields)
 {
 	for (auto& field: fields) {
 		if (!field.IsEmpty())
-			_AddField(Field(field));
+			fFields.push_back(std::move(field));
 	}
 }
 
@@ -470,18 +473,3 @@ BHttpFields::end() const noexcept
 {
 	return fFields.cend();
 }
-
-
-void
-BHttpFields::_AddField(Field&& field)
-{
-	// This could be made more efficient bay adding a set of existing keys to quickly check against
-	auto rIterator = std::find_if(fFields.rbegin(), fFields.rend(), [&, field](const Field& f){
-		return f.Name() == field.Name();
-	});
-
-	if (rIterator == fFields.rend())
-		fFields.push_back(field);
-	else
-		fFields.insert(rIterator.base(), field);
-}

src/kits/network/libnetservices2/HttpRequest.cpp

@@ -282,10 +282,7 @@ BHttpRequest::SerializeHeaderTo(BDataIO* target) const
 	}
 
 	for (const auto& field: outputFields) {
-		std::string_view name = field.Name();
-		bytesWritten += _write_to_dataio(target, name);
-		bytesWritten += _write_to_dataio(target, ": "sv);
-		bytesWritten += _write_to_dataio(target, field.Value());
+		bytesWritten += _write_to_dataio(target, field.RawField());
 		bytesWritten += _write_to_dataio(target, "\r\n"sv);
 	}
 

src/tests/kits/net/netservices2/HttpProtocolTest.cpp

@@ -79,6 +79,52 @@ HttpProtocolTest::HttpFieldsTest()
 		}
 	}
 
+	// Header line parsing validation
+	{
+		auto fields = BHttpFields();
+		try {
+			BString noWhiteSpace("Connection:close");
+			fields.AddField(noWhiteSpace);
+			BString extraWhiteSpace("Connection:     close\t\t  \t");
+			fields.AddField(extraWhiteSpace);
+			for (const auto& field: fields) {
+				std::string_view name = field.Name();
+				CPPUNIT_ASSERT_EQUAL("Connection"sv, name);
+				CPPUNIT_ASSERT_EQUAL("close"sv, field.Value());
+			}
+		} catch (const BHttpFields::InvalidInput& e) {
+			CPPUNIT_FAIL(e.input.String());
+			CPPUNIT_FAIL("Unexpected exception when adding a header with an valid value");
+		}
+
+		try {
+			BString noSeparator("Connection close");
+			fields.AddField(noSeparator);
+		} catch (const BHttpFields::InvalidInput& e) {
+			// success
+		} catch (...) {
+			CPPUNIT_FAIL("Unexpected exception when creating a header with an invalid value");
+		}
+
+		try {
+			BString noName = (":close");
+			fields.AddField(noName);
+		} catch (const BHttpFields::InvalidInput& e) {
+			// success
+		} catch (...) {
+			CPPUNIT_FAIL("Unexpected exception when creating a header with an invalid value");
+		}
+
+		try {
+			BString noValue = ("Connection     :");
+			fields.AddField(noValue);
+		} catch (const BHttpFields::InvalidInput& e) {
+			// success
+		} catch (...) {
+			CPPUNIT_FAIL("Unexpected exception when creating a header with an invalid value");
+		}
+	}
+
 	// Header field name case insensitive comparison
 	{
 		BHttpFields fields = BHttpFields();
@@ -127,30 +173,33 @@ HttpProtocolTest::HttpFieldsTest()
 	// Test query and modification tools
 	{
 		BHttpFields fields = defaultFields;
-		// test order of adding fields
+		// test order of adding fields (in order of construction)
 		fields.AddField("Set-Cookie"sv, "vfxdrm=9lpqrsvxm; Domain=haiku-os.co.uk"sv);
-		// query for Set-Cookie
+		// query for Set-Cookie should find the first in the list
 		auto it = fields.FindField("Set-Cookie"sv);
 		CPPUNIT_ASSERT(it != fields.end());
 		CPPUNIT_ASSERT((*it).Name() == "Set-Cookie"sv);
 		CPPUNIT_ASSERT_EQUAL(defaultFields[2].Value(), (*it).Value());
-		it++;
-		CPPUNIT_ASSERT(it != fields.end());
+
+		// the last item should be the newly insterted one
+		it = fields.end();
+		it--;
+		CPPUNIT_ASSERT(it != fields.begin());
 		CPPUNIT_ASSERT((*it).Name() == "Set-Cookie"sv);
-		CPPUNIT_ASSERT_EQUAL(defaultFields[3].Value(), (*it).Value());
-		it++;
-		CPPUNIT_ASSERT(it != fields.end());
-		CPPUNIT_ASSERT((*it).Name() == "Set-Cookie"sv);
-		it++;
-		CPPUNIT_ASSERT(it != fields.end());
-		CPPUNIT_ASSERT_EQUAL(defaultFields[4].Value(), (*it).Value());
-		// Remove the Accept-Encoding entry by iterator
+		CPPUNIT_ASSERT_EQUAL("vfxdrm=9lpqrsvxm; Domain=haiku-os.co.uk"sv, (*it).Value());
+
+		// the item before should be the Accept-Encoding one
+		it--;
+		CPPUNIT_ASSERT(it != fields.begin());
+		CPPUNIT_ASSERT((*it).Name() == "Accept-Encoding"sv);
+
+		// remove the Accept-Encoding entry by iterator
 		fields.RemoveField(it);
 		CPPUNIT_ASSERT_EQUAL(fields.CountFields(), defaultFields.CountFields());
-		// Remove the Set-Cookie entries by name
+		// remove the Set-Cookie entries by name
 		fields.RemoveField("Set-Cookie"sv);
 		CPPUNIT_ASSERT_EQUAL(fields.CountFields(), 2);
-		// Test MakeEmpty
+		// test MakeEmpty
 		fields.MakeEmpty();
 		CPPUNIT_ASSERT_EQUAL(fields.CountFields(), 0);
 	}
@@ -171,7 +220,7 @@ HttpProtocolTest::HttpFieldsTest()
 			auto value = BString("value");
 			key << count;
 			value << count;
-			CPPUNIT_ASSERT_EQUAL(key, field.Name());
+			CPPUNIT_ASSERT_EQUAL(std::string_view(key.String()), field.Name());
 			CPPUNIT_ASSERT_EQUAL(value, BString(field.Value().data(), field.Value().length()));
 		}
 		CPPUNIT_ASSERT_EQUAL(count, 4);