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

Refactor Interception logic to use base classes and be compatible with all request types #98

Merged
merged 5 commits into from
Nov 14, 2021

Conversation

lukaskurz
Copy link
Contributor

Hi again 😅

This PR is closely related to #94, being an alternative approach to solve the same problem.

Most of what i did and the Inspiration for this is already in #94 and in this comment.

TLDR;

The code uses the Request and Response class in the interception logic, which then gets wrapped in a RequestData and ResponseData class. Request and Response are used for specific types of requests and only extend the base class used inside the http logic. This is results in incompatability of the other types of requests, such as:

  • StreamedRequest and StreamedResponse
  • MultipartRequest
  • Any custom type of request, that implements/extends the base classes

I just replaced the current usages with the base classes, which enables compatability with all request types. The one downside to this is that you have to check or cast the base classes to the type of request that you are intercepting, in order to manipulate it (such as changing the body of a Response).

To make this easier, I included copyWith extension methods for all requests and responses

@CodingAleCR
Copy link
Owner

CodingAleCR commented Nov 12, 2021

I reaaaally like your approach here, it does feel right. Now, that said, I have a couple of thoughts around it:

Uri's property of queryParameters is actually an unmodifiable map (An exception is thrown in the example: Unsupported operation: Cannot modify unmodifiable map). That is the main reason for RequestData and ResponseData to exist. Now I don't quite like having the custom classes, like I said, your approach feels more natural, so, my suggestion would be to add an extension to clone the complete Uri class, that way we can still use the copyWith methods to clone requests and responses.

The reason why this is not noted in the tests is because the tests available at the moment only cover the model classes and not the complete library, which is a pain when implementing code that will/might break other stuff.

Anyway, in the meantime I am thinking of other options to solve this, so feel free to take a stab at cloning Uri or leave the PR here and I'll work on it when I get a bit of time soon.

I had completely forgotten about the AddParameters extension already built-in. This is great! I'll probably do some more testing but I'm loving the new API.

Repository owner deleted a comment from allcontributors bot Nov 14, 2021
@CodingAleCR
Copy link
Owner

@all-contributors add @lukaskurz for tests, ideas and code

@allcontributors
Copy link
Contributor

@CodingAleCR

I've put up a pull request to add @lukaskurz! 🎉

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

Successfully merging this pull request may close these issues.

2 participants