Skip to content

Include a proper SignUp response#92

Merged
lbalmaceda merged 3 commits intomasterfrom
fix-signup-response
Nov 30, 2017
Merged

Include a proper SignUp response#92
lbalmaceda merged 3 commits intomasterfrom
fix-signup-response

Conversation

@lbalmaceda
Copy link
Copy Markdown
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
Copy Markdown
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?

}

@Override
public VoidRequest setCustomFields(Map<String, String> customFields) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

*/
public interface SignUpRequest extends Request<Void> {
@SuppressWarnings("UnusedReturnValue")
public interface SignUpRequest extends Request<UserInfo> {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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