Add HTTPRequestBuilder

[crazytonyli]

Description

This new type will be used to construct URLRequests for all HTTP API requests.

Testing Details

See the unit tests. Let me know if you think there are more should be added.

---

• Please check here if your pull request includes additional test coverage.
• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[mokagio]

Suggested change

	}
	private var urlComponents: URLComponents
	}
	private var urlComponents: URLComponents

🤓

[mokagio]

How do you feel about creating a type to encapsulate this?

extension URL {

  struct Path {

      let value: String

      /// `Path` cannot not contain URL query fragments (? and #)
      init(from string: String) {
	assert(!string.contains("?") && !string.contains("#"), "Path should not have query or fragment: \(path)")
        value = string
      }

The end result is the same: The app will crash if the path is invalid, but I feel explicitly wrapping it in a dedicated type will make what to do more obvious to consumers.

[crazytonyli]

I see value in this dedicated type, if we have more places that need to refer to a "URL path". But for this one method in HTTPRequestBuilder, I don't feel like it's worth the effort?

[mokagio]

Yes, fair enough.

[mokagio]

All the methods before this use "set" as their verb.

I think if it would be good to use it consistently, to have a homogeneous usage at call site. E.g. from

var request = try HTTPRequestBuilder(url: URL(string: "https://wordpress.org/")!)
            .set(method: .post)
            .body(json: 42)

to

var request = try HTTPRequestBuilder(url: URL(string: "https://wordpress.org/")!)
            .set(method: .post)
            .set(jsonBody: 42)

or

var request = try HTTPRequestBuilder(url: URL(string: "https://wordpress.org/")!)
            .setMethod(.post)
            .setJSON(body: 42)

[crazytonyli]

Good catch! I took the other direction: removing the "set" part, because it's redundant considering this is a "builder" type. See b52e98d.

[mokagio]

Fine by me 😄

[mokagio]

Should this be

Suggested change

	var url = URLComponents(string: "https://wordpress.com")!
	var url = self.urlComponents

I'm surprised https://wordpress.com/ is hardcoded here but maybe I'm missing something about the intended use? E.g., only requests to the WordPress.com API can send URL form encoded requests?

[crazytonyli]

This function needs a URL encoded string. I'm just using a temporary URLComponents to construct a query which is url encoded.

[mokagio]

Much of a muchness, but what do you think of moving these specific methods into an extension? All I've seen so far in HTTPRequestBuilder is standard HTTP. These are the only methods that implement an additional spec.

[mokagio]

This behavior, which I read as "when the given path is absolute, override the original URL path" is surprising.

Or maybe is just me applying the expectation from NSString appendingPathComponent:

# from the REPL
  2> let s: NSString = "https://foo.test/bar"
s: NSMutableString = "https://foo.test/bar"
  3> s.appendingPathComponent("/baz")
$R0: String = "https:/foo.test/bar/baz"
  4> s.appendingPathComponent("bang")
$R1: String = "https:/foo.test/bar/bang"
  5>

I acknowledge that NSString explicitly says "appending", though, whereas this API gives no similar clear guardrail on what path would do.

[crazytonyli]

After a second look, I do agree with you that this behaviour is not very intuitive.

I have updated the API to only allow appending path. See 4bb8360

[mokagio]

This test made me want to suggest to triangulate setting the URL, that is add at least two different examples (cc @wordpress-mobile/mobile-ui-testing-squad)

XCTAssertEqual(
  try HTTPRequestBuilder(url: URL(string: "https://wordpress.org/")!).build().url?.absoluteString,
  "https://wordpress.org/"
)
XCTAssertEqual(
  try HTTPRequestBuilder(url: URL(string: "https://wordpress.com/")!).build().url?.absoluteString,
  "https://wordpress.com/"
)

The value of this approach is that it gives additional confidence in the robustness of the tests. But that's mostly redundant when we have more tests below that implicitly exercise this behavior.

However, something I noticed and that we could use to add a variety of initial URL inputs is that there are no tests for how set(method:) behaves. Granted, that's pretty straightforward, but given how core this object will be, I think "overtesting" it wouldn't be a waste. The same goes for the HTTPRequestBuilder.Method allowsHTTPBody logic.

[mokagio]

Thank you for taking my suggestions into consideration.

[mokagio]

Nice touch grouping the tests this way.

I don't subscribe to the "one assertion per test" rule that some tests enthusiast argue for. This kind of functional/semantic grouping strikes a good balance between not repeating the func test... cruft and having tests that point to the same behavior.