Bubble up errors while loading Resources

[ettore]

Errors are bubbled up to the app via a new NavigatorDelegate method:

navigator(_:didFailToLoadResourceAt:url:withError)

No breaking changes to the Navigators client-facing APIs.

This solves issue #398.

The 2nd commit depends on PR readium/GCDWebServer#11. It is not strictly required but it is nice to have, because it attaches the underlying error to the GCDWebServerErrorResponse.

[ettore]

ugh the build fails because of that GCDWebServer PR. Let me know what you think of it, and if needed I can just nuke the 2nd commit in this PR.

[mickael-menu]

Thanks @ettore!

The changes to HTTPServer are breaking the semantic versioning. We could either switch the base branch to v3, or maintain the old HTTPServer APIs with fallbacks. What do you prefer?

I plan to release at least one other minor version from develop before v3 is released.

[mickael-menu]

Now that we have several handlers with multiple parameters, I'm wondering if adding a HTTPServerEndpointDelegate protocol would be cleaner, with serve(at: endpoint, using delegate: HTTPServerEndpointDelegate). What do you think?

public protocol HTTPServerEndpointDelegate {
    func endpoint(_ endpoint: HTTPServerEndpoint, handle request: HTTPServerRequest) throws -> Resource

    func endpoint(_ endpoint: HTTPServerEndpoint, requestAt url: URL, href: String, didFailWithError error: Error)
}

Also I'm not sure url and href should be optional here?

[ettore]

the delegate is more scalable but on the other hand I kind of like how the server is handling things more directly. Also there are already a lot of moving parts in all these PRs, so for now I'd prefer to keep it as is for now. But it could be an iterative change before we get to v3?

Regarding url and href being optional, it's because there are 2 cases where one of them can be nil:

1. handling case where server doesn't start, where server.serverURL may be nil.
2. handling case where request.href may be nil

[mickael-menu]

Also there are already a lot of moving parts in all these PRs, so for now I'd prefer to keep it as is for now.

I'm fine with keeping things as they are if you prefer.

1. handling case where server doesn't start, where server.serverURL may be nil.

I think the FailureHandler should only notify errors for a particular request (the fact that you had to use ResourceError.other(GCDHTTPServerError.serverNotStarted) is a good indication). So for this case, IMHO the error thrown by serve() is enough.

2. handling case where request.href may be nil

That's true. Actually that makes me think the FailureHandler could just be (_ request: HTTPServerRequest, _ error: ResourceError). The request has the url and optional href, and if we ever add more information (e.g. headers), that could be interesting as well.

[ettore]

1. 👍 I'll remove the use of ResourceError.other(GCDHTTPServerError.serverNotStarted).
2. I started doing the 2nd change but in GCDHTTPServer::handle(request:completion:) I do not have a reference to the HTTPServerRequest. I only have GCDWebServerRequest and I don't think I can generate the former from the latter? Or maybe you meant to just create a new HTTPServerRequest from the Resource like so:

let httpServerRequest = HTTPServerRequest(url: request.url, href: resource.link.href) //??
failureHandler?(httpServerRequest, error)

but does this correspond to the actual request that was performed? 🤔

Otherwise I could pass the HTTPServerRequest from the responseResource(for:completion:) method but it gets gnarly:

for (endpoint, handler) in handlers {
    if endpoint == pathWithoutAnchor {
        let request = HTTPServerRequest(url: request.url, href: nil)
        let resource = handler.resourceHandler(request)
        completion(request,
                   transform(resource: resource, at: endpoint),
                   handler.failureHandler)
        return
    } else if path.hasPrefix(endpoint.addingSuffix("/")) {
        let request = HTTPServerRequest(
            url: request.url,
            href: path.removingPrefix(endpoint.removingSuffix("/"))
        )
        let resource = handler.resourceHandler(request)
        completion(request,
                   transform(resource: resource, at: endpoint),
                   handler.failureHandler)
        return
    }
}

// plus this seems pretty wrong since this is not an actual request that was attempted
completion(HTTPServerRequest(url: request.url, href: request.path), FailureResource(link: Link(href: request.url.absoluteString), error: .notFound(nil)), nil)

[mickael-menu]

I think passing the request computed in responseResource() to its completion block is best.

// plus this seems pretty wrong since this is not an actual request that was attempted
completion(HTTPServerRequest(url: request.url, href: request.path), FailureResource(link: Link(href: >request.url.absoluteString), error: .notFound(nil)), nil)

It is a real HTTP request, but without an href as we couldn't identify any supporting endpoint. So it would be just HTTPServerRequest(url: request.url, href: nil).

[ettore]

The changes to HTTPServer are breaking the semantic versioning. We could either switch the base branch to v3, or maintain the old HTTPServer APIs with fallbacks. What do you prefer?

🤔 I have not looked into v3 so I don't know what changes it entails on our end. If migrating to v3 is not going to be a big effort, maybe that's doable.

For the other option, do you mean having HTTPServer include both versions of the api, meaning:

// new
func serve(at endpoint: HTTPServerEndpoint, 
           handler: @escaping (HTTPServerRequest) -> Resource,
           failureHandler: FailureHandler?) throws -> URL
// old
func serve(at endpoint: HTTPServerEndpoint, handler: @escaping (HTTPServerRequest) -> Resource) throws -> URL

where basically the old one simply calls the new one with a nil failureHandler (perhaps in a protocol extension)?

I plan to release at least one other minor version from develop before v3 is released.

👍 In this case the 2nd option with fallbacks might be better. It would be preferable for us to be able to use this new stuff sooner rather than later. (And I'd prefer to avoid targeting develop for our production builds 😅 )

[mickael-menu]

That's right, keeping the old APIs and just forwarding the calls to the new ones could be enough here. 👌

[ettore]

I was able to build the TestApp and fix all the regressions 😅 I also fixed a couple easy warnings.

I ended up adding xcpretty since it was very simple to do so and the benefits are big. There's a night and day difference in how the logs are easier to read (before | after). It did help in diagnosing the problem from the logs, which was not possible before. There's some script-code duplication in the workflow, but hopefully that can be done at some other time.

[mickael-menu]

I wonder if the build logs could be improved, perhaps with xcpretty? we've been using it and it makes the build logs much clearer because it highlights the errors and it is more succinct for the commands that succeed. IIRC it is provided by GitHub out of the box, so I could try adding it to the xcodebuild commands.

That's great, thank you for the tip!

I made a couple of changes in the PR:

• I restored HTTPServer.serve(at:handler:) as the default API to implement for the protocol, to avoid breaking implementers updating Readium. In v3, I will remove it.
• I changed the NavigatorDelegate API to remove the URL (a Navigator doesn't necessarily use an HTTP server) but require a non optional href, as it identifies a particular resource. This is to be consistent with the same Kotlin API.

Let me know if you are fine with these changes, and I will merge the PR. Thanks again!

[ettore]

• I changed the NavigatorDelegate API to /.../ require a non optional href, as it identifies a particular resource. This is to be consistent with the same Kotlin API.

I tested my code with this change and the errors I was observing are still being reported, so this should be ok, I think. But I do worry though that the failureHandler calls where the HTTPServerRequest::href is nil (here, here and here) are now not going to reach the navigator side since we filter for a non-optional href.

[mickael-menu]

But I do worry though that the failureHandler calls where the HTTPServerRequest::href is nil (here, here and here) are now not going to reach the navigator side since we filter for a non-optional href.

These are outside the scope of broken publication resources, so they should not be reported with NavigatorDelegate.didFailToLoadResourceAt. If they occur, there's probably something wrong in the app itself which should be fixed by the integrator.

Although I would be open to add a global fallback failure handler directly in the GCDHTTPServer type, that applications could provide when creating the server.

[ettore]

These are outside the scope of broken publication resources, so they should not be reported with NavigatorDelegate.didFailToLoadResourceAt. If they occur, there's probably something wrong in the app itself which should be fixed by the integrator.

Although I would be open to add a global fallback failure handler directly in the GCDHTTPServer type, that applications could provide when creating the server.

I was even just thinking something like another "catch all" delegate method to do something like this

guard let href = href else {
    self.delegate?.epubNavigatorViewModel(self, didFailWithError: error)
    return
}
self.delegate?.epubNavigatorViewModel(self, didFailToLoadResourceAt: href, withError: error)

I did not propose it because it's still unclear to me when the case of a nil href actually happens. In any case I agree this is beyond the scope of Resource loading fails. We'd also be risking delegate callback pollution. 😅

Thanks for merging the PR! 🎉