Skip to content

Allow ELResolver to be specified in Config #432

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

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Conversation

mattcoley
Copy link
Collaborator

Alternative to #431

This will allow us to connect our custom ELResolver.

@mattcoley mattcoley requested a review from boulter April 16, 2020 21:11
@@ -307,7 +309,10 @@ public Builder withMaxMacroRecursionDepth(int maxMacroRecursionDepth) {
}

public Builder withReadOnlyResolver(boolean readOnlyResolver) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the existing behavior consistent so kept this method in for the builder.

@boulter
Copy link
Contributor

boulter commented Apr 16, 2020

instead of making it part of the config, how about allowing it to be set on the Jinjava instance like it is for ResourceLocator. Then you can create whatever implementation you want.

@mattcoley
Copy link
Collaborator Author

The ELResolver used right now is determined by the config (through the readOnlyResolver setting). Is there any advantage to exposing the ELResolver at the Jinjava level? That would mean you would need a separate instance of Jinjava for every ELResolver you would like to use vs. being able to reuse the same Jinjava object with different ELResolver's specified by the config.

@mattcoley
Copy link
Collaborator Author

I realized I forgot to add the Builder method to the config for specifying your own ELResolver.

@boulter
Copy link
Contributor

boulter commented Apr 17, 2020

I think it should be possible to provide your own chain of resolvers. The readOnlyResolver seems a little too inflexible. Maybe you could just pass your CompositeELResolver to config if you don't want to add a setter to Jinjava.

@mattcoley
Copy link
Collaborator Author

The config accepts any ELResolver so you can pass in your own CompositeELResolver to the config. The readOnlyResolver boolean was an existing config value that I didn't want to remove for backwards compatibility, I just moved the logic to map the boolean value to one of the two default values to the config from the JinjavaInterpreterResolver

@mattcoley mattcoley merged commit 3dde10f into master Apr 17, 2020
@mattcoley mattcoley deleted the elresolver-config branch April 17, 2020 17:56
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.

None yet

2 participants