Most of Second PR #2
Conversation
| @@ -0,0 +1,208 @@ | |||
| package com.auth0.msg; | |||
There was a problem hiding this comment.
You haven't refactored for package name?
| import java.io.IOException; | ||
| import java.net.MalformedURLException; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.*; |
| private String input; | ||
| private Error error = null; | ||
| private boolean verified = false; | ||
| ObjectMapper mapper = new ObjectMapper(); |
| * @throws InvalidClaimsException | ||
| */ | ||
| public String toJwt(Key key, Algorithm algorithm) throws InvalidClaimsException, SerializationException { | ||
| return null; |
| } | ||
| } | ||
| if (!errors.isEmpty()) { | ||
| String aggregateError = ""; |
There was a problem hiding this comment.
please reuse errorSB instead of aggregateError.
| * verify that the required claims are present | ||
| * @return whether the verification passed | ||
| */ | ||
| protected boolean verify() { |
There was a problem hiding this comment.
The method signature should tell that this method can throw InvalidClaimsException so that caller can appropriately handle failed validation.
|
|
||
| if (errorSB.length() != 0) { | ||
| errors.add("Message is missing required claims:" + errorSB.toString()); | ||
| return false; |
There was a problem hiding this comment.
This doesn't help - you are adding errors but not returning.
There was a problem hiding this comment.
Modified to put error messages into the InvalidClaimException thrown at the end which should be handled upstream and also update the instance of Error object
| public class ECKeyDefinition extends KeyDefinition { | ||
| private String crv; | ||
|
|
||
| public ECKeyDefinition(KeyType type, KeyUseCase useCase, String crv) { |
There was a problem hiding this comment.
Please explain each of the parameters.
| /** | ||
| * A runtime exception that is thrown when there is an invalid claim in a Message object type | ||
| */ | ||
| public class InvalidClaimsException extends RuntimeException { |
There was a problem hiding this comment.
Extending RuntimeException is not right. You should extend Exception. This should be a checked exception so that caller can handle and recover from this.
|
|
||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
|
|
||
| @Override | ||
| public boolean hasError() { | ||
| return false; |
There was a problem hiding this comment.
why are you overriding hasError and always returning false? What happens when verify fails? If this is a special scenario please provide javadoc to explain that.
| @@ -0,0 +1,87 @@ | |||
| package com.auth0.msg; | |||
There was a problem hiding this comment.
In abstract message, i have suggested some changes to methods, please make the same changes to interface.
No description provided.