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 a new class to handle Multipart requests #248

Merged
merged 8 commits into from
Apr 16, 2020
Merged

Conversation

lbalmaceda
Copy link
Contributor

@lbalmaceda lbalmaceda commented Apr 14, 2020

Changes

Implements a new class to handle multipart requests. This is required before implementing the "import users" Job endpoint.

These multipart requests are only allowed for HTTP methods other than GET. Some of the shared logic between CustomRequest and the introduced MultipartRequest has been moved up to a class named ExtendedBaseRequest (as it's adding things on top of BaseRequest). This shared logic class is package-private!

To review:

  • Name of the introduced request class: MultipartRequest.
  • The existence (or not) of the FormDataRequest interface.
  • The exceptions that should be declared as thrown on the abstract methods for creating the request body and parsing the response body.

Testing

Check the commit history!! First, I added the new request class. Then I added tests for that. Then I added a few helper classes for the tests. Lastly, I refactored and extracted the common parts to the shared logic class.

Please, do check for public API breaking changes. There shouldn't be any, but is always good to have another pair of eyes 👀

  • This change adds test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@lbalmaceda lbalmaceda changed the title Add Job "POST Users Import" endpoint Add a new class to handle Multipart requests Apr 14, 2020
@lbalmaceda lbalmaceda added this to the v1-Next milestone Apr 14, 2020
@lbalmaceda lbalmaceda marked this pull request as ready for review April 14, 2020 22:13
@lbalmaceda lbalmaceda requested a review from a team April 14, 2020 22:13
@jimmyjames jimmyjames requested review from jimmyjames and removed request for a team April 15, 2020 16:16
Copy link
Contributor

@jimmyjames jimmyjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding names, I think MultipartRequest makes sense. I think we might be able to do better than ExtendedBaseRequest - I know it's package private so not a huge deal, but as you add some JavaDoc to it maybe a more descriptive name will become obvious.

I also think the interface is fine, just add JavaDoc please ✍️

Regarding the exception types, let's connect later on this and discuss 👍

In general it looks good though, just a couple general points:

  • I know our existing test coverage provides confidence in not introducing any breaking changes, but it would be good to double check and verify that there are no unintended breaking changes to any of the impacted public APIs, especially any changes to method signatures/exception types.
  • JavaDocs 😄

src/main/java/com/auth0/net/ExtendedBaseRequest.java Outdated Show resolved Hide resolved
src/main/java/com/auth0/net/MultipartRequest.java Outdated Show resolved Hide resolved
src/main/java/com/auth0/net/EmptyBodyRequest.java Outdated Show resolved Hide resolved
@lbalmaceda lbalmaceda merged commit 2472686 into master Apr 16, 2020
@lbalmaceda lbalmaceda deleted the add-user-imports branch April 16, 2020 22:14
@jimmyjames jimmyjames modified the milestones: v1-Next, 1.16.0 Apr 24, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants