-
Notifications
You must be signed in to change notification settings - Fork 117
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
#Fixed 198 - Add interceptor to expand expressions in Config Values. #266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, just one comment!
implementation/src/test/java/io/smallrye/config/ExpressionConfigSourceInterceptorTest.java
Show resolved
Hide resolved
Actually a second comment. Quarkus needs the ability to disable expansion for its configuration reading code, so we need to carry something over to enable that. |
Doesn't Quarkus bootstraps different instances of SmallRyeConfig with the Builder? If so, we could achieve this by just registering the required interceptors. For Quarkus configs, we can leave the Expression interceptor out. Would that work? |
Yes there are different instances, but we toggle expansion on a per-property basis as the configuration is being read. Some of the properties have to be preserved unexpanded for delivery to the next stage of execution. |
Ok, I see that in Quarkus, that is done with a ThreadLocal. Do we want to do the same here? It is somehow tricky to do it on a per-property basis unless we change the API.
|
Or even better: add a new method to |
Yeah I think a thread-local is totally reasonable. The API for Quarkus is unsophisticated but effective, and it doesn't leak the details of property expansion to any consumer that doesn't care about it. turn off expansion;
try {
do things;
} finally {
restore expansion to previous setting;
} For additional safety it could be done like this: withExpansionTurnedOff(() -> { do things; }); |
Well, if we thing more generally, maybe other developers or consumer don't want expansion on all properties as well. In that case, we need to provide a more friendly way to enable / disable? |
That could be generally true of any feature provided by an interceptor, really... |
Hehe sure. So you prefer the ThreadLocal approach? Since Quarkus uses SmallRye Config directly, I think we could add the API to retrieve an expanded variable or not, or just expose the accessor we are creating. The goal in the end to have it exposed in Config, so you could either retrieve the original value or the expanded value through ConfigValue. |
I think the thread local/static approach is best (maybe for upstream too). This way we don't have to explode the Config or ConfigValue API for every possible thing that the user might want to toggle, which could get out of hand quickly. Using a separated API allows for better separation of concerns AFAICT. |
Well that can also be implemented in Quarkus only by its own interceptor to be applied before this one and skipping the chain :) I think it makes sense for ConfigValue to also hold the original expression. This will allow the user to build custom runtime expressions. Maybe we could support both. We can add the TheadLocal option, but provide a more friendly way for developers to access values. In fact, we may want to turn ConfigValue into an interface and then anyone is free to build their custom objects with additional metadata if they need. |
Wouldn't we then need combinatorial versions of all the possible ConfigValue implementations? What would it end up looking like if there were 4 or 5 interceptors that the user wanted to selectively control? Would we end up with chains of ConfigValue implementations that have to be unwrapped? This is the kind of thing I worry about. If ConfigValue is a class - preferably final - then it will always only be exactly what it is, and users can control cross-cutting behaviors in an independent way in any combination. It's future-proof; any number of additional interceptor behaviors can be added without affecting the basic contract of ConfigValue or even the core API in any way. |
Not exactly. You know that the each interceptor just returns a ConfigValue that obeys to a specific contract. As long as the contract is respected (by implementing the interface), then you only return an implementation of it with each interceptor call. You can chain and return the one you got from the context, or return your own. |
implementation/src/test/java/io/smallrye/config/ExpressionConfigSourceInterceptorTest.java
Show resolved
Hide resolved
…not able to resolve expansion.
No description provided.