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

Include a proper SignUp response #92

Merged
merged 3 commits into from
Nov 30, 2017
Merged

Include a proper SignUp response #92

merged 3 commits into from
Nov 30, 2017

Conversation

lbalmaceda
Copy link
Contributor

@lbalmaceda lbalmaceda commented Nov 28, 2017

To the reviewer:

  • Name of the SignUpRequest interface's implementation class. I used CreateUserRequest. User's will receive an instance with the interface name instead (as that's what the method signature defines), but because all the request classes are public scoped I'd like to leave this well. To be honest I could just name it "SignUpRequestImpl".
  • Type of signup response. I'm using the one from the /userinfo endpoint, which it's just a generic Map<String, Object> holder with a getter for "all values". The alternative is to create a new class with the 4 parameters returned by the signup endpoint.
  • Evaluate the impact of this PR as a breaking change. Because the method was returning a wrong object I would consider this as a fix and break it anyway. But the fix included even more changes on other classes than I thought at first.

Should close #86

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Public API is changing, can a developer be in a position the code no longer compiles?

@@ -21,10 +21,4 @@ protected Void parseResponse(Response response) throws Auth0Exception {
}
return null;
}

@Override
public VoidRequest setCustomFields(Map<String, String> customFields) {
Copy link
Member

Choose a reason for hiding this comment

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

So this is a BC as Public API removal, this is a compile fail for anyone using this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"CustomFields" were supposed to be available only for SignUp methods. Because of how the request classes were inherited, this method ended wrongly in the VoidRequest class. No, it wouldn't be a breaking change as there's not a single API method signature that declares a return type of VoidRequest. They all either return an AuthRequest (never a TokenRequest), SignUpRequest (never a CreateUserRequest) or a simple Request or Request<T>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If people were casting that returned type to VoidRequest (in the case that that method call was returning a VoidRequest class or subclass) and using that method, then yes it would be a breaking change. But that's why we return interfaces rather than classes, to obscure our internals. So I wouldn't consider this a breaking change anyway.

import java.util.Map;

/**
* Class that represents a Create User call.
*/
public interface SignUpRequest extends Request<Void> {
@SuppressWarnings("UnusedReturnValue")
public interface SignUpRequest extends Request<UserInfo> {
Copy link
Member

Choose a reason for hiding this comment

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

So previously the sign up did work. However it would simply return a void response on success.
So a developer may be checking for null to detect success? Which would now be a BC as the response is not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Void class is used in java generics as you can't return "void" nor a primitive value, they must actually return an object. When a method says it returns "Void" it must return the "null value" as seen here.

If they wanted to check success they would just try/catch the request.execute() call. But holding the Void instance is nonsense as they can't do anything with that. So probably, everyone calling this method before these changes would have something like this:

AuthAPI authAPI = new AuthAPI("domain.auth0.com", "clientId", "myClientSecret");
SignUpRequest signUpRequest = authAPI.signUp("[email protected]", "pwd123", "connect1on");
try {
    signUpRequest.execute();
} catch (Auth0Exception e) {
    //Check errors
    e.printStackTrace();
}

and after the change they CAN add the return type (the snippet above will continue working just fine as well👌 ).

AuthAPI authAPI = new AuthAPI("domain.auth0.com", "clientId", "myClientSecret");
SignUpRequest signUpRequest = authAPI.signUp("[email protected]", "pwd123", "connect1on");
try {
    UserInfo info = signUpRequest.execute();
    //use "info"
} catch (Auth0Exception e) {
    //Check errors
    e.printStackTrace();
}

Yes, previous method did work but the response wasn't the right one. Is there an alternative? The only breaking change I can consider is if they were using the nonsense version of the snippet:

AuthAPI authAPI = new AuthAPI("domain.auth0.com", "clientId", "myClientSecret");
SignUpRequest signUpRequest = authAPI.signUp("[email protected]", "pwd123", "connect1on");
try {
    Void result = signUpRequest.execute(); // --> Compile error as Void is now UserInfo
} catch (Auth0Exception e) {
    //Check errors
    e.printStackTrace();
}

They'd need to fix the type of the returned object.


import java.util.Map;

public class CreateUserRequest extends CustomRequest<UserInfo> implements SignUpRequest {
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be stand alone and the old Signup is simply deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow. The idea is that every API method returns an interface that just exposes the methods that the endpoint allows. A SignUpRequest would expose a custom fields setter method, for example.

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Following internal conversation

@lbalmaceda lbalmaceda added this to the v1-Next milestone Nov 30, 2017
@lbalmaceda lbalmaceda merged commit cf7aa3d into master Nov 30, 2017
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.4.0 Nov 30, 2017
@lbalmaceda lbalmaceda deleted the fix-signup-response branch November 30, 2017 20:43
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.

SignUp response is a VoidRequest
2 participants