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

Improve interceptor structure/base structure #544

Closed
Guldem opened this issue Jan 2, 2024 · 0 comments · Fixed by #547 or #598
Closed

Improve interceptor structure/base structure #544

Guldem opened this issue Jan 2, 2024 · 0 comments · Fixed by #547 or #598
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Guldem
Copy link
Contributor

Guldem commented Jan 2, 2024

Is your feature request related to a problem? Please describe.

Currently chopper has a lot of ways for defining a interceptors. Interceptors can also be of different types. The current setup is easy to understand and implement. But its difficult to extend internally.

Adding for example a new feature request like #334 is difficult because changing all interceptors after creation has the issues that interceptors are split into different type of interceptors (request and response). So we has to look for ways to right order of both set of requests. Or add separate function for updating request and response interceptors.

Another example is #284 timing request is currently not really possible for logging purpose. While the logging interceptor implements both interfaces (request/response interceptor) the calls to the interceptor aren't linked in any ways which make tracking of elapsed time near impossible.

Mostly I want to discuss the right way forward for chopper.

Describe the solution you'd like

Drawing inspiration from OkHttp it might be possible to change the interceptor working into a chain of interceptors. In the chain the request first go through the the list of interceptors where each interceptor has the possibility to change the request. When the request is send and the response is received the response goes backwards through the interceptor chain. And this time the interceptor have the possibility to change the response object if needed. This approach also means the internal bit of chopper need to change but could also benefits and make use of this chain. For example doing the conversion of request/response or formatting of headers could be a internal interceptor with a simple responsibility. This could also make the internal code base easier to understand.

See the RealCall as example what is done with OkHttp.

This would reduce the amount of different type of interceptor interfaces. A interceptors can change both the request and the response. This would make it easier to swap interceptors after the client is created and also add the possibility to time requests within a simple interceptor (Logging interceptor for example).

Describe alternatives you've considered

None.

Additional context

I have played around with this concept I think this is certainly possible but not without some breaking changes. I tried to make it work without breaking much of the current functionality but I doubt that is really possible. For example its now possible to have Functional interceptors in the interceptor list when creating the chopper client. I couldn't manage get that working properly with passing <BodyType,InnerType>.

Another benefit could be to possible a support for different http packages. (Not the goal) When the internal structure is abstracted into separate interceptor the final executing interceptor responsible from the actual http call could also be swapped. But this is just dreaming hahaha. And need a lot more work.

A small snippet from what I playing with.
Starting the chain (basically the sent function in base):

class Call {
 const Call({
   required this.request,
   required this.client,
 });

 final Request request;
 final ChopperClient client;

 Future<Response<BodyType>> execute<BodyType, InnerType>(
   ConvertRequest? requestConverter,
   ConvertResponse? responseConverter,
 ) async {
   final interceptors = <Interceptor>[
     ...client.interceptors,
     if (client.authenticator != null)
       AuthenticatorInterceptor(client.authenticator!),
     ConverterInterceptor(
       converter: client.converter,
       errorConverter: client.errorConverter,
       requestConverter: requestConverter ?? client.converter?.convertRequest,
       responseConverter:
           responseConverter ?? client.converter?.convertResponse,
     ),
     HttpClientInterceptor(client.httpClient),
   ];

   final interceptorChain =
       RealInterceptorChain(request: request, interceptors: interceptors);

   return await interceptorChain.proceed<BodyType, InnerType>(request);
 }
}

The interceptor chain:

class RealInterceptorChain implements Chain {
  RealInterceptorChain({
    required this.interceptors,
    required this.request,
    this.index = 0,
  });

  @override
  final Request request;
  final List<Interceptor> interceptors;
  final int index;

  @override
  FutureOr<Response<BodyType>> proceed<BodyType, InnerType>(
    Request request,
  ) async {
    assert(index < interceptors.length);

    final next = copyWith(request: request, index: index + 1);
    final interceptor = interceptors[index];
    final Response<BodyType> response = await interceptor.intercept<BodyType, InnerType>(next);

    return response;
  }

  RealInterceptorChain copyWith({
    List<Interceptor>? interceptors,
    Request? request,
    int? index,
  }) =>
      RealInterceptorChain(
        interceptors: interceptors ?? this.interceptors,
        request: request ?? this.request,
        index: index ?? this.index,
      );
}

The actual request internal interceptor:

class HttpClientInterceptor implements Interceptor {
  const HttpClientInterceptor(this.httpClient);

  final http.Client httpClient;

  @override
  Future<Response<BodyType>> intercept<BodyType, InnerType>(Chain chain) async {
    final realChain = chain as RealInterceptorChain;
    final finalRequest = await realChain.request.toBaseRequest();
    final streamRes = await httpClient.send(finalRequest);

    if (isTypeOf<BodyType, Stream<List<int>>>()) {
      return Response(streamRes, (streamRes.stream) as BodyType);
    }

    final response = await http.Response.fromStream(streamRes);
    dynamic res = Response(response, response.body);

    return res;
  }
}
@Guldem Guldem added the enhancement New feature or request label Jan 2, 2024
@techouse techouse assigned techouse and Guldem and unassigned techouse Jan 3, 2024
@Guldem Guldem mentioned this issue Jan 5, 2024
10 tasks
@techouse techouse linked a pull request Mar 2, 2024 that will close this issue
10 tasks
@techouse techouse added this to the 8.0.0 milestone Mar 13, 2024
@techouse techouse linked a pull request Apr 5, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants