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

test using dev service with named configuration fails with application.yaml #735

Open
vsevel opened this issue Feb 12, 2025 · 21 comments · May be fixed by #744
Open

test using dev service with named configuration fails with application.yaml #735

vsevel opened this issue Feb 12, 2025 · 21 comments · May be fixed by #744
Assignees
Labels

Comments

@vsevel
Copy link

vsevel commented Feb 12, 2025

When running tests with devservice using a named configuration in an application.yaml, the url fails to be set.
When instead using an application.properties it succeeds.
Here is a reproducer: https://github.com/vsevel/reproducer_artemis_dev_service_config_yaml
cc @zhfeng

@zhfeng
Copy link
Contributor

zhfeng commented Feb 12, 2025

@vsevel Thanks for the reporting! For a quick look, I think it might be an issue with Quarkus since the properties file is working.

@turing85
Copy link
Contributor

@vsevel Thanks for the reporting! For a quick look, I think it might be an issue with Quarkus since the properties file is working.

@zhfeng do you tackle this issue/forward it to https://github.com/quarkusio/quarkus?

@turing85 turing85 added the bug Something isn't working label Feb 12, 2025
@vsevel
Copy link
Author

vsevel commented Feb 12, 2025

what do you think @radcortez ?

@radcortez
Copy link

Has this worked before? Or is it new?

I can have a look.

@vsevel
Copy link
Author

vsevel commented Feb 12, 2025

Has this worked before? Or is it new?

no idea

@turing85
Copy link
Contributor

Has this worked before? Or is it new?

I can have a look.

@radcortez I did a quick test. It did not work with quarkus 3.8.1 and quarkus-artemis 3.2.0. So if it worked at some point, it stopped working a long while back.

@vsevel
Copy link
Author

vsevel commented Feb 18, 2025

should this be transferred to quarkus?

@turing85
Copy link
Contributor

@radcortez ☝ ?

@radcortez
Copy link

Sorry, I had a look yesterday, and before adding something here, I had to focus on higher-priority stuff.

The issue here is that the Dev Service configuration is being generated as quarkus.artemis."%s".url, while the YAML key is quarkus.artemis.foo.url, without the quotes.

Even if the quotes are present in the YAML file, they are removed by the parser, meaning that we have no idea if a segment should be quoted or not, and by default, single segments are left unquoted. When we populate a Map, the current rule is to use a single format to do the lookups, usually the first format found when iterating the property names. We force this to avoid having to query all combinations of quoted and unquoted. It may look simple to issue another lookup, but things can get out of hand if you have nested maps, which would double the lookups again and again and again.

I'm not sure if there is something I can do from the Config side. The ConfigSources are very low-level and have no idea about the mapping of a key (so we don't know there is a Map there). The YAML parser omits the quotes.

I believe the best option is to adjust QUARKUS_ARTEMIS_NAMED_URL_TEMPLATE and use quotes if the user set configuration with quotes, or omit the quotes if the user didn't set them.

BTW, if the name is a multi-segment path, like foo.bar, it works as expected; in that case, we know in the YAMLSource that the key is composed, and we add the quotes manually.

@vsevel
Copy link
Author

vsevel commented Feb 19, 2025

thanks for this analysis @radcortez . seems more complex than at first sight ;-)

Even if the quotes are present in the YAML file, they are removed by the parser

should there be a syntax, which would not be removed by the parser, that would allow to handle this use case without ambiguity?

I believe the best option is to adjust QUARKUS_ARTEMIS_NAMED_URL_TEMPLATE and use quotes if the user set configuration with quotes, or omit the quotes if the user didn't set them.

so that would be a change in the extension itself?

@radcortez
Copy link

should there be a syntax, which would not be removed by the parser, that would allow to handle this use case without ambiguity?

Yes, you can use single quotes and then double quotes as '"foo"'

so that would be a change in the extension itself?

Yes, ideally, the extension should check the configuration name flavour and generate the properties with the same flavour. While it is possible to use the '"foo"' syntax in YAML, each source would have its own way to deal with this, and the user would have to be aware of all combinations.

One recommendation is that when dynamic keys are known at build time, we can use @WithKeys in the runtime path and supply the list of keys. Instead of looking for the keys in the list of property names (which may not even be listed, in the case of the Vault source), SmallRye Config will directly look up the provided keys in @WithKeys. This is what we do now with the REST Client extension: https://github.com/quarkusio/quarkus/blob/de0e007d6c4b9d9495cf2cd0ce8a8bf312908651/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/AbstractRestClientConfigBuilder.java#L25-L431

@vsevel
Copy link
Author

vsevel commented Feb 20, 2025

I confirm this is working:

quarkus:
  artemis:
    '"cf-in"':
      enabled: true

so not obvious.
should that be encouraged when using yaml (e.g. stating it in https://quarkus.io/guides/config-reference)?

it sounds the WithKeys would make sense here.

@radcortez
Copy link

should that be encouraged when using yaml (e.g. stating it in https://quarkus.io/guides/config-reference)?

It can surely be added there. On the other hand, I think users shouldn't have to worry about it. Most users are probably not going to look into that detail and are just going to set the configuration as you did :)

Also, this will only work for the YAML source. Other sources may also provide the key without quotes, which would cause the same issue.

I was thinking again if this could be fixed on the SmallRye Config side, but I don't see a good solution... the only way would be to query both the quoted and unquoted key. Considering that many of these mappings start with a Map it means to double the lookups of every configuration under that tree... and once you find a nested Map, you need to double it again (which is not an uncommon case either). Also, we would have to define which one has priority in case both quoted and unquoted are available.

it sounds the WithKeys would make sense here.

Because it is the extension that is adding additional keys, I believe the best option is the extension to adjust and to generate the key in the same format that the user has provided the other keys. This would not only solve the issue for the YAML source but for any other source.

@vsevel
Copy link
Author

vsevel commented Feb 20, 2025

will you plan this improvement @zhfeng ?

@zhfeng
Copy link
Contributor

zhfeng commented Feb 20, 2025

@radcortez

I believe the best option is to adjust QUARKUS_ARTEMIS_NAMED_URL_TEMPLATE and use quotes if the user set configuration with quotes, or omit the quotes if the user didn't set them.

But when I am running with properties or yaml file, by both of them we alway get the same foo without quotes in ArtemisBuildTimeConfigs.getNames(). so there is no way to adjust QUARKUS_ARTEMIS_NAMED_URL_TEMPLATE

@zhfeng
Copy link
Contributor

zhfeng commented Feb 20, 2025

Hmm, I find a solution like

        for (ConfigSource source : ConfigProvider.getConfig().getConfigSources()) {
            if (source.getName().contains("YamlConfigSource")) {
                useQuote = false;
            }
        }

@radcortez @vsevel WDYT?

@vsevel
Copy link
Author

vsevel commented Feb 20, 2025

but you could use quotes, or not in yaml. this would force the user to use quotes, always.
@radcortez was suggesting to detect if it was used or not. is it not possible?
and what about the mapping with @WithKeys? is this a viable solution?

zhfeng added a commit that referenced this issue Feb 20, 2025
@zhfeng
Copy link
Contributor

zhfeng commented Feb 20, 2025

Hmm, I test it both with quotes or not in yaml, both of them work.

But it does not work with properties file like quarkus.artemis.foo.enabled=true
`

@zhfeng
Copy link
Contributor

zhfeng commented Feb 20, 2025

was suggesting to detect if it was used or not. is it not possible?

No, it seems we can not detect if quotes is used or not in DevServicesArtemisProcessor.java.

@radcortez
Copy link

You would need to look for the raw property names in Config.getPropertyNames to figure out if the user-defined key is using quotes or not and then use the same format to generate the additional keys.

Another option is to just use @WithKeys and instruct precisely which keys should the Map load. This may require some Gizmo generation bits, to populate the annotation supplier, like we do with the REST Client: https://github.com/quarkusio/quarkus/blob/de0e007d6c4b9d9495cf2cd0ce8a8bf312908651/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/AbstractRestClientConfigBuilder.java#L25-L431

@zhfeng
Copy link
Contributor

zhfeng commented Feb 20, 2025

Thanks @radcortez - I'm much clearer now. Let's try the first option to get raw property names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants