Conversation
cocojoe
left a comment
There was a problem hiding this comment.
Public API is changing, can a developer be in a position the code no longer compiles?
| } | ||
|
|
||
| @Override | ||
| public VoidRequest setCustomFields(Map<String, String> customFields) { |
There was a problem hiding this comment.
So this is a BC as Public API removal, this is a compile fail for anyone using this?
There was a problem hiding this comment.
"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>.
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Can't this be stand alone and the old Signup is simply deprecated?
There was a problem hiding this comment.
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.
a869dc9 to
cd758b1
Compare
cocojoe
left a comment
There was a problem hiding this comment.
Following internal conversation
To the reviewer:
SignUpRequestinterface's implementation class. I usedCreateUserRequest. 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".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.Should close #86