-
Notifications
You must be signed in to change notification settings - Fork 408
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
Substitution in <dyn_variable> for JSONPath #317
Conversation
I'd really like to hear your feedback, @nniclausse. Mainly if this approach is at all desirable or not. And if not, if you have another idea what approach you'd see to realise such a enhancement. |
@nniclausse ping. I'd really like your feedback. We're using the current patch for some time now and so far it did work just fine. My idea would be to merge this now, and create follow-up issues that we also apply substitutions to other dyn var types. They might have a perf impact, as regular expressions and xpaths will require recompilation. But IMO this is okay, if we document this properly so that the users are aware of this. |
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.
I'm OK with the general idea. Appart from the small typo, the current PR lacks some documentation and maybe some unit tests ?
src/tsung_controller/ts_config.erl
Outdated
@@ -655,6 +655,14 @@ parse(#xmlElement{name=dyn_variable, attributes=Attrs}, | |||
?LOGF("Add new xpath: ~s ~n", [Expr],?INFO), | |||
CompiledXPathExp = mochiweb_xpath:compile_xpath(FlattenExpr), | |||
{xpath,Name,CompiledXPathExp}; | |||
jsonpath -> | |||
?LOGF("Add new jsonpath: ~s ~n", [Expr],?INFO), | |||
EnableSubstition = case {SubstitutionFlag, re:run(Expr, "%%.+%%")} of |
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.
small typo to fix here: should be EnableSubstitution
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.
fixed
okay great. I'll take a look at how to test this and will add some remarks to the documentation. |
Do you mind to take another look, @nniclausse? I added some tests and a paragraph to the JSONpath documentation. I'm not sure how to go about mentioning unreleased versions. Is it okay to just assume that this will be in 1.8.0? |
Yes, that's ok, thanks ! |
Proposal for #316
This PR propagates the substitution flag from
<request subst="…">
into the dynvar specification used ints_search:parse_dynvar
. NOTE Onlyjsonpath
has been adjusted currently as they don't need to compile. The performance impact should be minimal.It was necessary to pass the current
DynVars
intots_search:parse_dynvar
so the arity was changed from 2 to 3. For compatibility reasons (tests mainly)ts_search:parse_dynvar/2
is now also present with the same behaviour as before.ts_search:parse_dynvar/3
is now used byts_client:update_stats
and it is the main point of entry.This PR basically allows us to have this working, which works a lot more as expected to the user:
Note, that the substitution is only performed, when the
subst
attribute is set totrue
and we detect a variable inside the expression (%%*%%
).Example
JSON document returned by https://httpbin.org/json:
Will produce:
How to proceed?