Added parser registry - #2
Conversation
…hted-ig-generation-formatter
| return Optional.empty(); | ||
| } | ||
| return parser.parse(idString); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
CODE_MAINTAINABILTIY: In the parse method of IdParsers class, the code logs a warning with just the error message when an exception occurs, losing valuable stack trace information. This makes debugging difficult.
} catch (Exception e) {
log.warn("Could not parse idString {}", idString, e);
return Optional.empty();
}
| private static final Pattern PATTERN = Pattern.compile("(.*)([0-9]{22})"); | ||
| private static final Pattern PATTERN = Pattern.compile("([A-Za-z]*)([0-9]{22})([0-9]{2})?(.*)"); | ||
|
|
||
| private final Map<Integer, IdFormatter> parserRegistry = Map.of( |
There was a problem hiding this comment.
ERROR: The parserRegistry Map only includes the original formatter but does not register the Base36IdFormatter. Since Base36IdFormatter is referenced in IdFormatters.base36(), it should be included in the registry for the parser to work properly.
private final Map<Integer, IdFormatter> parserRegistry = Map.of(
IdFormatters.original().getType().getValue(), IdFormatters.original(),
IdFormatters.base36().getType().getValue(), IdFormatters.base36()
);
|
|
||
| @Getter | ||
| public enum IdParserType { | ||
| DEFAULT (0); |
There was a problem hiding this comment.
ERROR: The IdParserType enum only defines DEFAULT but is missing BASE36 which is needed by Base36IdFormatter. This will prevent the parser registry from being able to properly register Base36IdFormatter.
@Getter
public enum IdParserType {
DEFAULT (0),
BASE36 (1);
private final int value;
IdParserType(final int value) {
this.value = value;
}
}
|
|
||
| @Override | ||
| public IdParserType getType() { | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
- SECURITY: The
getType()method inBase36IdFormatterthrows anUnsupportedOperationException, which could lead to a denial of service if called in production. Implement this method to return a validIdParserType. - CODE_MAINTAINABILTIY: Throwing an exception in the
getType()method is not a good practice for interface implementations. Ensure all implementations ofIdFormatterproperly implement this method. - ERROR: The
getType()method should return a validIdParserTypeto prevent runtime errors when used with the parser registry.
@Override
public IdParserType getType() {
return IdParserType.BASE36;
}
|
DeputyDev has completed a review of your pull request for commit dea19d7. |
DeputyDev generated PR summary:
Size M: This PR changes include 86 lines and should take approximately 30-60 minutes to review
The pull request titled "Added parser registry" introduces several key changes to the codebase, primarily focusing on enhancing the ID parsing mechanism by registering different parsers. Here's a breakdown of what the PR does:
IdFormatter Interface Update:
getType()is added to theIdFormatterinterface, which returns anIdParserType. This allows different implementations ofIdFormatterto specify their type.Enum for IdParserType:
IdParserTypeis introduced to define different types of ID parsers. Currently, it includes a single typeDEFAULT.Base36IdFormatter and DefaultIdFormatter Changes:
getType()method.Base36IdFormatterthrows anUnsupportedOperationException, whileDefaultIdFormatterreturnsIdParserType.DEFAULT.IdParsers Class Enhancements:
parserRegistrymap is introduced to register differentIdFormatterinstances based on their type.parsemethod is updated to utilize this registry to parse IDs based on their type. It attempts to match the ID string against a pattern and retrieves the appropriate parser from the registry.Test Updates:
IdGeneratorTestare updated to reflect changes in ID generation and parsing.IdParsersTestis added to specifically test the parsing logic with the new registry.The main goal of this PR is to allow for a more flexible and extensible ID parsing mechanism by introducing a registry of parsers, which can be expanded with additional types in the future. This makes the system more adaptable to different ID formats.
Here's a sample of how the
IdFormatterinterface has been updated:And the
IdParsersclass with the registry:DeputyDev generated PR summary until dea19d7