Trailing whitespace after quarkus.jaeger.sampler-param=1 fails to build

[jamesfalkner]

Describe the bug
Configured for OpenTracing using:

quarkus.jaeger.sampler-param=1

(notice the whitespace after the 1), it fails to build with:

[ERROR] Failed to execute goal io.quarkus:quarkus-maven-plugin:0.27.0:build (default) on project people: Failed to build a runnable JAR: Failed to augment application classes: java.lang.reflect.InvocationTargetException: NumberFormatException -> [Help 1]

Expected behavior
No errors

Actual behavior
Errors (see above)

To Reproduce
Steps to reproduce the behavior:

1. Create a new project and include the opentracing extension:

    -DprojectGroupId=org.acme \
    -DprojectArtifactId=fruit-taster \
    -DclassName="org.acme.quickstart.GreetingResource" \
    -Dextensions="opentracing" \
    -Dpath="/hello"

2. Add configuration:

quarkus.jaeger.service-name=people
quarkus.jaeger.sampler-type=const
quarkus.jaeger.sampler-param=1 
quarkus.jaeger.endpoint=http://jaeger-collector:14268/api/traces

(add space after the 1)

3. mvn clean package -DskipTests

Configuration

# Add your application.properties here, if applicable.
quarkus.jaeger.service-name=people
quarkus.jaeger.sampler-type=const
quarkus.jaeger.sampler-param=1 
quarkus.jaeger.endpoint=http://jaeger-collector:14268/api/traces

Environment (please complete the following information):

• Output of uname -a or ver:
  Darwin jfalkner-OSX 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
• Output of java -version:

java version "1.8.0_201"
Java(TM) SE Runtime Environment (build 1.8.0_201-b09)
Java HotSpot(TM) 64-Bit Server VM (build 25.201-b09, mixed mode)

• GraalVM version (if different from Java):
  N/A
• Quarkus version or git rev:
  0.27.0

[geoand]

Let me take a quick look at this and if I can come up with a quick win

[geoand]

This happens because the trailing whitespace trips the BigDecimal constructor:

Caused by: java.lang.NumberFormatException
    at java.math.BigDecimal.<init> (BigDecimal.java:497)
    at java.math.BigDecimal.<init> (BigDecimal.java:383)
    at java.math.BigDecimal.<init> (BigDecimal.java:809)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0 (Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance (NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance (DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance (Constructor.java:423)
    at io.smallrye.config.ImplicitConverters$ConstructorConverter.convert (ImplicitConverters.java:168)
    at io.smallrye.config.SmallRyeConfig.getOptionalValue (SmallRyeConfig.java:139)
    at io.smallrye.config.SmallRyeConfig.getOptionalValue (SmallRyeConfig.java:131)
    at io.quarkus.runtime.configuration.ConfigUtils.getOptionalValue (ConfigUtils.java:92)
    at io.quarkus.deployment.configuration.OptionalObjectConfigType.acceptConfigurationValueIntoGroup (OptionalObjectConfigType.java:81)

@kenfinnigan @dmlloyd what the final resolution for the whitespace handling?

Note that was able to reproduce this with both 0.27.0 and master.

[kenfinnigan]

we've removed whitespace trimming, but it seems that this was an issue even with trimming.

One option is to switch the config to be a String, and then just valid what's entered is a valid number before the recorder sets the system property?

[geoand]

@kenfinnigan That is certainly possible but I am curious as to why this actually fails.

[kenfinnigan]

Because the ImplicitConverter is trying to convert the string to a BigDecimal

[dmlloyd]

Why is this a bug? If you've added whitespace then it's not a valid number anymore.

[kenfinnigan]

I think the issue is that a nicer error message would be better

[geoand]

So it's expected then. We'll need a better error message then

[dmlloyd]

Ah well if that's the problem, it's already rolled into my config mega-patch, so I'll just add this bug ID to it.

[kenfinnigan]

lol, what's that, now 45 issues resolved in the PR?

[dmlloyd]

18 by last count. :)

[jamesfalkner]

Why is this a bug? If you've added whitespace then it's not a valid number anymore.

Technically that's correct, but in my mind (and I suspect many developers) in a properties file 1 is the same as 1<whitespace> at the end of a property. A nicer error message would be good.

[dmlloyd]

That can't be changed without changing the MP Config specification, or else abandoning spec compliance. Both are options but we'd have to commit to one or the other.

[jamesfalkner]

Yeah.. that's understandable.

[gsmet]

@dmlloyd @kenfinnigan wouldn't it be possible to have a couple of SmallRye converters more lenient without breaking the spec/TCK tests?

For everything number, the current behavior is a bit pedantic and doesn't bring much.

[kenfinnigan]

I don't think so.

As soon as you have a converter that's more lenient, the TCK test would pick it up and use it

[jamesfalkner]

Not sure if relevant, but the same whitespace corrupts the values for JWT issuer values. e.g.

mp.jwt.verify.issuer=some_issuer[whitespace here]

causes the whitespace to be part of the expected issuer values when validating tokens.

[dmlloyd]

That would be expected behavior per the specification.

[gsmet]

We are now more lenient for the converters for which it makes sense.