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

Escaping backslash does not work in front of replacement expression #746

Open
vsevel opened this issue Apr 29, 2022 · 12 comments
Open

Escaping backslash does not work in front of replacement expression #746

vsevel opened this issue Apr 29, 2022 · 12 comments
Assignees
Labels
bug Something isn't working spec-related

Comments

@vsevel
Copy link

vsevel commented Apr 29, 2022

I can't find a way to evaluate successfully something like x\${x} to x\foo.

With config:

x: foo
a: x/${x}
b: x\${x}
c: x\${x}
d: x\\${x}
e: x\\\${x}
f: x\\\\${x}

and code:

    @ConfigProperty(name = "x") String x;
    @ConfigProperty(name = "a") String a;
    @ConfigProperty(name = "b") String b;
    @ConfigProperty(name = "c") String c;
    @ConfigProperty(name = "d") String d;
    @ConfigProperty(name = "e") String e;
    @ConfigProperty(name = "f") String f;

    @Test
    public void x() {
        log.info("x=" + x);
        log.info("a=" + a);
        log.info("b=" + b);
        log.info("c=" + c);
        log.info("d=" + d);
        log.info("e=" + e);
        log.info("f=" + f);
    }

We get:

x=foo
a=x/foo
b=x${x}
c=x${x}
d=x\${x}
e=x\\${x}
f=x\\\${x}

The \ seems to escape the $, resulting in ${x} not being evaluated.
But then I can't escape the escape character.

/cc @radcortez

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 29, 2022

This may actually be a bug in smallrye/smallrye-common's expression module string parser.

@radcortez radcortez self-assigned this May 2, 2022
@radcortez
Copy link
Member

For now, you can work around this by adding an expression to the backslash:

endpoint=foo${backslash}${path}
backslash=\\
path=bar

@dmlloyd
Copy link
Contributor

dmlloyd commented May 3, 2022

The expression ${\} should also work to produce a literal backslash. No, that's incorrect; sorry. I'm reading the code and I was in the ESCAPES section when I thought I was somewhere else.

I wonder if the config parser did not set the ESCAPES flag to the parser?

@radcortez
Copy link
Member

The Expression code works correctly and as expected. The main issue was when MP Config added support for Property Expressions; it defined a specific escape sequence to skip the expansion with \\$. If I remember correctly, my original proposal was to just support double $$(which is our way to escape it) but that was rejected by the group.

So, to comply with MP Config I've added this:

* MicroProfile Config defines the backslash escape for dollar to retrieve the raw expression. We don't want to
* turn {@link Expression.Flag#ESCAPES} on because it may break working configurations.
*
* This will replace the expected escape in MicroProfile Config by the escape used in {@link Expression}, a double
* dollar.
*/
private String escapeDollarIfExists(final String value) {
int index = value.indexOf("\\$");
if (index != -1) {
int start = 0;
StringBuilder builder = new StringBuilder();
while (index != -1) {
builder.append(value, start, index).append("$$");
start = index + 2;
index = value.indexOf("\\$", start);
}
builder.append(value.substring(start));
return builder.toString();
}
return value;
}
}

Which turns the \\$ into our espace sequence $$, so that is why you don't get the expansion working in that case.

Turning on the Expression.Flag#ESCAPES introduced other issues, observed here now #747, so we opted out of it to not break users. If I remember correctly, when I tried to turn it on, it was causing issues with the PATH variable.

So at this point, I'm not totally sure what to do. Either we add another escape sequence for the \\$or we ignore MP Config (which may break other users).

@dmlloyd what do you think?

@dmlloyd
Copy link
Contributor

dmlloyd commented May 3, 2022

In this regard (as in many others) I think we can see that MP Config is fundamentally broken. The caution/lesson here (which, as you know, I've stated repeatedly to the Jakarta Config spec group) is that when designing a specification with syntax, the syntax has to be clearly specified - at least more strongly than "let's put in a random solution without thinking too deeply about the potential problems". 😞

The potential problem in this case is that with \$ escaping $ but \ otherwise not being an escape, we've created a syntax issue. How does one express a string that contains the sequence \$?

In the end we need a syntax grammar which allows:

  1. Every possible string to be expressed, somehow, and
  2. The inconsistent/ill-considered syntax rules of MP Config to somehow still work

while hopefully accounting for Jakarta Config. It almost doesn't seem worth it.

@dmlloyd
Copy link
Contributor

dmlloyd commented May 3, 2022

In my opinion I think we're better off having a consistent syntax that we can support, and just documenting the differences with MP Config and giving up on spec compliance altogether (unless the spec in question actually makes sense).

@radcortez
Copy link
Member

I agree. I probably should have opposed this harder, but you know how things work.

I don't think this is used extensively. Of course, there is a chance to break someone with unpredictable results, but I see no other consistent way to do it.

I wouldn't worry about Jakarta Config having to support MP Config syntax. This will be the least of the problems with all the discussions and how the spec is moving in Jakarta Config.

@vsevel
Copy link
Author

vsevel commented May 3, 2022

and giving up on spec compliance altogether (unless the spec in question actually makes sense).

at least temporarily, and hopefully get a way to change the spec.

@radcortez
Copy link
Member

I think we should be able to push a change to MP Config.

@dmlloyd What do you think we propose to change it with the original implementation support of the double $ to escape expansion?

@dmlloyd
Copy link
Contributor

dmlloyd commented May 3, 2022

I'd have to review the syntax rules again before having an informed opinion, but I think it's a reasonable idea. My feeling is that \$ is likely to still be a problem though. We'd need to specify what \\$ and \$$ should do, at the least.

@radcortez
Copy link
Member

Sure.

@radcortez
Copy link
Member

I've found this comment from me:

eclipse/microprofile-config#577 (comment)

I guess that was not enough, but I'm also to blame here 👿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working spec-related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants