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

Add a way for the builder mechanism to provide access to the Config object passed to a builder #7358

Closed
tjquinno opened this issue Aug 10, 2023 · 3 comments
Assignees
Labels
4.x Version 4.x bug Something isn't working builder Related to the builder support triage

Comments

@tjquinno
Copy link
Member

Environment Details

  • Helidon Version: 4.x
  • Helidon SE or Helidon MP
  • JDK version:
  • OS:
  • Docker version (if applicable):

Problem Description

It would be very useful if the builder mechanism provided a way to retrieve, from the built XXXConfig object, the Config (if any) that was passed to the builder's config(Config) method.

For an example, suppose we create MetricsConfigBlueprint that starts out like this:

@ConfigBean()
@Configured(root = true, prefix = MetricsConfigBlueprint.METRICS_CONFIG_KEY)
@Prototype.Blueprint(decorator = MetricsConfigBlueprint.BuilderDecorator.class)
@Prototype.CustomMethods(MetricsConfigSupport.class)
interface MetricsConfigBlueprint {

Tomas suggested these steps as a workaround which almost worked but exposed a small bug:

  • Add a method to the blueprint:

    @ConfiguredOption(builderMethod = false, configured = false)
    Config metricsConfig();
  • In the builder decorator method (run as build() executes):

        class BuilderDecorator implements Prototype.BuilderDecorator<MetricsConfig.BuilderBase<?, ?>> {
    
          @Override
          public void decorate(MetricsConfig.BuilderBase<?, ?> builder) {
              if (builder.config().isEmpty()) {
                  builder.config(GlobalConfig.config().get(METRICS_CONFIG_KEY));
              }
              builder.metricsConfig(target.config().get());
          }
      }
  • Write a custom method for the prototype that refers to the saved config instance:

    class MetricsConfigSupport {
    
      @Prototype.PrototypeMethod
      static Optional<String> lookupConfig(MetricsConfig metricsConfig, String key) {
          return metricsConfig.metricsConfig()
                  .get(key)
                  .asString()
                  .asOptional();
      }

Tomas provided this patch that allows this approach to work:

Subject: [PATCH] Fix prototype builder for Config type
---
Index: builder/processor/src/main/java/io/helidon/builder/processor/GenerateAbstractBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/builder/processor/src/main/java/io/helidon/builder/processor/GenerateAbstractBuilder.java b/builder/processor/src/main/java/io/helidon/builder/processor/GenerateAbstractBuilder.java
--- a/builder/processor/src/main/java/io/helidon/builder/processor/GenerateAbstractBuilder.java	(revision af5d14b8a9565af4011713a94a631748b6b87322)
+++ b/builder/processor/src/main/java/io/helidon/builder/processor/GenerateAbstractBuilder.java	(date 1691673740771)
@@ -628,7 +628,7 @@
             pw.println("private Config config;");
         }
         for (PrototypeProperty child : typeContext.propertyData().properties()) {
-            if (!isBuilder || !child.typeHandler().actualType().equals(CONFIG_TYPE)) {
+            if (!isBuilder || !(child.typeHandler().actualType().equals(CONFIG_TYPE) && child.name().equals("config"))) {
                 pw.print(spacing);
                 pw.print(child.fieldDeclaration(isBuilder));
                 pw.println(";");
Index: builder/processor/src/main/java/io/helidon/builder/processor/PrototypeProperty.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/builder/processor/src/main/java/io/helidon/builder/processor/PrototypeProperty.java b/builder/processor/src/main/java/io/helidon/builder/processor/PrototypeProperty.java
--- a/builder/processor/src/main/java/io/helidon/builder/processor/PrototypeProperty.java	(revision af5d14b8a9565af4011713a94a631748b6b87322)
+++ b/builder/processor/src/main/java/io/helidon/builder/processor/PrototypeProperty.java	(date 1691673524635)
@@ -324,7 +324,7 @@
                             .map(typeHandler::toDefaultValue)
                             .orElse(null),
                     configuredAnnotation.getValue("builderMethod").map(Boolean::parseBoolean).orElse(true),
-                    configuredAnnotation.getValue("notConfigured").map(Boolean::parseBoolean).orElse(false),
+                    !configuredAnnotation.getValue("configured").map(Boolean::parseBoolean).orElse(true),
                     provider,
                     provider ? ProviderOption.create(configuredAnnotation) : null);
         }
@tjquinno tjquinno added 4.x Version 4.x triage builder Related to the builder support labels Aug 10, 2023
@tomas-langer
Copy link
Member

The intendend approach was to allow definition of Optional<Config> config() method on prototype.
All the infrustructure code is there (the method is correctly generated on implementation), but due to some changes in the builder code (refactoring of getters, where it used to implement the prototype and now has dedicated getters), the feature got broken (and was not tested).

Solution: add a check when generating the package local Optional<Config> config() method on builder base, and if such a method is declared on prototype, generate it as public (and do not generated it during the usual processing of fields)

@tomas-langer tomas-langer self-assigned this Aug 11, 2023
@tomas-langer tomas-langer added the bug Something isn't working label Aug 11, 2023
@tomas-langer
Copy link
Member

Another issue with builders - if a type has static Type create(Config), treat it as a static factory method (even if the type is not a prototype)

@tomas-langer
Copy link
Member

Waiting for #7256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x bug Something isn't working builder Related to the builder support triage
Projects
Archived in project
Development

No branches or pull requests

2 participants