Use jexl for nocode expressions#2203
Conversation
johnbley
changed the title
| * Please be careful with this, it's directly tied to @Advice.AllArguments. | ||
| * | ||
| * @return | ||
| * @return @Advice.AllArguments - please be careful |
There was a problem hiding this comment.
just curious what concerns did you have to add this warning?
There was a problem hiding this comment.
Jason pointed out that this is a mutable array and users of it should take care to know that it is tied to the given advice instrumentation. #2184 (comment)
There was a problem hiding this comment.
@Advice.AllArguments takes a parameter readOnly that defaults to true. This parameter controls whether method arguments can be mutated using the returned array. Reading #2184 (comment) I think that Jason was just concerned that array is a mutable type. I wouldn't be too concerned with this.
| String evaluate(String expression, Object thiz, Object[] params); | ||
| } | ||
|
|
||
| private static final AtomicReference<Evaluator> globalEvaluator = new AtomicReference<>(); |
There was a problem hiding this comment.
I probably would have used a volatile variable, but AtomicReference is fine too
| context.set("param" + i, params[i]); | ||
| } | ||
| try { | ||
| // could cache the Expression in the Rule if desired |
There was a problem hiding this comment.
I think this would be a good idea. Wouldn't need to parse the expression for each evaluation.
There was a problem hiding this comment.
I'll do a round of profiling for easy wins later, this will likely jump out as a pretty obvious one.
| public final class YamlParser { | ||
| private static final Logger logger = Logger.getLogger(YamlParser.class.getName()); | ||
| // FIXME support method override selection - e.g., with classfile method signature or something | ||
| // FUTURE support method override selection - e.g., with classfile method signature or something |
There was a problem hiding this comment.
One issue with instrumenting methods from sub classes is that we currently can't easily tell from advice code which rule is tied to the method invocation because we can't just look up the rule based on the invoked class and method name. I haven't tried it out yet, but I believe we could make this work by adding an api in the upstream that would let us pass extra data, such as rule id, into the invoked advice method.
There was a problem hiding this comment.
Yes, I've looked at this and though I can't see how yet, there must be a way to get either something passed into the Advice along these lines, or else have the thing that pastes the bytecode into the instrumented method be parameterized per-instance in some way with a ldc.
| return Stream.of( | ||
| // Might be nice to support a bare "param0" or "this" but as a workaround can always use | ||
| // "this.toString()" | ||
| arguments("this", "{key=value}"), |
There was a problem hiding this comment.
an alternative to using expressions would be to use templates (I think this is what they are called in jexl) where you have something like Hello ${this.name}
There was a problem hiding this comment.
Yeah, jexl has a lot of features; I tried to keep things more java-y than expression-y. When this gets more mature/field-tested I'll add docs for common scenarios, the names of the provided variables, etc.
There was a problem hiding this comment.
Unfortunately the behavior is hard to change once this feature is made public so we'll have to consider that we are probably going to be stuck with whatever we come up here for a long time.
… statement-oriented expressions
2 participants
Use jexl (Apache 2.0 license) for nocode expressions. Very little syntax change from the first prototype.