Add URL to RequestError

[jrmsklar]

This pull request adds a URL object to the RequestError struct that represents the URL for the request that the error represents.

Motivation

We needed a way to access the URL that the RequestError related to so that when logging RequestErrors, we could include the URL for the request that was causing the error.

[Uncommon]

Perhaps this should be private(set)

[jrmsklar]

What are your thoughts on me changing it to a let?

[Uncommon]

Yeah, if it's only set in init then I'd say that's appropriate.

[victorcotap]

go for let

[jrmsklar]

👍 will do. Thanks!

[jrmsklar]

@victorcotap all set!

[pcantrell]

Hi @jrmsklar, and thanks for suggesting this! I have several concerns about attaching the full HTTPURLResponse to RequestError. Here they are, in order of decreasing importance:

1. Duplicated, possibly inconsistent information

   RequestError already has an entity property that exposes most of the same data (though not the URL). The app’s transformer pipeline can and should transform errors, including the metadata in the entity. Siesta’s Entity itself normalizes headers to make them case-insensitive.

   This means that with the addition of the raw HTTPURLResponse, RequestError would contain two separate and possibly conflicting versions of the response headers, mime type, etc. I can imagine somebody getting very confused about whether to use error.entity.header(forKey: foo) or error.httpResponse.allHeaderFields[foo] — and even more confused about the fact that they can contain different data and exhibit different behavior even with the same data.

   All of this smells of leaky abstraction.

   A partial solution would be to make everything exposed by RequestError and Entity be a computed property that delegates to an underlying RequestError — just as you’ve done with the status code. However, this would make response transformers much more difficult to write, and exacerbate the problems below….

2. Mutability

   RequestError is a struct (as is Entity). This is crucial to Siesta’s API design:

   • Both the transformer pipeline and EntityCache need to be able to pass them safely across threads.
   • Resource exposes them as read-only properties, and thus since they’re structs they can’t be mutated in place. This helps Siesta guarantee that observers are notified about any changes to resource state.

   HTTPURLResponse is an open class with mutable fields. It thus breaks all of the above.

   (Aside: these concerns are why Siesta urges you to use only immutable or struct data in for Entity content.)

3. Memory weight

   This is minor compared to the above, but I’m slightly concerned about HTTPURLResponse’s memory weight, mostly that [AnyHashable : Any]. (At least it doesn’t hold the full response body … I think?)

   The memory weight is fine when it’s an ephemeral object that disappears after a single response, but Siesta can potentially hold large numbers of errors over a long period of time. That’s not what HTTPURLResponse was designed for. I’d want to double check memory usage before using it this way.

I did originally consider designing the API this way. These concerns are why I decided against it.

Sorry to pour cold water on this idea! It seems to me that RequestError holding only the URL would have none of these problems.

I’m curious: how would you use the URL? Your need for it suggests there may be some underlying unmet need in Siesta’s API.

[jrmsklar]

Thanks a ton for the detailed response here @pcantrell. All of your concerns make complete sense.

I'd like the to have access to the URL in order to log it when handling API response errors. We have remote logging set up (via PaperTrail), and have not been able to send up the URL when logging response errors, and it would be very helpful for us in order to see where these errors are originating from.

I'm happy to revert these changes and instead add a URL property to RequestError if you see fit.

[pcantrell]

I'd like the to have access to the URL in order to log it when handling API response errors.

That does make perfect sense.

Adding a url property sounds fine to me, and I see little harm in it. And like the other RequestError properties, I see no harm in making it var: because it’s a struct, you can’t still mangle a Resource’s error in place, and there’s no interrelationship with other properties that the struct needs to manage.

It’s not obvious at first, but url must be optional. Some apps use RequestError to signal that it was not possible to form a valid URL / resolve something to a valid resource, so there may not always be a URL to associate with the error. (This is the same reason that Resource.failedRequest(…) is static. See #50.)

[jrmsklar]

@pcantrell I've reverted the earlier commits, and added a url property to RequestError. Thanks again for the helpful feedback/suggestions!

[jeffaburt]

@pcantrell when you get a free second, do you mind reviewing the changes here?

[jrmsklar]

Hey @pcantrell,

Any update here?

Thanks!

[pcantrell]

@jrmsklar and @jeffaburt: Hello, and sorry for the very slow turnaround on this! I see you've been maintaining your own fork; I hope my delay hasn't caused your project distress.

This innocent-looking property opened a little a can of worms for me. It occurred to me that it’s not sufficient to set url only when we receive an actual HTTP response; the property should be set for any RequestError associated with a resource. (For example, pre-request errors such as unencodable strings or malformed JSON objects do not have an associated server response but did come from an identifiable URL.)

It then occurred to me that the property should be set not only when request hooks and resource observers receive the response, but throughout the response pipeline. Why should RequestError have a different shape during pipeline processing than after? And it shouldn’t be the responsibility of a transformer to set it, so shouldn’t the pipeline itself set the URL property after every transformer, just in case the next transformer needs it?

At this point, the reasoning seemed to be getting out of hand, and the implementation got mildly messy:

https://github.com/bustoutsolutions/siesta/compare/error-url-property

That's not terrible, but it smells funny to me. I’m passing the resource URL around all over the pipeline code, only to have transformers ignore it 99% of the time. And if we follow this reasoning, shouldn’t successes also have a URL property? That means it’s a property of Entity. But that undermines the whole idea that Entity represents the state of a resource decoupled from how it arrived. Separations of concerns are crumbling left and right — all in the name of logging URLs.

This is where I decided to put the brakes on.

---

The way Siesta is designed, it would make more sense for transformers, observers, and hooks alike to all get the URL either by capturing it from context or by receiving it as an input than for it to be passed in the response.

It would be helpful to know a little more about the problem you’re trying to solve. So, questions for you:

You said this was for logging errors? Are you logging them via an onFailure hook? A resource observer? What does that code look like?

[jrmsklar]

Thanks for the thoughtful response, @pcantrell! I appreciate you taking the time to look into this issue and solve in a clean and elegant way. I understand your concerns and am interested in moving this forward and coming up with a solution.

To answer your questions:

You said this was for logging errors?

Yep - this is for logging the URL related to a RequestError value.

Are you logging them via an onFailure hook? A resource observer? What does that code look like?

We have a custom API response content transformer that conforms to ResponseTransformer. In our implementation of the process(_:) function, we have a handler for the failure case of the response that pulls our the URL from the associated error, and logs it.

The code looks as follows:

struct APIResponseContentTransformer: ResponseTransformer {
    func process(_ response: Response) -> Response {
        switch response {
        case .success(let entity):
            return processEntity(entity)

        case .failure(let error):
            return processError(error)
        }
    }

    fileprivate func processError(_ error: RequestError) -> Response {
        let urlString = error.url?.absoluteString ?? "unknown url"
        // Log it
        return .failure(error)
    }

Let me know if you need more information!

[pcantrell]

Hmm, that is a compelling and clarifying example. Thanks for taking the time to share it!

Although you’re logging only errors, I can imagine scenarios where one would also want the URL for successes (e.g. logging all requests made). In this context, it would make less sense for url to be a property of Entity or RequestError than for it to be a parameter to process(…).

I’m not thrilled with making the url a centerpiece of the pipeline API, however. That runs against best practices, and I have a gut feeling (which I haven't fully worked out) that it fundamentally breaks the pipeline’s separation of concerns.

On reflection, this is probably yet another compelling reason for implementing what @Reedyuk suggested in #154. The basic idea is that you could configure observers that receive events only as long as a resource is still in memory, while not causing that resource to be retained indefinitely. That hypothetical non-retaining observer API would certainly send observers the resource in question, which I think would solve your problem.

[jrmsklar]

Apologies for the delay here, and thank you for the thoughtful response!

I also see value in having the URL for not only errors, but successes too - it would be very nice to log all requests made! If @Reedyuk's suggested will allow for the URL to be accessible in errors and successes, then I am all for that 👍.

[gaming-hacker]

Is this going to merged?