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

null keys in YAML config break JSON compatibility #34292

Closed
tsprlng opened this issue Jun 25, 2023 · 3 comments · Fixed by #35246
Closed

null keys in YAML config break JSON compatibility #34292

tsprlng opened this issue Jun 25, 2023 · 3 comments · Fixed by #35246
Assignees
Labels
area/config kind/enhancement New feature or request
Milestone

Comments

@tsprlng
Copy link

tsprlng commented Jun 25, 2023

Description

I'm not sure how common this is in general, though I doubt I'm the only platform engineer on Earth who does this: when deploying applications which accept YAML configuration, I quite often feed in the configuration as JSON -- which is also always valid YAML (with yes, pedantically, a few minor exceptions that don't normally matter in real life as long as you use UTF-8).

(Why? In short, generally it's less ambiguous and is compatible with more tools. More convenient to canonicalize (RFC 8785), and therefore to validate. Particularly in a microservices environment, using a tool to manage and generate config files is fairly standard, and I just tend to find that JSON makes that easier.)

This normally works very well with most applications, but unfortunately in Quarkus there is a design element that makes this difficult. Let's take this example from the guide: https://quarkus.io/guides/config-yaml#configuration-key-conflicts

In the underlying properties representation we have quarkus.http.cors=true and quarkus.http.cors.methods="GET,PUT,POST".

When we attempt to represent these as YAML this is of course impossible, as the config value at ['quarkus']['http']['cors'] can't simultaneously be true and {"methods": "GET,PUT,POST"}.

There is a hack in the YAML representation to work around this, where instead the true value is placed under ['quarkus']['http']['cors'][null]. Unfortunately, while all JSON is valid YAML, the reverse is not true. This YAML is unrepresentable in JSON -- all keys must be strings, so null is impossible to express.

Even with natively YAML-based tooling, if any kind of schema or type system is used, the use of null as a key always stands out as an awkward exception.

In an application which parsed YAML natively as its config format, this would likely never have become a problem in the first place -- the design would simply use non-overlapping keys and call the first property quarkus.http.cors.enabled instead of the unrepresentable quarkus.http.cors.

(I'm not trying here to get into whether the methods property in the same example should really be an array. Clearly people find the Java properties representation valuable, and clearly some compromises have to be made to be compatible with both formats.)

Implementation ideas

What I would like to suggest, though, is that a change could be considered to allow the more compatible key quarkus.http.cors.enabled either as a replacement or an alias for the unrepresentable quarkus.http.cors key (along with similar aliases for any other keys with the same "overlapping" problem), so that the YAML config representation remains compatible with JSON.

As this is not really a "bug", though I do think it's a surprising and counterintuitive design, I've raised this as an improvement.

To be fair I should say we do have a workaround in place already, which is to put the quarkus.http.cors=true in the built-in application.yaml, separately from the other runtime config which is injected by the config system. This does seem to work -- the two files do seem to merge correctly, at least in the current version -- so this is hardly a critical issue for us. It would definitely be much nicer not to have to do this, though.

Thanks for reading 🙇

@geoand
Copy link
Contributor

geoand commented Jun 26, 2023

cc @radcortez

@radcortez
Copy link
Member

I'm in favor of relocating the quarkus.http.cors to quarkus.http.cors.enabled. This seems to be more consistent with other extensions where we already have .enabled flags and does remove some of the confusion about how to set this property in YAML, which we have to document explicitly here: https://quarkus.io/guides/config-yaml#configuration-key-conflicts

@geoand
Copy link
Contributor

geoand commented Jun 26, 2023

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants