Quarkus: CORSConfig is not representable as YAML

[gwenneg]

As seen during #4525 development, CORSConfig cannot be represented as YAML because we'd need the quarkus.http.cors key to be both the key of a key/value pair and the key of a dictionary, which is not possible.

A properties-based CORS filter configuration currently looks like this:

quarkus.http.cors=true
quarkus.http.cors.origins=http://foo.com,http://www.bar.io
quarkus.http.cors.methods=GET,PUT,POST
quarkus.http.cors.headers=X-Custom
quarkus.http.cors.exposed-headers=Content-Disposition
quarkus.http.cors.access-control-max-age=24H

We only need to change the quarkus.http.cors=true property to quarkus.http.cors.enabled=true to make it convertible to YAML.

[gsmet]

As discussed on Zulip, +1 for this.

[machi1990]

@gwenneg Hi, is this issue only for the CORSConfig? or for all config items inheriting parent's name with

@ConfigItem(name = ConfigItem.PARENT)

?

[gsmet]

I think the issue is related to having something like:
quarkus.extension.foo = ...
quarkus.extension.foo.bar = ...

On Tue, Oct 15, 2019 at 6:20 PM Manyanda Chitimbo [e-mail]
wrote:

@gwenneg https://github.com/gwenneg Hi, is this issue only for the
CORSConfig? or for all config items inheriting parent's name with

@ConfigItem(name = ConfigItem.PARENT)

?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/quarkusio/quarkus/issues/4568?email_source=notifications&email_token=AAJYOBOPSU2KHLHPIDNFFLDQOXUVHA5CNFSM4JA3ALIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBJMAWY#issuecomment-542294107,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAJYOBNI2XKZ5ZL5V33Z6P3QOXUVHANCNFSM4JA3ALIA
.

[gwenneg]

Yes, that's a good point @machi1990.

I was planning on checking all config classes while fixing CORSConfig :)

[machi1990]

Great, thanks @gwenneg

We have the test-extension with possible configuration values - https://github.com/quarkusio/quarkus/tree/master/core/test-extension/runtime/src/main/java/io/quarkus/extest/runtime/config with a rather large test suites here - https://github.com/quarkusio/quarkus/tree/master/core/test-extension/deployment/src/test/java/io/quarkus/extest

not sure if we must have the same coverage with a yaml file, but the extension is handy when you want to test various things, config among others :-)

HTH.

[gwenneg]

Here's the complete list of config keys that are currently not representable as YAML, based on the ALL CONFIGURATION OPTIONS doc page (I'm glad it existed to do this check!):

quarkus.hibernate-orm.database.generation
quarkus.hibernate-orm.database.generation.halt-on-error

^ quarkus.hibernate-orm.database.generation could be replaced with quarkus.hibernate-orm.database.generation.auto to match the original Hibernate property. @gsmet WDYT?

quarkus.hibernate-orm.dialect
quarkus.hibernate-orm.dialect.storage-engine

^ quarkus.hibernate-orm.dialect could be replaced with quarkus.hibernate-orm.dialect.name but it differs from the original Hibernate property. @gsmet WDYT?

quarkus.http.cors
quarkus.http.cors.origins
quarkus.http.cors.methods
quarkus.http.cors.headers
quarkus.http.cors.exposed-headers
quarkus.http.cors.access-control-max-age

^ quarkus.http.cors will become quarkus.http.cors.enabled

quarkus.log.console.async
quarkus.log.console.async.overflow
quarkus.log.console.async.queue-length

^ quarkus.log.console.async could be replaced with quarkus.log.console.async.enabled

quarkus.log.file.async
quarkus.log.file.async.overflow
quarkus.log.file.async.queue-length

^ quarkus.log.file.async could be replaced with quarkus.log.file.async.enabled

quarkus.log.syslog.async
quarkus.log.syslog.async.overflow
quarkus.log.syslog.async.queue-length

^ quarkus.log.syslog.async could be replaced with quarkus.log.syslog.async.enabled

I also noticed several configuration keys being used by quarkus-agroal, quarkus-reactive-mysql-client and quarkus-reactive-pg-client. I'm not sure that's an issue though, but I'd like your opinion about it:

quarkus.datasource.max-size
quarkus.datasource.password
quarkus.datasource.url
quarkus.datasource.username

I'll see if that kind of issue can be detected at build time, but I'm not sure it'll be easy. Half of these cases were related to the use of @ConfigItem(name = ConfigItem.PARENT) and the other half was the consequence of hardcoded config keys names using @ConfigItem(name = "foo").

[dmlloyd]

I understand the problem with the ConfigItem.PARENT items, but why are the hardcoded config key names a problem?

[dmlloyd]

And for PARENT, what about having a special key for the parent value like _ or *? Or, does YAML support empty keys?

[gwenneg]

I just realized AsyncConfig.java#L14 is the cause of the three last cases. Removing ConfigItem.PARENT would add .enable (should it be .enabled instead?) at the end of quarkus.log.file.async, quarkus.log.syslog.async and quarkus.log.syslog.async.

Regarding _ or *, are you suggesting this could be used in a YAML file as the key of a config value?

There's something about empty keys in the YAML spec, but I've never tested it with Java.

Hardcoded config keys are a problem because they can lead to the same key base being shared by config attributes that were originally named differently. There's a good example here.

[dmlloyd]

Regarding _ or *, are you suggesting this could be used in a YAML file as the key of a config value?

Right. Rather than upend the entire configuration system to accommodate YAML, maybe we can hack YAML a bit to accommodate the config system. So if I have foo.bar and foo.bar.baz, then in the YAML maybe in addition to looking for a bar inside of a foo, we should look for a * within a bar within a foo.

There's something about empty keys in the YAML spec, but I've never tested it with Java.

Ah well empty keys are probably the exact right thing to use in this case since it's in the spec, presumably for this exact purpose.

Hardcoded config keys are a problem because they can lead to the same key base being shared by config attributes that were originally named differently. There's a good example here.

This would be solved as well by allowing an empty key within dialect to be used for dialect.

[gwenneg]

I like the idea. I guess this would have to be done in the YamlConfigSource that is currently being donated from micrprofile-extensions to smallrye-config, as discussed in #4525.

I'll give empty YAML keys a try with #4525 as it is currently using the same YAML parser (snakeyaml) than the future smallrye-config version. It might already support empty keys.

[gsmet]

What would be the resulting YAML file? What I understand from this makes me think it won't be very user friendly. So I might misunderstand the end result. Could someone give a YAML example?

[gwenneg]

@gsmet I will test empty keys tonight and post a YAML example here as soon as I have something working.

It'll probably be something like:

quarkus:
  hibernate-orm:
    dialect:
      null: MySQL5InnoDBDialect
      storage-engine: InnoDB

Which is not very user-friendly indeed.

It also seems ~: MySQL5InnoDBDialect is another way of writing null: MySQL5InnoDBDialect.

[dmlloyd]

It's YAML; I don't think it's our mission to make spec-conformant YAML be user-friendly. It should be idiomatic, and therefore intuitive to YAMLphiles, but other than that I don't think we ought to be harming ourselves trying to accommodate it.

Maybe I misunderstood the spec but I thought that a null key was represented by literal nothing? e.g.

quarkus:
  hibernate-orm:
    dialect:
      : MySQL5InnoDBDialect
      storage-engine: InnoDB

I'm not sure I read it correctly as the spec language is very terse.

[gwenneg]

I'm not sure if the null or ~ key either. I'll test each possibility, including : MySQL5InnoDBDialect.

[dmlloyd]

You appear to be correct: testing with libyaml indicates that ~ maps to an empty key, as does null. A literal empty key results in an error.

[gsmet]

IMHO, it all comes down to: do we want YAML to be a first class citizen or not?

If we do then I think the format should be straightforward in YAML too and it's worth changing our configurations properties.

@emmanuelbernard WDYT?

[dmlloyd]

No. :)

We can't have everything be a first class citizen. But anyway it is straightforward in YAML since there's a predefined, idiomatic mechanism for exactly this, so it doesn't matter: YAML gets first class treatment just coincidentally.

Essentially we treat any construct like:

foo:
   ~: bar

To be equivalent to:

foo: bar

And that's how it'll go into SmallRye, unless there's a valid objection.

[emmanuelbernard]

Ah that sucks a bit... Not sure what to do, have a .class sub key or keep prioritizing the dialect + dialect.subkey model.

Do we use this model often? Vs always setting properties on leaves only?

[dmlloyd]

We do, and more importantly, MP-Config allows it (it's not really a Quarkus question). So the YAML mapping for MP-Config (which is what we're really talking about here) must also allow it. And if it must allow it, then it should use the idiomatic YAML approach, whether or not we as individuals think it's good or it sucks: whoever wrote the YAML spec seems to like this approach at any rate. So, the conclusion seems clear to me.

[gwenneg]

I tested empty YAML keys with #4525 and snakeyaml (which is also the YAML parser in the future SmallRye YAML config source unless it's replaced with something else).

Both null:true and ~:true on the YAML side were parsed to a Java map entry with a null key and a true value. However, :true (omitting the YAML key) was not valid and could not be parsed.

As @dmlloyd wrote here, empty YAML keys will have to be treated in SmallRye and when it's done, the initial topic of this issue won't be a problem anymore.

[gsmet]

I must admit that having some standard Yaml parser not supporting the Yaml syntax we need is a bit concerning.

So IMHO snakeyaml needs fixing to support the syntax (if it's a supported Yaml syntax) but having workarounds on our side is really something I would like to avoid.

[dmlloyd]

SnakeYaml seems to work fine; the problem is really in our reading of the spec. I think that ~ as a key is correct and supported.

[manodupont]

Can i have the solution for this ?

is ~: actually fixing it ?

[dmlloyd]

Yes, use ~ for this case.