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

#Fixed 198 - Add interceptor to expand expressions in Config Values. #266

Merged
merged 3 commits into from Mar 20, 2020

Conversation

radcortez
Copy link
Member

No description provided.

Copy link
Contributor

@dmlloyd dmlloyd left a 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!

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 18, 2020

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.

@radcortez
Copy link
Member Author

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?

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 18, 2020

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.

@radcortez
Copy link
Member Author

radcortez commented Mar 18, 2020

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.

  • Should we add a getValue(String name, boolean expand)?
  • Or some sort of marker in the key / value that signals to skip expansion?

@radcortez
Copy link
Member Author

Or even better: add a new method to ConfigValue to retrieve the original value without expansion?

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 18, 2020

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; });

@radcortez
Copy link
Member Author

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?

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 18, 2020

That could be generally true of any feature provided by an interceptor, really...

@radcortez
Copy link
Member Author

radcortez commented Mar 18, 2020

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.

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 18, 2020

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.

@radcortez
Copy link
Member Author

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.

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 18, 2020

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.

@radcortez
Copy link
Member Author

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.

@radcortez radcortez merged commit 56a511c into smallrye:master Mar 20, 2020
@radcortez radcortez deleted the #198 branch April 2, 2020 11:38
@radcortez radcortez added this to the 1.7.1 milestone Apr 16, 2020
@radcortez radcortez linked an issue Apr 16, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Interceptor Mechanism for Resolving
2 participants