Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some basic support for networking interceptors #118

Open
brendanlensink opened this issue Jun 5, 2023 · 4 comments
Open

Add some basic support for networking interceptors #118

brendanlensink opened this issue Jun 5, 2023 · 4 comments

Comments

@brendanlensink
Copy link
Collaborator

brendanlensink commented Jun 5, 2023

Adding interceptors to Netable will be necessary for working with our mocking libraries.

I think we can be pretty flexible here and offload most of the work to the actual interceptors, but we need to provide something at the base level to allow interceptors to interface with Netable.

I think a good starting place might be:

  • Introduce new top level types called Interceptor and RequestInterceptor that look something like

     struct Interceptor {
       let interceptors: [RequestInterceptor]
    }
    
     protocol RequestInterceptor {
       func adapt(_ request: URLRequest, instance: Netable) async throws -> URLRequest {}
    }
    
  • Add optional parameters to both Netable.init and Netable.request that accept an Interceptor

  • Modify request to apply any interceptors to our URLRequest before it gets sent off

@brendanlensink
Copy link
Collaborator Author

@nbrooke does this seem reasonable to you? Any gotchas or other features that might be nice to have?

@nbrooke
Copy link
Member

nbrooke commented Jun 6, 2023

That looks reasonable to me. One question and two things to consider (one lower level, one more conceptual):

Question: I assume in that sample signature above that "Request" return value should actually be a URLRequest, right? Like

func adapt(_ request: URLRequest, instance: Netable) async throws -> URLRequest {}

It seems like that being a Netable Request would not make sense.

Comment One: It might make sense for the adapt function to indicate if it has rewritten the request in some way, even as simple as the return value being optional and only returning a new URLRequest if it did a rewrite. Having multiple RequestInterceptors seems to necessary to be able to abstract those nicely, but I think Ideally they should be non-overlapping (i.e. no actual request would be rewritten by two interceptors). Without some indication of if the URL was rewritten in the API, there's no way to even detect that t runtime and raise a warning / error.

Comment Two: Focusing on URL rewriting seems correct given that WE are planning to move towards server side mocking, but I do wonder if we should maybe still support the style of "return some JSON file included in the app bundle" mocking that we do now via the same interface. Maybe that could be made to work with this if you can like rewrite those URLRequests to use file:// URLs or something, but if not, it might want to return some sort of three state enum (url request or file URL or no intercept) so that you can do that sort of mocking.

@brendanlensink
Copy link
Collaborator Author

I assume in that sample signature above that "Request" return value should actually be a URLRequest, right

Oops, fixed

I think Ideally they should be non-overlapping (i.e. no actual request would be rewritten by two interceptors)

Is this true? I'm definitely not convinced either way, but the Alamofire that this is based on, seem to indicate that you can rewrite requests multiple times.

I could certainly imagine some cases where you might want to apply two interceptors to the same request, like if you've got one Netable level interceptor and a second request level one? Or maybe you've got a collection of interceptors that each filter on a different value and you want to be able to apply different mutations to different requests, if that makes sense?

Also, are there scenarios where someone might attach an interceptor that doesn't change the request but generates some other kind of side effects? In this case, I think it might be a little weird to return an optional, where in my mind if it were to return null I might assume nothing's changed? I'm not sure if that makes sense or not though.

it might want to return some sort of three state enum

This is compelling to me... maybe something like

enum AdaptedRequest {
 case adapted(let URLRequest)
 case mocked(let String)
 case noChange
}

??

@nbrooke
Copy link
Member

nbrooke commented Jun 6, 2023

Is this true? I'm definitely not convinced either way, but the Alamofire that this is based on, seem to indicate that you can rewrite requests multiple times.

Maybe it comes down to how narrowly the feature is targeted at mocking. I think it IS true that you wouldn't want two interceptors to be doing a full rewrite of the URL of the sort that mocking would generally do (and if we do have the way to support mocked data being returned directly, it seems pretty obvious that we can't support two different interceptors doing that). Doing rewriting of the request for other purposes, that could compose fine, like the "add in headers" example they use in the Alamofire docs, definitely make conceptual sense. I'm not sure if being that general purpose is necessary for us (Netable a lot more opinionated than Alamofire), but if we ARE seeing this as a way to implement OTHER transformations on the request then just mocking, than problaby it is necessary to support multiple adaptations on the same request.

if it were to return null I might assume nothing's changed?

I think in the case where there is a side effect, but no change to the request, that is probably fine. If we did think that info was useful, and were returning an enum, we could have both a noChange and a notHandled case to indicate the difference between "this is a request I do care about, but don't need to change" and "this is a request I don't care about", but I'm not sure if there is anything we could actually DO with that info.

case mocked(let String)

I could see this case containing any of:

  • A file URL
  • A String of the file path
  • A String of the JSON text
  • A Data containing the JSON

Being reasonable, and am not sure which one to prefer, but other than that, yeah, that sort of thing was basically what I was imagining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants