Skip to content
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

Refactor: FileParser and related implementations in preparation for migration to Spring Kafka #162

Merged
merged 45 commits into from
Jan 31, 2025

Conversation

mcook42
Copy link

@mcook42 mcook42 commented Jan 21, 2025

PR Details

Description

  • Extracted LogParser creation to LogFileParserFactory for testability and separation of concerns
  • Moved setting of recordType and fileName to the constructors of FileParser implementations to streamline code flow and prevent potential bugs
  • Added missing Javadocs to every file touched by LogFileParserFactory extraction and constructor updates
  • Corrected style to align with checkstyle rules in all files touched by refactor
  • Refactored LogFileParser's usage of instanceof to deduplicate casting and improve readability
  • Extracted inline strings into test resource files to improve readability and maintainability of tests
  • Removed unnecessary public modifiers from test methods
  • Migrated away from JMockit in favor of Mockito for improved testability and maintainability (Mockito is defacto mocking framework of Spring Boot) in test files touched by refactor

Related Issue

N/A

Motivation and Context

This change is intended to make migrating the ImporterProcessor to Spring Kafka easier to complete, test, and review. The bulk of this PR is documentation updates. I used the JetBrains AI to generate the bulk of the documentation updates. I then reviewed each update for clarity, accuracy, and consistency. The documentation should reflect the current state of the code.

How Has This Been Tested?

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • Quality of Life improvements (non-breaking change that makes code easier to work with)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
    ODE Contributing Guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Implemented LogFileParserFactory to streamline parser creation based on log file prefixes. Updated relevant classes and tests to use the new factory, ensuring consistent instantiation of log file parsers. This change improves maintainability and readability of the codebase.
Replaced JUnit 4 imports and annotations with JUnit 5 equivalents. Updated test methods to follow JUnit 5 method signatures and added necessary setup using @beforeeach. This improves test consistency and ensures compatibility with modern testing practices.
Replaced JUnit 4 imports and annotations with JUnit 5 equivalents. Updated test methods to follow JUnit 5 method signatures and added necessary setup using @beforeeach. This improves test consistency and ensures compatibility with modern testing practices.
Replaced JUnit 4 imports and annotations with JUnit 5 equivalents. Updated test methods to follow JUnit 5 method signatures and added necessary setup using @beforeeach. This improves test consistency and ensures compatibility with modern testing practices.
Replaced JUnit 4 imports and annotations with JUnit 5 equivalents. Updated test methods to follow JUnit 5 method signatures and added necessary setup using @beforeeach. This improves test consistency and ensures compatibility with modern testing practices.
Replaced JUnit 4 imports and annotations with JUnit 5 equivalents. Updated test methods to follow JUnit 5 method signatures and added necessary setup using @beforeeach. This improves test consistency and ensures compatibility with modern testing practices.
Updated the publish method to explicitly accept a LogFileParser instance, allowing for improved flexibility and better separation of concerns. Adjusted related methods and tests to accommodate the new parameter.
Replaced JMockit with Mockito for mocking behavior in LogFileToAsn1CodecPublisherTest. Simplified dependency initialization and adjusted test methods for compatibility, improving test maintainability and consistency.
Moved inline JSON strings in LogFileToAsn1CodecPublisherTest to external test resource files for better readability and maintainability. Added a utility function to load JSON from files and updated test cases to use these resources.
Updated method implementations to use modern Java features like pattern matching and inline casting for improved readability. Added `@Slf4j` for better logging support and streamlined `buildReceivedMessageDetails` for concise logic and debug messaging when `locationParser` is unavailable.
Removed the unnecessary default initialization of the `ParserStatus` variable in the `parseFile` method. This change ensures cleaner code and avoids redundant assignments during method execution. Functionality remains unaffected.
…Test

Replaced assertTrue with assertInstanceOf to improve clarity and type-specific assertions in parser tests. Updated the factory exception test to assert the correct custom exception type, ensuring better alignment with expected behavior.
Updated file parsers to pass filename and record type via constructor, eliminating the need to pass filenames repeatedly during parsing. Simplified method signatures and improved encapsulation for more streamlined and maintainable code.
Replaced the use of `@Tested` and `@Injectable` annotations with explicit constructors to initialize parser instances. This change improves clarity and aligns with best practices for test setup.
Replaced JUnit 4 assertions with JUnit 5 equivalents in test files. This modernizes the codebase for compatibility with JUnit 5 and aligns with updated testing standards. No functionality changes were introduced.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Reorganize code to improve readability, addressing checkstyle violations such as indentation and formatting. Documented class details and parsing steps to enhance code clarity and maintainability.
Copy link
Author

@mcook42 mcook42 left a comment

Choose a reason for hiding this comment

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

I highly recommend setting Show whitespace to false, as these files had some wild code formatting.

Comment on lines +185 to +188
if (locationParser == null) {
log.debug("No locationParser available - returning null");
return null;
}
Copy link
Author

Choose a reason for hiding this comment

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

Note: Instead of nesting some core logic under an if statement, I adjusted the code to return null early to improve its readability. The previous implementation is logically identical to this. Now, the core of the method's logic is unnested, and the reader can more quickly identify that a populated ReceivedMessageDetails object is only returned if the locationParser is instantiated.

@mcook42 mcook42 marked this pull request as ready for review January 21, 2025 22:01
Copy link
Collaborator

@drewjj drewjj left a comment

Choose a reason for hiding this comment

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

I like the changes made to the log parser. I have also tested the parser with each of the different types of supported logs and the functionality is definitely maintained. Unit tests pass as well.

Copy link
Member

@dmccoystephenson dmccoystephenson left a comment

Choose a reason for hiding this comment

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

Looks good & the unit tests pass! I just had some non-blocking suggestions/questions.

@mcook42 mcook42 merged commit 1584977 into dev Jan 31, 2025
3 checks passed
@mcook42 mcook42 deleted the mcook42/spring-kafka/importer-processor-docs-and-style branch January 31, 2025 16:52
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.

3 participants