-
Notifications
You must be signed in to change notification settings - Fork 0
Added parser registry #2
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
base: main
Are you sure you want to change the base?
Conversation
…hted-ig-generation-formatter
log.warn("Could not parse idString {}, Invalid formatter type {}", idString, parserType); | ||
return Optional.empty(); | ||
} | ||
return parser.parse(idString); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
}
@@ -29,6 +29,11 @@ public Base36IdFormatter(IdFormatter idFormatter) { | |||
this.idFormatter = idFormatter; | |||
} | |||
|
|||
@Override | |||
public IdParserType getType() { | |||
throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- SECURITY: The
getType()
method inBase36IdFormatter
throws 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 ofIdFormatter
properly implement this method. - ERROR: The
getType()
method should return a validIdParserType
to 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 theIdFormatter
interface, which returns anIdParserType
. This allows different implementations ofIdFormatter
to specify their type.Enum for IdParserType:
IdParserType
is introduced to define different types of ID parsers. Currently, it includes a single typeDEFAULT
.Base36IdFormatter and DefaultIdFormatter Changes:
getType()
method.Base36IdFormatter
throws anUnsupportedOperationException
, whileDefaultIdFormatter
returnsIdParserType.DEFAULT
.IdParsers Class Enhancements:
parserRegistry
map is introduced to register differentIdFormatter
instances based on their type.parse
method 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:
IdGeneratorTest
are updated to reflect changes in ID generation and parsing.IdParsersTest
is 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
IdFormatter
interface has been updated:And the
IdParsers
class with the registry:DeputyDev generated PR summary until dea19d7