Task/remove deprecated projection #1
Conversation
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Thanks @2lambda123 for opening this PR! For COLLABORATOR only :
|
|
Unable to locate .performanceTestingBot config file |
![gitginie[bot]](https://avatars.githubusercontent.com/in/712912?s=60&v=4){kind=small}
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in opening pull request.
Happy coding!
|
Processing PR updates... |
|
First PR by @2lambda123 PR Details of @2lambda123 in NationalSecurityAgency-datawave :
|
Their most recently public accepted PR is: 2lambda123/Accenture-sfmc-devtools#108 |
Reviewer's Guide by SourceryThis pull request removes deprecated projection methods and updates the code to use new constructors with parameters. Deprecated annotations and methods such as setIncludes, setExcludes, and default constructors have been removed from various classes and test files. File-Level Changes
Tips
|
PR summaryThis Pull Request removes deprecated methods and constructors related to projections in the codebase. The changes involve refactoring out deprecated projection calls and updating unit tests to use the new methods. The purpose is to clean up the code by eliminating outdated methods and ensuring that the codebase uses the latest implementations. The impact is a more maintainable and cleaner codebase, reducing the risk of using deprecated methods that may be removed in future versions. SuggestionConsider adding more detailed documentation or comments in the code to explain the new methods and their usage, especially if they replace deprecated ones. This will help future developers understand the changes more easily. Disclaimer: This comment was entirely generated using AI. Be aware that the information provided may be incorrect. Current plan usage: 12.49% Have feedback or need help? |
labels-and-badges
bot
added
NO JIRA
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in closing pull request.
Happy coding!
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| final JsonParser parser = new JsonParser(); | ||
| try (final Reader rdr = new FileReader(fName)) { | ||
| @SuppressWarnings("deprecation") | ||
| final JsonElement json = parser.parse(rdr); |
There was a problem hiding this comment.
Using the deprecated JsonParser class from the com.google.gson package is not recommended as it may be removed in future versions, leading to potential maintenance issues.
Recommended Solution: Replace the deprecated JsonParser with the non-deprecated alternatives provided by the Gson library, such as JsonParser.parseReader(Reader).
| try (final Reader rdr = new FileReader(fName)) { | ||
| @SuppressWarnings("deprecation") | ||
| final JsonElement json = parser.parse(rdr); | ||
| final IngestConfig val = gson.fromJson(json.toString(), IngestConfig.class); | ||
| cfg.set(val); |
There was a problem hiding this comment.
The code does not handle potential JsonSyntaxException or JsonIOException that could be thrown by gson.fromJson(). This could lead to unhandled exceptions and application crashes.
Recommended Solution: Add specific exception handling for JsonSyntaxException and JsonIOException to ensure that the application can handle these errors gracefully.
| public DocumentProjection(boolean includeGroupingContext, boolean reducedResponse, boolean trackSizes, Set<String> projections, | ||
| Projection.ProjectionType projectionType) { |
There was a problem hiding this comment.
The constructor of the DocumentProjection class has too many parameters, which can make it difficult to understand and maintain. Consider using a builder pattern or a configuration object to encapsulate these parameters. This will improve readability and maintainability.
| private boolean initialized; | ||
|
|
||
| @Deprecated | ||
| public void setIncludes(Set<String> includes) { | ||
| if (this.initialized) { | ||
| throw new RuntimeException("This Projection instance was already initialized"); | ||
| } | ||
|
|
||
| // do not make a copy of the incoming include fields. It could be a UniversalSet | ||
| this.projections = includes; | ||
| this.initialized = true; | ||
| type = ProjectionType.INCLUDES; | ||
| } | ||
|
|
||
| @Deprecated | ||
| public void setExcludes(Set<String> excludes) { | ||
| if (this.initialized) { | ||
| throw new RuntimeException("This Projection instance was already initialized"); | ||
| } | ||
|
|
||
| this.projections = Sets.newHashSet(excludes); | ||
| this.initialized = true; | ||
| type = ProjectionType.EXCLUDES; | ||
| } | ||
|
|
||
| @Deprecated | ||
| public Set<String> getIncludes() { | ||
| return Collections.unmodifiableSet(this.projections); | ||
| } | ||
|
|
||
| @Deprecated | ||
| public Set<String> getExcludes() { | ||
| return Collections.unmodifiableSet(this.projections); | ||
| } | ||
|
|
||
| /* Explicit constructor needed now that the new constructor was added */ | ||
| @Deprecated | ||
| public Projection() { | ||
| this.initialized = false; | ||
| } | ||
|
|
||
| private Set<String> projections; | ||
| private ProjectionType type; |
There was a problem hiding this comment.
The fields initialized, projections, and type are not marked as volatile and there are no synchronization mechanisms in place. If this class is intended to be used in a multi-threaded environment, this could lead to data races and inconsistent state.
Recommended Solution: Ensure proper synchronization or use volatile for these fields if they are accessed by multiple threads.
| @@ -56,26 +47,6 @@ public void testNoConfiguration() { | |||
| assertTrue(projection.apply(iter.next())); | |||
| } | |||
There was a problem hiding this comment.
The test testNoConfiguration is expected to throw a RuntimeException, but it does not verify the exception message or type beyond the base RuntimeException. This can lead to false positives if a different RuntimeException is thrown for an unexpected reason.
Recommendation: Use ExpectedException rule or assertThrows to verify the exception message and type more precisely. This will ensure that the test fails for the correct reason.
| @Test(expected = RuntimeException.class) | ||
| public void testNoConfiguration() { | ||
| Projection projection = new Projection(null, Projection.ProjectionType.INCLUDES); | ||
| assertTrue(projection.apply("FIELD_A")); | ||
| } |
There was a problem hiding this comment.
The test testNoConfiguration is expected to throw a RuntimeException, but it does not verify the exception message or type beyond RuntimeException. This can lead to false positives if any RuntimeException is thrown for reasons other than the intended one.
Recommendation: Use ExpectedException rule or assertThrows to verify the exception type and message to ensure the test is validating the correct behavior.
| public void testIncludesDepricated() { | ||
| Projection projection = new Projection(); | ||
| projection.setIncludes(Sets.newHashSet("FIELD_A", "FIELD_B")); | ||
|
|
||
| Projection projection = new Projection(Sets.newHashSet("FIELD_A", "FIELD_B"), Projection.ProjectionType.INCLUDES); | ||
| assertTrue(projection.isUseIncludes()); | ||
| assertFalse(projection.isUseExcludes()); | ||
|
|
There was a problem hiding this comment.
The method name testIncludesDepricated contains a typo. The correct spelling is Deprecated. This can cause confusion and reduce code readability.
Recommendation: Rename the method to testIncludesDeprecated to correct the spelling.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @2lambda123 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding migration instructions or documentation for users transitioning from the deprecated API to the new one.
- The SuppressWarnings annotations in IngestConfig.java suggest there might be more deprecated code to address. Consider updating these as well if possible.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
There was an issue running the performance test |
4 similar comments
|
There was an issue running the performance test |
|
There was an issue running the performance test |
|
There was an issue running the performance test |
|
There was an issue running the performance test |
Description
Related Issue
Types of changes
Checklist:
Summary by Sourcery
Remove deprecated projection methods and constructors, and update test cases to use the new API.
Enhancements: