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

Avoid using replicated default values for Lists when creating from builder or instance #8428

Merged
merged 8 commits into from
Mar 27, 2024

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Feb 27, 2024

Description

Resolves #8324

When populating a new builder's List property XXX from either a builder or an instance, the generated from code used addXXX. But the List for the property in the new builder had already been initialized with the default values so this code improperly added those default values again.

The PR causes the builder to generate a new protected method addDistinctXXX for each List property, and the generated from methods now invoke that method instead of addXXX to avoid adding duplicate default values to the new builder.

This PR enhances the code generator so it:

  • Creates (in the builder class) an isXxxMutated boolean for each List property xxx.
  • Sets the isXxxMutated to true in the generated addXxx and xxx methods.
  • Generates the two from methods so, before adding the corresponding values from the referenced instance or other builder, checks to see if isXxxMutated is true:
    • If so, from merges the values in the current builder with those from the other object.
    • If not, from overwrites the values in the current builder with those from the other object.

Note: Further, the generator creates each from(Builder) method so that, if this builder's xxx values have been mutated then the values from the other builder are added only if the other builder's values were also mutated. This prevents incorrectly adding defaulted values from the other builder to the mutated values in this builder.

The same approach is not currently taken in generating the from(Prototype) method. That can be a later refinement if the need arises and would require adding isXxxMutated to the generated implementation class and setting it based on the builder's value of isXxxMutated.

The PR adds tests for List properties with single-valued defaults and dual-valued defaults.

This PR touches some files also touched by Tomas's big PR but these changes are minor and should be easily reconciled.

Documentation

Bug fix - no doc impact.

@tjquinno tjquinno self-assigned this Feb 27, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 27, 2024
@tjquinno tjquinno requested a review from Verdent March 4, 2024 21:04
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

This actually has impact on semantics of the behavior. If you have a built value with a list containing duplicate entries (which is valid and expected), you will remove them (because of using something like addDistinctStrings(prototype.strings()); as in DualValuedDefaultValues.
I am not in favor of this behavior. I think we can do distinct for the first instances of the default values themself (this is still not perfect), or we will need to add some concept of "dirty" flag for this kind of fields - i.e. the user has either called clear, or set on this field, it is customized. If we call from on a non-customized field, we could just call clear and set for the values.

I think this deserves a bit of an analysis on the possible states of these fields, to find the right solution.

@tjquinno
Copy link
Member Author

tjquinno commented Mar 6, 2024

Noted. Good point. I have an idea for a fairly clean way to handle this differently. Work in progress.

Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

I have commented in the PR, requesting changes so it does not get merged.

@tjquinno tjquinno force-pushed the 4.x-build-from-instance-dup branch from 8a7528c to fd51901 Compare March 24, 2024 18:36
@tjquinno
Copy link
Member Author

I edited the PR description of how the PR changes the code.

tomas-langer
tomas-langer previously approved these changes Mar 26, 2024
@tjquinno tjquinno marked this pull request as ready for review March 26, 2024 18:46
@tjquinno tjquinno merged commit 2a0b4a7 into helidon-io:main Mar 27, 2024
12 checks passed
@tjquinno tjquinno deleted the 4.x-build-from-instance-dup branch March 27, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[4.x] Creating a builder based on an existing built instance repeats default values
2 participants