Add HTTPBodyStream interface, three concrete implementations, and their tests.

util/net/http_body_test.cc

Index: util/net/http_body_test.cc
diff --git a/util/net/http_body_test.cc b/util/net/http_body_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9d7b41c5035a060850bbe15da4544806a5ecb5af
--- /dev/null
+++ b/util/net/http_body_test.cc
@@ -0,0 +1,244 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/net/http_body.h"
+
+#include "gtest/gtest.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+std::string ReadStreamToString(HTTPBodyStream* stream) {

[Mark Mentovai, 2014/10/23 19:31:04]

I would move this down to above ReadASCIIFile, since it’s not used until that
test, and the proximity kind of helps when you read the whole file, in the same
way as “don’t declare variables until you use them” helps.

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+  uint8_t buf[32];
+  std::string result;
+
+  while (stream->HasBytesRemaining()) {
+    ssize_t bytes_read = stream->GetBytesBuffer(buf, sizeof(buf));
+    if (bytes_read == 0) {
+      break;
+    } else if (bytes_read < 0) {
+      ADD_FAILURE() << "Failed to read from stream: " << bytes_read;
+      return std::string();
+    }
+
+    result.append(reinterpret_cast<char*>(buf), bytes_read);
+  }
+
+  return result;
+}
+
+void ExpectStringBuffer(const std::string& expected,

[Mark Mentovai, 2014/10/23 19:31:04]

Ditto, above SmallString.

[Robert Sesek, 2014/10/24 16:53:33]

Done.

+                        const uint8_t* actual,
+                        size_t actual_len) {
+  for (size_t i = 0; i < actual_len; ++i) {

[Mark Mentovai, 2014/10/23 19:31:04]

You could just do:

  std::string actual_string(reinterpret_cast<const char*>(actual), actual_len);
  EXPECT_EQ(expected, actual);

[Robert Sesek, 2014/10/24 16:53:33]

Done.

+    ASSERT_LT(i, expected.length());
+    EXPECT_EQ(expected[i], actual[i]);
+  }
+}
+
+TEST(StringHTTPBodyStreamTest, EmptyString) {

[Mark Mentovai, 2014/10/23 19:31:04]

Except for death tests, the test class name never ends in Test in crashpad,
because it’s just redundant boilerplate noise.

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+  uint8_t buf[32];
+  memset(buf, '!', sizeof(buf));

[Mark Mentovai, 2014/10/23 19:31:04]

#include <string.h>

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+
+  std::string empty_string;
+  StringHTTPBodyStream stream(empty_string);
+  EXPECT_FALSE(stream.HasBytesRemaining());
+  EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf)));
+  EXPECT_EQ('!', buf[0]);

[Mark Mentovai, 2014/10/23 19:31:04]

Check the whole (remainder) of the buffer. Maybe you could have a helper do
this. Same for SmallString and the other cases where you validate the unused
portion of the buffer was actually untouched.

[Robert Sesek, 2014/10/24 16:53:33]

Done.

+  EXPECT_FALSE(stream.HasBytesRemaining());
+}
+
+TEST(StringHTTPBodyStreamTest, SmallString) {
+  uint8_t buf[32];
+  memset(buf, '!', sizeof(buf));
+
+  std::string string("Hello, world");
+  StringHTTPBodyStream stream(string);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+  EXPECT_EQ(static_cast<ssize_t>(string.length()),
+            stream.GetBytesBuffer(buf, sizeof(buf)));
+
+  EXPECT_EQ('!', buf[string.length()]);
+  ExpectStringBuffer(string, buf, string.length());
+
+  EXPECT_FALSE(stream.HasBytesRemaining());
+  EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf)));
+}
+
+TEST(StringHTTPBodyStreamTest, MultipleReads) {
+  uint8_t buf[2];
+  memset(buf, '!', sizeof(buf));
+
+  {
+    std::string string("test");
+    SCOPED_TRACE("aligned buffer boundary");
+
+    StringHTTPBodyStream stream(string);
+    EXPECT_TRUE(stream.HasBytesRemaining());
+    EXPECT_EQ(2, stream.GetBytesBuffer(buf, sizeof(buf)));
+    EXPECT_EQ('t', buf[0]);
+    EXPECT_EQ('e', buf[1]);
+    EXPECT_TRUE(stream.HasBytesRemaining());
+    EXPECT_EQ(2, stream.GetBytesBuffer(buf, sizeof(buf)));
+    EXPECT_EQ('s', buf[0]);
+    EXPECT_EQ('t', buf[1]);
+    EXPECT_FALSE(stream.HasBytesRemaining());
+    EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf)));
+    EXPECT_EQ('s', buf[0]);
+    EXPECT_EQ('t', buf[1]);
+  }
+
+  {
+    std::string string("abc");
+    SCOPED_TRACE("unaligned buffer boundary");
+
+    StringHTTPBodyStream stream(string);
+    EXPECT_TRUE(stream.HasBytesRemaining());
+    EXPECT_EQ(2, stream.GetBytesBuffer(buf, sizeof(buf)));
+    EXPECT_EQ('a', buf[0]);
+    EXPECT_EQ('b', buf[1]);
+    EXPECT_TRUE(stream.HasBytesRemaining());
+    EXPECT_EQ(1, stream.GetBytesBuffer(buf, sizeof(buf)));
+    EXPECT_EQ('c', buf[0]);
+    EXPECT_EQ('b', buf[1]);  // Unmodified from last read.
+    EXPECT_FALSE(stream.HasBytesRemaining());
+    EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf)));
+    EXPECT_EQ('c', buf[0]);
+    EXPECT_EQ('b', buf[1]);
+  }
+}
+
+TEST(FileHTTPBodyStreamTest, ReadASCIIFile) {
+  base::FilePath path = base::FilePath("util/net/testdata/ascii_http_body.txt");

[Mark Mentovai, 2014/10/23 19:31:04]

This is fine for now, but I filed bug 4 to do something better. You should link
to that bug here and in the ReadBinaryFile test.

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+  FileHTTPBodyStream stream(path);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+  std::string contents = ReadStreamToString(&stream);
+  EXPECT_EQ("This is a test.\n", contents);
+  EXPECT_FALSE(stream.HasBytesRemaining());
+}
+
+TEST(FileHTTPBodyStreamTest, ReadBinaryFile) {
+  // HEX contents of file: |FEEDFACE A11A15|.
+  base::FilePath path =
+      base::FilePath("util/net/testdata/binary_http_body.dat");
+  uint8_t buf[4];

[Mark Mentovai, 2014/10/23 19:31:04]

Comment that the buffer size was intentionally chosen so that it would take
multiple reads. (For the string implementation test, no comment was necessary,
because the test name was MultipleReads).

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+
+  FileHTTPBodyStream stream(path);
+
+  EXPECT_TRUE(stream.HasBytesRemaining());
+  memset(buf, '!', sizeof(buf));
+  EXPECT_EQ(4, stream.GetBytesBuffer(buf, sizeof(buf)));
+  EXPECT_EQ(0xfe, buf[0]);
+  EXPECT_EQ(0xed, buf[1]);
+  EXPECT_EQ(0xfa, buf[2]);
+  EXPECT_EQ(0xce, buf[3]);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+
+  memset(buf, '!', sizeof(buf));
+  EXPECT_EQ(3, stream.GetBytesBuffer(buf, sizeof(buf)));
+  EXPECT_EQ(0xa1, buf[0]);
+  EXPECT_EQ(0x1a, buf[1]);
+  EXPECT_EQ(0x15, buf[2]);
+  EXPECT_EQ('!', buf[3]);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+
+  memset(buf, '!', sizeof(buf));
+  EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf)));
+  EXPECT_FALSE(stream.HasBytesRemaining());
+  for (size_t i = 0; i < sizeof(buf); ++i)

[Mark Mentovai, 2014/10/23 19:31:04]

If you had a !-untouched verifier helper, you could use it here. And even on
line 154.

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+    EXPECT_EQ('!', buf[i]) << i;
+}
+
+TEST(FileHTTPBodyStreamTest, NonExistentFile) {
+  base::FilePath path = base::FilePath("/tmp/crashpad/util/net/http_body/null");

[Mark Mentovai, 2014/10/23 19:31:04]

/var/empty, not /tmp

[Robert Sesek, 2014/10/24 16:53:34]

Done.

+  FileHTTPBodyStream stream(path);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+
+  uint8_t buf = 0xff;
+  EXPECT_LT(stream.GetBytesBuffer(&buf, 1), 0);
+  EXPECT_EQ(0xff, buf);
+  EXPECT_FALSE(stream.HasBytesRemaining());
+  EXPECT_LT(stream.GetBytesBuffer(&buf, 1), 0);
+  EXPECT_EQ(0xff, buf);
+  EXPECT_FALSE(stream.HasBytesRemaining());
+}
+
+TEST(CompositeHTTPBodyStreamTest, TwoEmptyStrings) {
+  std::vector<HTTPBodyStream*> parts;
+  parts.push_back(new StringHTTPBodyStream(std::string()));
+  parts.push_back(new StringHTTPBodyStream(std::string()));
+
+  CompositeHTTPBodyStream stream(parts);
+  EXPECT_FALSE(stream.HasBytesRemaining());

[Mark Mentovai, 2014/10/23 19:31:04]

All of your tests call HasBytesRemaining() before trying to get bytes, which
might mask bugs where HasBytesRemaining() alters the state of the object (by
making it usable).

I don’t think that you actually need to expose (or perhaps even implement)
HasBytesRemaining() at all. Merely having the documented behavior be “keep
calling GetBytesBuffer() until it returns 0” should be sufficient. I can’t think
of a reason for a real consumer to ever need to call HasBytesRemaining().

[Robert Sesek, 2014/10/24 16:53:34]

HasBytesRemaining() is now gone.

+
+  uint8_t buf[5];
+  memset(buf, '!', sizeof(buf));
+  EXPECT_EQ(0, stream.GetBytesBuffer(buf, sizeof(buf)));
+  for (size_t i = 0; i < sizeof(buf); ++i)
+    EXPECT_EQ('!', buf[i]) << i;
+
+  EXPECT_FALSE(stream.HasBytesRemaining());
+}
+
+TEST(CompositeHTTPBodyStreamTest, ThreeStringParts) {
+  std::string string1("crashpad");
+  std::string string2("test");
+  std::string string3("foobar");
+  const size_t all_strings_length = string1.length() + string2.length() +
+      string3.length();
+  uint8_t buf[all_strings_length + 3];
+  memset(buf, '!', sizeof(buf));
+
+  std::vector<HTTPBodyStream*> parts;
+  parts.push_back(new StringHTTPBodyStream(string1));
+  parts.push_back(new StringHTTPBodyStream(string2));
+  parts.push_back(new StringHTTPBodyStream(string3));
+
+  CompositeHTTPBodyStream stream(parts);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+
+  std::string actual_string = ReadStreamToString(&stream);
+  EXPECT_EQ(string1 + string2 + string3, actual_string);
+
+  EXPECT_EQ('!', buf[all_strings_length]);
+  EXPECT_EQ('!', buf[all_strings_length + 1]);
+  EXPECT_EQ('!', buf[all_strings_length + 2]);
+
+  EXPECT_FALSE(stream.HasBytesRemaining());
+}
+
+TEST(CompositeHTTPBodyStreamTest, StringsAndFile) {
+  std::string string1("Hello! ");
+  std::string string2(" Goodbye :)");
+
+  std::vector<HTTPBodyStream*> parts;
+  parts.push_back(new StringHTTPBodyStream(string1));
+  parts.push_back(new FileHTTPBodyStream(
+      base::FilePath("util/net/testdata/ascii_http_body.txt")));
+  parts.push_back(new StringHTTPBodyStream(string2));
+
+  CompositeHTTPBodyStream stream(parts);
+  EXPECT_TRUE(stream.HasBytesRemaining());
+
+  std::string expected_string = string1 + "This is a test.\n" + string2;
+  std::string actual_string = ReadStreamToString(&stream);

[Mark Mentovai, 2014/10/23 19:31:04]

ReadStreamToString will get the whole thing in three reads, one for each of the
three composite parts. Nothing actually tests that the composite part isn’t
advancing to its next subpart until it’s exhausted the subpart’s data. For all
this test knows, the composite part could just be advancing to the next subpart
on each read.

This is a very important attribute of the composite part implementation to test.

[Robert Sesek, 2014/10/24 16:53:34]

Done. Now have a parameterized test for various buffer sizes.

+  EXPECT_EQ(expected_string, actual_string);
+
+  EXPECT_FALSE(stream.HasBytesRemaining());
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad