Address review comments vol. 03

Signed-off-by: Václav Muzikář <[email protected]>
vmuzikar · Dec 11, 2023 · e91a93d · e91a93d
1 parent 8eeea00
commit e91a93d
Show file tree

Hide file tree

Showing 19 changed files with 193 additions and 199 deletions.
diff --git a/docs/guides/server/hostname.adoc b/docs/guides/server/hostname.adoc
@@ -40,7 +40,7 @@ Most of the time, it should be enough to set the `hostname` option in order to c
 When using the `hostname` option the server is going to resolve the HTTP scheme, port, and path, automatically so that:
 
 * `https` scheme is used unless you set `hostname-strict-https=false`
-* if the `proxy-headers` option is set, the proxy will use the default ports (i.e.: 80 and 443).  If the proxy uses a different port, it needs to be specified via the `hostname-port` configuration option
+* if the `proxy-headers` option is set, the proxy will use the default ports (i.e.: 80 and 443).  If the proxy uses a different port, it needs to be specified via the `hostname-url` configuration option
 
 However, if you want to set not only the host but also a scheme, port, and path, you can set the `hostname-url` option:
 

diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/DeprecatedMetadata.java b/quarkus/config-api/src/main/java/org/keycloak/config/DeprecatedMetadata.java
@@ -19,25 +19,26 @@
 
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
 /**
  * @author Vaclav Muzikar <[email protected]>
  */
 public class DeprecatedMetadata {
-    private final List<String> newOptionsKeys;
+    private final Set<String> newOptionsKeys;
     private final String note;
 
     public DeprecatedMetadata() {
-        newOptionsKeys = Collections.emptyList();
+        newOptionsKeys = Collections.emptySet();
         note = null;
     }
 
-    public DeprecatedMetadata(List<String> newOptionsKeys, String note) {
-        this.newOptionsKeys = newOptionsKeys == null ? Collections.emptyList() : Collections.unmodifiableList(newOptionsKeys);
+    public DeprecatedMetadata(Set<String> newOptionsKeys, String note) {
+        this.newOptionsKeys = newOptionsKeys == null ? Collections.emptySet() : Collections.unmodifiableSet(newOptionsKeys);
         this.note = note;
     }
 
-    public List<String> getNewOptionsKeys() {
+    public Set<String> getNewOptionsKeys() {
         return newOptionsKeys;
     }
 

diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java
@@ -90,21 +90,21 @@ public enum ClientAuth {
     public static final Option HTTPS_TRUST_STORE_FILE = new OptionBuilder<>("https-trust-store-file", File.class)
             .category(OptionCategory.HTTP)
             .description("The trust store which holds the certificate information of the certificates to trust.")
-            .deprecatedWithNote("Use the System Truststore instead, see the docs for details.")
+            .deprecated("Use the System Truststore instead, see the docs for details.")
             .build();
 
     public static final Option HTTPS_TRUST_STORE_PASSWORD = new OptionBuilder<>("https-trust-store-password", String.class)
             .category(OptionCategory.HTTP)
             .description("The password of the trust store file.")
-            .deprecatedWithNote("Use the System Truststore instead, see the docs for details.")
+            .deprecated("Use the System Truststore instead, see the docs for details.")
             .build();
 
     public static final Option<String> HTTPS_TRUST_STORE_TYPE = new OptionBuilder<>("https-trust-store-type", String.class)
             .category(OptionCategory.HTTP)
             .description("The type of the trust store file. " +
                     "If not given, the type is automatically detected based on the file name. " +
                     "If '" + SecurityOptions.FIPS_MODE.getKey() + "' is set to '" + FipsMode.STRICT + "' and no value is set, it defaults to 'BCFKS'.")
-            .deprecatedWithNote("Use the System Truststore instead, see the docs for details.")
+            .deprecated("Use the System Truststore instead, see the docs for details.")
             .build();
 
     public static final Option<Boolean> HTTP_SERVER_ENABLED = new OptionBuilder<>("http-server-enabled", Boolean.class)

diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java b/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java
@@ -1,9 +1,8 @@
 package org.keycloak.config;
 
-import java.util.Arrays;
-import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
@@ -119,18 +118,18 @@ public OptionBuilder<T> deprecated() {
         return this;
     }
 
-    public OptionBuilder<T> deprecatedWithNote(String note) {
+    public OptionBuilder<T> deprecated(String note) {
         this.deprecatedMetadata = new DeprecatedMetadata(null, note);
         return this;
     }
 
-    public OptionBuilder<T> deprecated(String... newOptionsKeys) {
-        this.deprecatedMetadata = new DeprecatedMetadata(Arrays.asList(newOptionsKeys), null);
+    public OptionBuilder<T> deprecated(Set<String> newOptionsKeys) {
+        this.deprecatedMetadata = new DeprecatedMetadata(newOptionsKeys, null);
         return this;
     }
 
-    public OptionBuilder<T> deprecatedWithNote(String note, String... newOptionsKeys) {
-        this.deprecatedMetadata = new DeprecatedMetadata(Arrays.asList(newOptionsKeys), note);
+    public OptionBuilder<T> deprecated(String note, Set<String> newOptionsKeys) {
+        this.deprecatedMetadata = new DeprecatedMetadata(newOptionsKeys, note);
         return this;
     }
 

diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/ProxyOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/ProxyOptions.java
@@ -1,5 +1,7 @@
 package org.keycloak.config;
 
+import java.util.Set;
+
 public class ProxyOptions {
 
     public enum Headers {
@@ -37,7 +39,7 @@ public boolean isProxyHeadersEnabled() {
             .category(OptionCategory.PROXY)
             .description("The proxy address forwarding mode if the server is behind a reverse proxy.")
             .defaultValue(Mode.none)
-            .deprecated(PROXY_HEADERS.getKey())
+            .deprecated(Set.of(PROXY_HEADERS.getKey()))
             .build();
 
     public static final Option<Boolean> PROXY_FORWARDED_HOST = new OptionBuilder<>("proxy-forwarded-host", Boolean.class)

diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/OptionRenderer.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/OptionRenderer.java
@@ -47,16 +47,14 @@ public Text[][] render(OptionSpec option, IParamLabelRenderer paramLabelRenderer
         Text shortName = names.length > 1 ? scheme.optionText(names[0]) : EMPTY_TEXT;
         Text longName = createLongName(option, scheme);
         Text[][] result = new Text[1][];
-        String[] descriptions = option.description();
+        Text description = scheme.text(option.description()[0]);
 
         // for better formatting, only a single line is expected in the description
         // formatting is done by customizations to the text table
-        if (descriptions.length > 1) {
+        if (option.description().length > 1) {
             throw new CommandLine.PicocliException("Option[" + option + "] description should have a single line.");
         }
 
-        Text description = formatDescription(descriptions, option, scheme);
-
         if (EMPTY_TEXT.equals(shortName)) {
             result[0] = new Text[] { longName, description };
         } else {
@@ -66,26 +64,6 @@ public Text[][] render(OptionSpec option, IParamLabelRenderer paramLabelRenderer
         return result;
     }
 
-    private Text formatDescription(String[] descriptions, OptionSpec option, ColorScheme scheme) {
-        String description = descriptions[0];
-        String defaultValue = option.defaultValue();
-        Iterable<String> completionCandidates = option.completionCandidates();
-
-        if (!option.type().equals(Boolean.class) && completionCandidates != null) {
-            List<String> expectedValues = StreamSupport.stream(completionCandidates.spliterator(), false).collect(Collectors.toList());
-
-            if (!expectedValues.isEmpty()) {
-                description = description + " Possible values are: " + String.join(", ", expectedValues) + ".";
-            }
-        }
-
-        if (defaultValue != null) {
-            description = description + " Default: " + defaultValue + ".";
-        }
-
-        return scheme.text(description);
-    }
-
     private Text createLongName(OptionSpec option, ColorScheme scheme) {
         Text name = scheme.optionText(option.longestName());
         String paramLabel = formatParamLabel(option);

diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java
@@ -30,6 +30,7 @@
 import static org.keycloak.quarkus.runtime.configuration.Configuration.getCurrentBuiltTimeProperty;
 import static org.keycloak.quarkus.runtime.configuration.Configuration.getRawPersistedProperty;
 import static org.keycloak.quarkus.runtime.configuration.Configuration.getRuntimeProperty;
+import static org.keycloak.quarkus.runtime.configuration.MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX;
 import static org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers.formatValue;
 import static org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers.isBuildTimeProperty;
 import static org.keycloak.utils.StringUtil.isNotBlank;
@@ -48,12 +49,10 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.TreeSet;
 import java.util.function.BiConsumer;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
-import io.quarkus.logging.Log;
 import org.eclipse.microprofile.config.spi.ConfigSource;
 import org.jboss.logging.Logger;
 import org.keycloak.config.DeprecatedMetadata;
@@ -316,8 +315,13 @@ public static void validateNonCliConfig(List<String> cliArgs, AbstractCommand ab
 
                 mapper.getDeprecatedMetadata().ifPresent(d -> {
                     DeprecatedMetadata metadata = (DeprecatedMetadata) d;
+                    String optionName = mapper.getFrom();
+                    if (optionName.startsWith(NS_KEYCLOAK_PREFIX)) {
+                        optionName = optionName.substring(NS_KEYCLOAK_PREFIX.length());
+                    }
+
                     StringBuilder sb = new StringBuilder("\t- ");
-                    sb.append(mapper.getFrom());
+                    sb.append(optionName);
                     if (metadata.getNote() != null || !metadata.getNewOptionsKeys().isEmpty()) {
                         sb.append(":");
                     }
@@ -596,7 +600,7 @@ private static void addMappedOptionsToArgGroups(CommandLine commandLine, Map<Opt
                 }
 
                 OptionSpec.Builder optBuilder = OptionSpec.builder(name)
-                        .description(getTransformedOptionDescription(mapper))
+                        .description(getDecoratedOptionDescription(mapper))
                         .paramLabel(mapper.getParamLabel())
                         .completionCandidates(new Iterable<String>() {
                             @Override
@@ -631,26 +635,36 @@ public Iterator<String> iterator() {
         }
     }
 
-    private static String getTransformedOptionDescription(PropertyMapper<?> mapper) {
+    private static String getDecoratedOptionDescription(PropertyMapper<?> mapper) {
         StringBuilder transformedDesc = new StringBuilder(mapper.getDescription());
 
+        if (mapper.getType() != Boolean.class && !mapper.getExpectedValues().isEmpty()) {
+            transformedDesc.append(" Possible values are: " + String.join(", ", mapper.getExpectedValues()) + ".");
+        }
+
+        mapper.getDefaultValue().map(d -> " Default: " + d + ".").ifPresent(transformedDesc::append);
+
         mapper.getDeprecatedMetadata().ifPresent(deprecatedMetadata -> {
             List<String> deprecatedDetails = new ArrayList<>();
-            if (deprecatedMetadata.getNote() != null) {
-                deprecatedDetails.add(deprecatedMetadata.getNote());
+            String note = deprecatedMetadata.getNote();
+            if (note != null) {
+                if (!note.endsWith(".")) {
+                    note += ".";
+                }
+                deprecatedDetails.add(note);
             }
             if (!deprecatedMetadata.getNewOptionsKeys().isEmpty()) {
-                deprecatedDetails.add("Use: " + String.join(", ", deprecatedMetadata.getNewOptionsKeys()) + ".");
+                String s = deprecatedMetadata.getNewOptionsKeys().size() > 1 ? "s" : "";
+                deprecatedDetails.add("Consider non-deprecated option" + s + ": " + String.join(", ", deprecatedMetadata.getNewOptionsKeys()) + ".");
             }
 
-            StringBuilder deprecateDesc = new StringBuilder("@|bold DEPRECATED.");
+            transformedDesc.insert(0, "@|bold DEPRECATED.|@ ");
             if (!deprecatedDetails.isEmpty()) {
-                deprecateDesc
-                        .append(" ")
-                        .append(String.join(" ", deprecatedDetails));
+                transformedDesc
+                        .append(" @|bold ")
+                        .append(String.join(" ", deprecatedDetails))
+                        .append("|@");
             }
-            deprecateDesc.append("|@ -- ");
-            transformedDesc.insert(0, deprecateDesc);
         });
 
         return transformedDesc.toString();

diff --git a/...oak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelp.unix.approved.txt b/...oak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelp.unix.approved.txt
@@ -150,17 +150,17 @@ HTTP(S):
 --https-protocols <protocols>
                      The list of protocols to explicitly enable. Default: TLSv1.3,TLSv1.2.
 --https-trust-store-file <file>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The trust store which holds the certificate information of the certificates
-                       to trust.
+                     DEPRECATED. The trust store which holds the certificate information of the
+                       certificates to trust. Use the System Truststore instead, see the docs for
+                       details.
 --https-trust-store-password <password>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The password of the trust store file.
+                     DEPRECATED. The password of the trust store file. Use the System Truststore
+                       instead, see the docs for details.
 --https-trust-store-type <type>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The type of the trust store file. If not given, the type is automatically
-                       detected based on the file name. If 'fips-mode' is set to 'strict' and no
-                       value is set, it defaults to 'BCFKS'.
+                     DEPRECATED. The type of the trust store file. If not given, the type is
+                       automatically detected based on the file name. If 'fips-mode' is set to
+                       'strict' and no value is set, it defaults to 'BCFKS'. Use the System
+                       Truststore instead, see the docs for details.
 
 Health:
 
@@ -186,9 +186,9 @@ Metrics:
 
 Proxy:
 
---proxy <mode>       DEPRECATED. Use: proxy-headers. -- The proxy address forwarding mode if the
-                       server is behind a reverse proxy. Possible values are: none, edge,
-                       reencrypt, passthrough. Default: none.
+--proxy <mode>       DEPRECATED. The proxy address forwarding mode if the server is behind a
+                       reverse proxy. Possible values are: none, edge, reencrypt, passthrough.
+                       Default: none. Consider non-deprecated option: proxy-headers.
 --proxy-headers <headers>
                      The proxy headers that should be accepted by the server. Misconfiguration
                        might leave the server exposed to security vulnerabilities. Takes precedence

diff --git a/.../it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelp.windows.approved.txt b/.../it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelp.windows.approved.txt
@@ -145,17 +145,17 @@ HTTP(S):
 --https-protocols <protocols>
                      The list of protocols to explicitly enable. Default: TLSv1.3,TLSv1.2.
 --https-trust-store-file <file>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The trust store which holds the certificate information of the certificates
-                       to trust.
+                     DEPRECATED. The trust store which holds the certificate information of the
+                       certificates to trust. Use the System Truststore instead, see the docs for
+                       details.
 --https-trust-store-password <password>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The password of the trust store file.
+                     DEPRECATED. The password of the trust store file. Use the System Truststore
+                       instead, see the docs for details.
 --https-trust-store-type <type>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The type of the trust store file. If not given, the type is automatically
-                       detected based on the file name. If 'fips-mode' is set to 'strict' and no
-                       value is set, it defaults to 'BCFKS'.
+                     DEPRECATED. The type of the trust store file. If not given, the type is
+                       automatically detected based on the file name. If 'fips-mode' is set to
+                       'strict' and no value is set, it defaults to 'BCFKS'. Use the System
+                       Truststore instead, see the docs for details.
 
 Health:
 
@@ -172,9 +172,9 @@ Metrics:
 
 Proxy:
 
---proxy <mode>       DEPRECATED. Use: proxy-headers. -- The proxy address forwarding mode if the
-                       server is behind a reverse proxy. Possible values are: none, edge,
-                       reencrypt, passthrough. Default: none.
+--proxy <mode>       DEPRECATED. The proxy address forwarding mode if the server is behind a
+                       reverse proxy. Possible values are: none, edge, reencrypt, passthrough.
+                       Default: none. Consider non-deprecated option: proxy-headers.
 --proxy-headers <headers>
                      The proxy headers that should be accepted by the server. Misconfiguration
                        might leave the server exposed to security vulnerabilities. Takes precedence

diff --git a/.../it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelpAll.unix.approved.txt b/.../it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelpAll.unix.approved.txt
@@ -150,17 +150,17 @@ HTTP(S):
 --https-protocols <protocols>
                      The list of protocols to explicitly enable. Default: TLSv1.3,TLSv1.2.
 --https-trust-store-file <file>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The trust store which holds the certificate information of the certificates
-                       to trust.
+                     DEPRECATED. The trust store which holds the certificate information of the
+                       certificates to trust. Use the System Truststore instead, see the docs for
+                       details.
 --https-trust-store-password <password>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The password of the trust store file.
+                     DEPRECATED. The password of the trust store file. Use the System Truststore
+                       instead, see the docs for details.
 --https-trust-store-type <type>
-                     DEPRECATED. Use the System Truststore instead, see the docs for details. --
-                       The type of the trust store file. If not given, the type is automatically
-                       detected based on the file name. If 'fips-mode' is set to 'strict' and no
-                       value is set, it defaults to 'BCFKS'.
+                     DEPRECATED. The type of the trust store file. If not given, the type is
+                       automatically detected based on the file name. If 'fips-mode' is set to
+                       'strict' and no value is set, it defaults to 'BCFKS'. Use the System
+                       Truststore instead, see the docs for details.
 
 Health:
 
@@ -186,9 +186,9 @@ Metrics:
 
 Proxy:
 
---proxy <mode>       DEPRECATED. Use: proxy-headers. -- The proxy address forwarding mode if the
-                       server is behind a reverse proxy. Possible values are: none, edge,
-                       reencrypt, passthrough. Default: none.
+--proxy <mode>       DEPRECATED. The proxy address forwarding mode if the server is behind a
+                       reverse proxy. Possible values are: none, edge, reencrypt, passthrough.
+                       Default: none. Consider non-deprecated option: proxy-headers.
 --proxy-headers <headers>
                      The proxy headers that should be accepted by the server. Misconfiguration
                        might leave the server exposed to security vulnerabilities. Takes precedence