smallrye-config icon indicating copy to clipboard operation
smallrye-config copied to clipboard

Escaping backslash does not work in front of replacement expression

Open vsevel opened this issue 3 years ago • 12 comments

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

vsevel avatar Apr 29 '22 07:04 vsevel

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

dmlloyd avatar Apr 29 '22 16:04 dmlloyd

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

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

radcortez avatar May 02 '22 17:05 radcortez

~~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?

dmlloyd avatar May 03 '22 13:05 dmlloyd

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:

https://github.com/smallrye/smallrye-config/blob/9fdb499e168b46f6031b8af7b8fef309d8fc6101/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java#L71-L92

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 https://github.com/smallrye/smallrye-config/pull/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?

radcortez avatar May 03 '22 15:05 radcortez

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 avatar May 03 '22 15:05 dmlloyd

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).

dmlloyd avatar May 03 '22 15:05 dmlloyd

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.

radcortez avatar May 03 '22 15:05 radcortez

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.

vsevel avatar May 03 '22 15:05 vsevel

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?

radcortez avatar May 03 '22 15:05 radcortez

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.

dmlloyd avatar May 03 '22 16:05 dmlloyd

Sure.

radcortez avatar May 03 '22 16:05 radcortez

I've found this comment from me:

https://github.com/eclipse/microprofile-config/pull/577#issuecomment-659502523

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

radcortez avatar May 05 '22 00:05 radcortez