Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Added parser registry #2

wants to merge 8 commits into from

Conversation

ankit27755
Copy link
Owner

@ankit27755 ankit27755 commented Apr 30, 2025


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:

  1. IdFormatter Interface Update:

    • A new method getType() is added to the IdFormatter interface, which returns an IdParserType. This allows different implementations of IdFormatter to specify their type.
  2. Enum for IdParserType:

    • An enum IdParserType is introduced to define different types of ID parsers. Currently, it includes a single type DEFAULT.
  3. Base36IdFormatter and DefaultIdFormatter Changes:

    • Both classes now implement the getType() method. Base36IdFormatter throws an UnsupportedOperationException, while DefaultIdFormatter returns IdParserType.DEFAULT.
  4. IdParsers Class Enhancements:

    • A parserRegistry map is introduced to register different IdFormatter instances based on their type.
    • The 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.
  5. Test Updates:

    • Tests in IdGeneratorTest are updated to reflect changes in ID generation and parsing.
    • A new test class 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:

public interface IdFormatter {

    IdParserType getType();

    String format(final DateTime dateTime,
                  final int nodeId,
                  final int randomNonce);
}

And the IdParsers class with the registry:

@Slf4j
@UtilityClass
public class IdParsers {
    private static final int MINIMUM_ID_LENGTH = 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(
            IdFormatters.original().getType().getValue(), IdFormatters.original()
    );

    public Optional<Id> parse(final String idString) {
        try {
            if (idString.length() < MINIMUM_ID_LENGTH) {
                return Optional.empty();
            }
            val matcher = PATTERN.matcher(idString);
            if (!matcher.find()) {
                return Optional.empty();
            }

            val parserType = matcher.group(3);
            if (parserType == null) {
                return IdFormatters.original().parse(idString);
            }

            val parser = parserRegistry.get(Integer.parseInt(matcher.group(3)));
            if (parser == null) {
                log.warn("Could not parse idString {}, Invalid formatter type {}", idString, parserType);
                return Optional.empty();
            }
            return parser.parse(idString);
        } catch (Exception e) {
            log.warn("Could not parse idString {}", e.getMessage());
            return Optional.empty();
        }
    }
}

DeputyDev generated PR summary until dea19d7

log.warn("Could not parse idString {}, Invalid formatter type {}", idString, parserType);
return Optional.empty();
}
return parser.parse(idString);
} catch (Exception e) {

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(

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);

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • SECURITY: The getType() method in Base36IdFormatter throws an UnsupportedOperationException, which could lead to a denial of service if called in production. Implement this method to return a valid IdParserType.
  • CODE_MAINTAINABILTIY: Throwing an exception in the getType() method is not a good practice for interface implementations. Ensure all implementations of IdFormatter properly implement this method.
  • ERROR: The getType() method should return a valid IdParserType to prevent runtime errors when used with the parser registry.
@Override
public IdParserType getType() {
    return IdParserType.BASE36;
}

Copy link

DeputyDev has completed a review of your pull request for commit dea19d7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant