Fixes #7006: Parameter validation in Rudder should accept rudder variables ( ${ } ) #2942
Conversation
|
PR rebased |
fanf
force-pushed
the
ust_7006/regex_validation_in_rudder_should_accept_rudder_variables
branch
from
3a6740f
to
5637598
Compare
| // here we defined a function to build them, with the possibility to | ||
| val plainString : Parser[CharSeq] = ("""(?iums)((?!(\Q${\E\s*rudder\s*\.|\Q${\E\s*node\s*\.\s*properties)).)+""").r ^^ { CharSeq(_) } | ||
| // here we defined a function to build them | ||
| def plainStringNoDollar[_: P] : P[CharSeq] = P( (!"${" ~ AnyChar).rep(1).! ).map { CharSeq(_) } |
There was a problem hiding this comment.
i'm worried that everything is a def rather than a val: the number of instanciated objects will explode.
There was a problem hiding this comment.
I'm asking on fastparse how to handle that case
There was a problem hiding this comment.
https://gitter.im/lihaoyi/fastparse?at=5ea9e817f0377f163158e8b4
Since fastparse 2.x, parsers no longer have any initialization cost. They are simply methods that you call just like any other methods
| VTypeConstraint.checkIsProperty(value, forField) { v => | ||
| try { | ||
| v.toInt | ||
| Right(()) |
There was a problem hiding this comment.
why is it returning an empty Right ?
There was a problem hiding this comment.
to force the user (me) to map(_ => escaped) for the return value. We don't use these value, the parsing/etc is just done to check if the provided input is of the correct type, but we need to use the escaped string char that is already computed before that method is called
| @@ -52,15 +54,26 @@ case class RegexConstraint(pattern: String, errorMsg: String) { | |||
|
|
|||
| val compiled = Pattern.compile(pattern) | |||
|
|
|||
| val variablePattern = Pattern.compile(".*?\\$\\{.*?\\}.*?") | |||
|
this looks good to me, but I'm not sure i'll be able to understand the parsing part |
|
Fortunately, there is a bunch of test cases. It seems that whoever wrote the first parser had had to deal with a great deal of edge cases. Or he was just a bit paranoid. They helped a lot and caught a lot of problem when adding the |
...r-core/src/main/scala/com/normation/rudder/services/policies/InterpolatedValueCompiler.scala
Outdated
Show resolved
Hide resolved
...r-core/src/main/scala/com/normation/rudder/services/policies/InterpolatedValueCompiler.scala
Outdated
Show resolved
Hide resolved
...r-core/src/main/scala/com/normation/rudder/services/policies/InterpolatedValueCompiler.scala
Show resolved
Hide resolved
...r-core/src/main/scala/com/normation/rudder/services/policies/InterpolatedValueCompiler.scala
Outdated
Show resolved
Hide resolved
|
Also I think you should rename s to space, put long names instead of shortname (ie: sqStr, should be singleQuoteString) |
|
I started a PR of renaming, a removal, also adding some edge cases tests |
|
PR updated with a new commit |
2 similar comments
|
PR updated with a new commit |
|
PR updated with a new commit |
fanf
force-pushed
the
ust_7006/regex_validation_in_rudder_should_accept_rudder_variables
branch
from
f7f3326
to
024d539
Compare
...r-core/src/main/scala/com/normation/rudder/services/policies/InterpolatedValueCompiler.scala
Outdated
Show resolved
Hide resolved
|
PR updated with a new commit |
|
This PR is not mergeable to upper versions. |
|
OK, squash merging this PR |
VinceMacBuche
force-pushed
the
ust_7006/regex_validation_in_rudder_should_accept_rudder_variables
branch
from
8b0de66
to
54504e8
Compare
https://issues.rudder.io/issues/7006
"oups".
It was really too complicated to add non-rudder variable to our compiler with scala lib parser combintor. Since we now have
fastparsein the path, I tried to see if it was easier... And it was thanks to its amazing log possibilities (the parsing by itself is a bit tricky).The most interesting is how little code changes from scala regex combinator to fastparse. And the perf are just in other league (it does not change much in our case, since node prop interpolation is in second range, but now we are in the 10s of millisec - it may still be interesting with group properties and many more parsing to do).
So I also switch the last parser we had (the one for our "search anything" bar queries), what allows to get ride of one dependancy and use the (very cool!) same combinator parser everywhere.