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

Custom operator can't use external context #497

Closed
vlalykin opened this issue Feb 5, 2020 · 11 comments · Fixed by #499
Closed

Custom operator can't use external context #497

vlalykin opened this issue Feb 5, 2020 · 11 comments · Fixed by #499
Milestone

Comments

@vlalykin
Copy link
Contributor

vlalykin commented Feb 5, 2020

I register a new operation "LIKE":

public class MyExtension extends AbstractExtension {
    @Override
    public List<BinaryOperator> getBinaryOperators() {
        return Arrays.asList(
                new BinaryOperatorImpl("like", 20, LikeExpression.class, Associativity.LEFT)
        );
    }
}

I can only specify the class name LikeExpression. Its instance is created by the engine and there is no good way to inject some necessary data into the class instance. Filters have no such problem.

Now I see such context injection methods:

  1. through a static variable of some class
  2. through the context of template variables
  3. when using spring extension - through SpringApplicationContext ("beans" variable). Similar to point (2), only the path to the goal is even longer

I suggest registering not a class, but a factory (or provider) of class instances:

LikeExpressionInstanceFactory factory = new LikeExpressionInstanceFactory( ... ); /* the desired context is passed here */
new BinaryOperatorImpl("like", 20, factory, Associativity.LEFT);

These two methods can exist together without breaking backward compatibility. Registering a class can use a lambda factory:

(clazz) -> clazz.newInstance ();
@vlalykin
Copy link
Contributor Author

vlalykin commented Feb 5, 2020

if you don't mind, I am ready to do PR

@ebussieres
Copy link
Member

What data do you need to inject in LikeExpression class ?

@vlalykin
Copy link
Contributor Author

vlalykin commented Feb 7, 2020

cache instance for storing compiled regular expressions

@cjbrooks12
Copy link
Contributor

I'd imagine this would improve performance slightly, too, as it would eliminate the need for reflection when evaluating operators

@ebussieres
Copy link
Member

We can just change the BinaryOperatorImpl constructor. Instead of getting a

Class<? extends BinaryExpression<?>> nodeClass,

We should directly pass the instance of the class.

BinaryExpression<?> nodeClass,

We should do the same modification in UnaryOperatorImpl too

@vlalykin
Copy link
Contributor Author

vlalykin commented Feb 7, 2020

no, the parser every time creates a new instance of the class for each node in the tree

@vlalykin
Copy link
Contributor Author

vlalykin commented Feb 7, 2020

and initializes a new class (leftExpression, rightExpression, lineNumber)

@ebussieres
Copy link
Member

My bad ... you're right

@vlalykin
Copy link
Contributor Author

vlalykin commented Feb 7, 2020

and the class itself is used as a marker to distinguish the filter expression and test expression from the rest of the expressions. I think this is not good.

it’s all easy to fix, you can pass the expression type to the constructor (for example, enum)

@vlalykin
Copy link
Contributor Author

vlalykin commented Feb 28, 2020

@ebussieres
Hi!
I created PR.
Have a look, please.

@vlalykin
Copy link
Contributor Author

vlalykin commented Mar 6, 2020

I used Java 8 syntax. Maybe I'm wrong? I did not find in the documentation whether pebble supports Java 7.
If compilation on Java 7 is required, let me know, please

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 a pull request may close this issue.

3 participants