CodingAleCR / http_interceptor

A lightweight, simple plugin that allows you to intercept request and response objects and modify them if desired.
MIT License
134 stars 67 forks source link

Bug: HTTP Methods have misaligned parameters #5

Closed jensencelestial closed 4 years ago

jensencelestial commented 4 years ago

Hi, I have noticed that for Http methods that doesn't need query parameters (POST, PUT, PATCH, etc.), the body parameter is being assigned to params in _sendUnstreamed function instead. Refer to the example below:

Future<Response> post(url,
          {Map<String, String> headers, body, Encoding encoding}) =>
      _sendUnstreamed("POST", url, headers, body, encoding);

For the post function above, the body parameter is being passed to params, causing type mismatch exceptions when being used.

This is the _sendUnstreamed function for reference:

Future<Response> _sendUnstreamed(String method, url,
      Map<String, String> headers, Map<String, String> params,
      [body, Encoding encoding]) async {
    if (url is String) {
      var paramUrl = url;
      if (params != null && params.length > 0) {
        paramUrl += "?";
        params.forEach((key, value) {
          paramUrl += "$key=$value&";
        });
        paramUrl = paramUrl.substring(0, paramUrl.length); // to remove the last '&' character
      }
      url = Uri.parse(paramUrl);
    }

    var request = new Request(method, url);

    if (headers != null) request.headers.addAll(headers);
    if (encoding != null) request.encoding = encoding;
    if (body != null) {
      if (body is String) {
        request.body = body;
      } else if (body is List) {
        request.bodyBytes = body.cast<int>();
      } else if (body is Map) {
        request.bodyFields = body.cast<String, String>();
      } else {
        throw new ArgumentError('Invalid request body "$body".');
      }
    }

    //Perform request interception
    for (InterceptorContract interceptor in interceptors) {
      RequestData interceptedData = await interceptor.interceptRequest(
        data: RequestData.fromHttpRequest(request),
      );
      request = interceptedData.toHttpRequest();
    }

    var stream = requestTimeout == null
        ? await send(request)
        : await send(request).timeout(requestTimeout);

    var response = await Response.fromStream(stream);

    var responseData = ResponseData.fromHttpResponse(response);
    for (InterceptorContract interceptor in interceptors) {
      responseData = await interceptor.interceptResponse(data: responseData);
    }

    return responseData.toHttpResponse();
  }
CodingAleCR commented 4 years ago

Nice catch! To be honest I completely missed this, you are right, it shouldn't be like that. This is easily fixed by either changing _sendUnstreamed to have named parameters or by creating a separate _sendUnstreamed for the get HTTP Method. Feel free to drop a PR if you can, otherwise I'll fix it as soon as I can.

Thanks!

jensencelestial commented 4 years ago

Thank you for the quick reply. I'll try to make a PR, will it be alright to just insert a null parameter to these methods, since that is what you did with the head and delete methods.

Future<Response> post(url,
          {Map<String, String> headers, body, Encoding encoding}) =>
      _sendUnstreamed("POST", url, headers, null, body, encoding);
CodingAleCR commented 4 years ago

To be honest I would like it to be like the _sendInterception method in http_with_interceptor.dart. I think I'll have a couple of hours available later during the day to fix it and release a hotfix. But if it works for you that's fine for now.

jensencelestial commented 4 years ago

Upon taking a look at _sendInterception, it really looks like the better approach. I'll wait for the hotfix release, but I'll try to use the null approach in my fork in the meantime. Thank you very much! 😄

CodingAleCR commented 4 years ago

Cool, and sure, anytime! 👍

CodingAleCR commented 4 years ago

Done! The new version 0.1.1 should fix the issue.