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

Fixes #12616: ${const.dollar} in generic method parameter leads to missing report #1930

Conversation

fanf
Copy link
Member

@fanf fanf commented May 14, 2018

@fanf
Copy link
Member Author

fanf commented May 15, 2018

PR rebased

@fanf fanf force-pushed the bug_12616/const_dollar_in_generic_method_parameter_leads_to_missing_report branch from 54cec38 to a65cda4 Compare May 15, 2018 00:02
final def replaceCFEngineVars(x : String) : String = {
x.replaceAll(replaceCFEngineVars, ".*").replaceAll("""\\""", """\\\\""")
final def replaceCFEngineVars(x : String) : Pattern = {
Pattern.compile("""\Q"""+ x.replaceAll("""\\""", """\\\\""").replaceAll(replaceCFEngineVars, """\\E.*\\Q""") + """\E""")
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the goal of the "\" => "\\" here. Is it because CFEngine change "" into "\"?
If so, why don't we do it always (not only when the expression contains a cfengine var), because here, only one of the test on "\" passe.

, reports = Repaired("yx\\\\x}") :: Nil
)

test("""escape '\' into '\\' *without* cfengine vars"""
Copy link
Member Author

Choose a reason for hiding this comment

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

This does not passe (the other one above does pass).

@fanf
Copy link
Member Author

fanf commented May 15, 2018

PR rebased

@fanf fanf force-pushed the bug_12616/const_dollar_in_generic_method_parameter_leads_to_missing_report branch from a65cda4 to 3b04655 Compare May 15, 2018 09:31
*/
final def replaceCFEngineVars(x : String) : String = {
x.replaceAll(replaceCFEngineVars, ".*").replaceAll("""\\""", """\\\\""")
Copy link
Member Author

Choose a reason for hiding this comment

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

The replace all seems to have been here to avoid having "\x" be interpreted as a regex predifined character classes. Now that we correctly quote the regex, we don't need it anymore.

* Returns a string that is suitable for a being used as a regexp
* Returns a string that is suitable for a being used as a regexp, with anything not
* in ".*" quoted with \Q...\E.
* For example, "${foo}(bar)$(baz)foo" => ".*\Q(bar)\E.*\Qfoo\E"
Copy link
Member

Choose a reason for hiding this comment

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

are you missing the first Q/E in your example ? I think it hsould be: "\Q\E ".\Q(bar)\E.\Qfoo\E"

Copy link
Member Author

Choose a reason for hiding this comment

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

yes! Sorry

@fanf
Copy link
Member Author

fanf commented May 15, 2018

PR rebased

@fanf fanf force-pushed the bug_12616/const_dollar_in_generic_method_parameter_leads_to_missing_report branch from 3b04655 to 97a9e63 Compare May 15, 2018 13:42
@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/1930
-- Your faithful QA

@fanf
Copy link
Member Author

fanf commented May 15, 2018

OK, merging this PR

@fanf fanf merged commit 97a9e63 into Normation:branches/rudder/4.1 May 15, 2018
@fanf fanf deleted the bug_12616/const_dollar_in_generic_method_parameter_leads_to_missing_report branch March 15, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants