CodingAleCR / http_interceptor

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

Fix send when Request isn't a StreamedRequest #110

Closed FabianTerhorst closed 1 year ago

FabianTerhorst commented 2 years ago

Send doesn't need to be used with a StreamedRequest. When it doesn't, the interceptor converts the StreamedResponse to a Response.

CodingAleCR commented 2 years ago

Hey, thank you for the PR.🎉

I will take a look as soon as I can so that I can review the issue and also the solution proposed.

giiyms commented 2 years ago

Hello, not sure why but using @FabianTerhorst main git repo, I am able to send a request and it works perfectly, but when using @CodingAleCR main branch or 2.0.0-beta.5, it does not work. My send request hangs indefinitely.

CodingAleCR commented 2 years ago

Interesting. Probably pointing to the problem about the streamed request that the PR fixes.

What type of request are you making @giiyms ?

giiyms commented 2 years ago

Interesting. Probably pointing to the problem about the streamed request that the PR fixes.

What type of request are you making @giiyms ?

I am sending an http GET request for an event stream.

  final streamUrl = Uri.https(_sseUrl, '$_channel', {'symbols': symbol});
  final request = http.Request('GET', streamUrl);

  final headers = {'Content-Type': 'text/event-stream'};
  request.headers.addAll(headers);

  final response = await _tokenizedClient.send(request);

The tokenized Client is the standard InterceptorContract which talks to a tokenservice for a parameter on the intercept request.

  @override
  Future<BaseRequest> interceptRequest({required BaseRequest request}) async {
    return request.copyWith(url: request.url.addParameters({'token': _tokenService.token}));
  }

With Fabian's repo, I am able to await stream.first in my test file.

giiyms commented 2 years ago

Oh my bad @CodingAleCR. I thought the commit you did last was applied to your repo and therefore 2.0.0-beta.5, but I see now its on Fabian's repo. Sorry for mix up. I guess I can confirm Fabian's changes fixed my issue!

CodingAleCR commented 2 years ago

Nice! Thank you for the confirmation. I am trying to check that these changes don't affect the MultiPartRequest. Could you confirm what type of request you are using with the send method?

giiyms commented 2 years ago

Nice! Thank you for the confirmation. I am trying to check that these changes don't affect the MultiPartRequest. Could you confirm what type of request you are using with the send method?

Hello, it was not a MultiPartRequest, so I can not confirm it does not affect that.

CodingAleCR commented 1 year ago

Currently adding a new MultipartApp to the example app, just so we can test properly. In the meantime I'd like to know are you using server sent events by any chance? I want to add as many tests to this as possible.

CodingAleCR commented 1 year ago

I was looking at the implementation of this PR and this might break streamed requests, if we added tests or thought of a different solution then I would definitely merge this to the main repo.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.