Adding support for Expression Resolver #143
Conversation
| @@ -99,7 +99,7 @@ public class EngineConfiguration extends Configuration | |||
| ENGINE_CREDITOR_CHILD_CLEANUP_LINGER_MILLIS = config.property("child.cleanup.linger", SECONDS.toMillis(5L)); | |||
| ENGINE_VERBOSE = config.property("verbose", false); | |||
| ENGINE_WORKERS = config.property("workers", Runtime.getRuntime().availableProcessors()); | |||
| ENGINE_CONFIG_SYNTAX_MUSTACHE = config.property("config.syntax.mustache", false); | |||
| ENGINE_CONFIG_SYNTAX_MUSTACHE = config.property("config.syntax.mustache", true); | |||
There was a problem hiding this comment.
We should also rename this to something like config.resolve.expressions and change the constant name and method name accordingly.
| for (String context: expressionResolver.names()) | ||
| { | ||
| configText = expressionResolver.resolve(context, configText); | ||
| } |
There was a problem hiding this comment.
We can do this in a single pass instead of a loop, no need to pass each context separately.
| public class EnvironmentResolverSpi implements ExpressionResolverSpi | ||
| { | ||
|
|
||
| private static final Pattern MUSTACHE_PATTERN = Pattern.compile("\\$\\{\\{\\s*env\\.([^\\s\\}]*)\\s*\\}\\}"); |
There was a problem hiding this comment.
Rename to EXPRESSION_PATTERN instead.
Also, this can be moved to the ExpressionResolver itself, match the pattern, extract the context, then use the appropriate ExpressionResolverSpi to resolve the value.
Each ExpressionResolverSpi implementation should be as simple as possible to avoid duplication when we extend to support more contexts.
| return MUSTACHE_PATTERN | ||
| .matcher(config) | ||
| .replaceAll(r -> Optional.ofNullable(env.apply(r.group(1))).orElse("")); | ||
| } |
There was a problem hiding this comment.
This implementation can simplify to return System.getenv(var); where the String parameter represents the variable name, such as SASL_USERNAME in env.SASL_USERNAME.
| return resolverSpis.keySet(); | ||
| } | ||
|
|
||
| public ExpressionResolver(Map<String, ExpressionResolverSpi> resolverSpis) |
There was a problem hiding this comment.
This should be a private constructor so that only instantiate is used to create a new instance via service loader.
| public Iterable<String> names() | ||
| { | ||
| return resolverSpis.keySet(); | ||
| } |
There was a problem hiding this comment.
We probably don't need this one exposed externally as lookup is happening inside resolve(...).
|
|
||
| public String resolve( | ||
| String name, | ||
| String config) |
There was a problem hiding this comment.
Just a single argument, config needed here.
Applying the pattern to the config allows us to find each occurrence of ${{ context.var }} and then lookup the resolver SPI for context to resolve var.
| return resolverSpi.resolve(config); | ||
| } | ||
|
|
||
| private static ExpressionResolver instantiate(ServiceLoader<ExpressionResolverSpi> resolvers) |
There was a problem hiding this comment.
Please check your indentation of method parameters (all methods), single indent, next line per parameter.
| { | ||
| String name(); | ||
|
|
||
| String resolve(String config); |
| @@ -0,0 +1,9 @@ | |||
| package io.aklivity.zilla.runtime.engine.resolver; | |||
There was a problem hiding this comment.
Please refactor package name to io.aklivity.zilla.runtime.engine.expression instead.
| public String resolve( | ||
| String config) | ||
| { | ||
| return EXPRESSION_PATTERN.matcher(config) |
There was a problem hiding this comment.
Instead of recreating the Matcher on every call to resolve(config), suggest instead creating the matcher via EXPRESSION_MATCHER.matcher("") in constructor, then matcher.reset(config).replaceAll(...) in resolve method.
Also suggest changing parameter names from config to template, since config is just one possible usage of this expression resolver.
| String var) | ||
| { | ||
| UnaryOperator<String> env = System::getenv; | ||
| return Optional.ofNullable(env.apply(var)).orElse(""); |
There was a problem hiding this comment.
Avoid object allocation of Optional on each call to resolve.
Return null if not found, default to "" is happening at caller ExpressionResolver.
Simplify to return System.getenv(var);.
| { | ||
| configText = expressionResolver.resolve(context, configText); | ||
| } | ||
| ExpressionResolver resolver = ExpressionResolver.instantiate(); |
There was a problem hiding this comment.
Let's move this instantiation to the constructor, so it is done once, even when detecting changed configuration (later).
Suggest calling it expressions so the code reads as expressions.resolve(...).
| public String resolve( | ||
| String var) | ||
| { | ||
| return "PASSWORD".equals(var) ? "ACTUALPASSWORD" : ""; |
There was a problem hiding this comment.
Even though it's just a test implementation, let's return null instead of "" as the defaulting will happen at the ExpressionResolver instead.
fix #91