Refactor WrapperProperties template to remove caching and simplify implementation#2436
Merged
gnodet merged 2 commits intoapache:masterfrom Jun 12, 2025
Merged
Conversation
…plementation This commit refactors the WrapperProperties template used for Maven model properties: TEMPLATE CHANGES (src/mdo/java/WrapperProperties.java): - Removed caching mechanism (orderedProps field and ensureInitialized() method) - Simplified read operations to directly delegate to getter.get() - Introduced writeOperation() pattern for all write operations that: * Creates a fresh OrderedProperties from current state * Performs the operation on the copy * Only calls setter if changes were made - All write operations now use synchronized writeOperation() wrapper - Maintains insertion order through OrderedProperties inner class - Supports multiple wrapper instances sharing the same backend storage The new approach eliminates cache invalidation complexity while ensuring: - Thread safety through synchronized write operations - Insertion order preservation - Immediate visibility of changes across wrapper instances - Proper delegation to underlying storage SIDE EFFECTS: - Consolidated redundant test classes into single PropertiesTest - Removed PropertiesOrderTest and WrapperPropertiesOrderTest (functionality covered by PropertiesTest) - Cleaned up duplicate test methods in PropertiesTest This refactoring simplifies the WrapperProperties implementation while maintaining all required functionality for Maven model property management.
gnodet
force-pushed
the
cleanup-redundant-properties-tests
branch
from
a345aea to
81bc67c
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Pankraz76
reviewed
Jun 5, 2025
src/mdo/java/WrapperProperties.java
Outdated
| private Object writeReplace() throws java.io.ObjectStreamException { | ||
| Properties props = new Properties(); | ||
| props.putAll(getter.get()); | ||
| OrderedProperties props = new OrderedProperties(getter.get()); |
Contributor
There was a problem hiding this comment.
yes it is, its declared L:340 and used L:341 in return props;.
lets just ask judge PMD, im sure it will point out the same.
Pankraz76
reviewed
Jun 5, 2025
cstamas
approved these changes
Jun 7, 2025
Pankraz76
reviewed
Jun 7, 2025
| setter.accept(orderedProps); | ||
| writeOperation(properties -> { | ||
| runner.perform(properties); | ||
| return null; |
Contributor
There was a problem hiding this comment.
Suggested change
| return null; | |
| return null; |
format seems broken as offset.
Pankraz76
reviewed
Jun 7, 2025
Comment on lines
+195
to
+196
| runner.perform(properties); | ||
| return null; |
Contributor
There was a problem hiding this comment.
Suggested change
| runner.perform(properties); | |
| return null; | |
| runner.perform(properties); | |
| return null; |
format seems broken as offset.
Contributor
Author
There was a problem hiding this comment.
We use auto formatting, so either it fails or we need to accept the output. If the CI test pass, we cannot change the formatting.
Pankraz76
reviewed
Jun 8, 2025
Comment on lines
+39
to
+40
| @Nested | ||
| class OrderPreservationTests { |
Contributor
There was a problem hiding this comment.
Suggested change
| @Nested | |
| class OrderPreservationTests { |
why nested if no rivals?
i like this, but only if its really in use. ATM it seems not.
FYI: see the nested feature again. @elharo
Contributor
There was a problem hiding this comment.
overhead for no obvious reason, seems to be the thing here all around.
Pankraz76
reviewed
Jun 8, 2025
| assertEquals("putall.value1", wrapper.getProperty("putall.key1")); | ||
| assertEquals("putall.value2", wrapper.getProperty("putall.key2")); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit refactors the WrapperProperties template used for Maven model properties:
TEMPLATE CHANGES (src/mdo/java/WrapperProperties.java):
The new approach eliminates cache invalidation complexity while ensuring:
SIDE EFFECTS:
This refactoring simplifies the WrapperProperties implementation while maintaining
all required functionality for Maven model property management.