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

Configuration Form Editor in Dev UI is confusing - disable rewriting application.properties #43229

Open
ueberfuhr opened this issue Sep 12, 2024 · 7 comments · May be fixed by #43354
Open

Configuration Form Editor in Dev UI is confusing - disable rewriting application.properties #43229

ueberfuhr opened this issue Sep 12, 2024 · 7 comments · May be fixed by #43354

Comments

@ueberfuhr
Copy link
Contributor

ueberfuhr commented Sep 12, 2024

Description

Multiple irritations concerning the Configuration Form Editor in Dev UI, when we edit properties - and the editor stores the changes into the application.properties. TL;DR: I would suggest to disable re-writing properties changes to the application.properties file.

Encoding

SmallRye Config loads the application.properties with this code:

try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream, UTF_8))) {
    properties.load(reader); 
}

(Source)

Quarkus' ConfigurationProcessor uses

Files.readAllLines(configPath)

In case of ISO-8859-1 encoded application.properties files, the first one works (except for some characters that are decoded to invalid chars), the second one throws an exception.

Environment Variables

As described in the MicroProfile Config spec, properties are resolved from multiple config sources with a given priority. E.g. environment variables overwrite values in the application.properties. Changing the values in the Dev UI and writing them into the application.properties won't work because they are still shadowed by the environment variables. Much worse: This changes the convention for environments that do not have such an environment variable!

Profiles

Same with profiles. If we have a %dev.<name>=<value> property, it is re-written as <name>=<value>, which won't work for dev profile, but change default values for all other profiles!

Formatting

If we use space chars in application.properties(variable = value), then the entry will occur twice, because it is not found when re-writing the values.

Maven Resource Filtering

If we use Maven resource filtering to replace ${} expressions by Maven properties, the resolved values are shown in the Dev UI. If we change them, we will replace the ${} expression in the source. This is because of another difference between SmallRye Config and the ConfigurationProcessor: SmallRye loads from target/classes/application.properties, whereas the ConfigurationProcessor writes to src/main/resources/application.properties.

Hot Replacement

Of course, properties are loaded and injected only at startup, changes during runtime won't be processed by the components. For continuous testing, this might be helpful, so we could provide a CDI-event-based approach in the Dev UI, but this won't work for all properties. (and the developer should be aware of that - this might be unclear when providing an editor that allows changing values at runtime)

Implementation ideas

Because of all these confusing points, I'm not sure what added value the editor has (except for single cases like continuous testing). I would suggest to

  • disable rewriting changes to application.properties (this could be done by the developer manually)
  • allow changes only for some properties (maybe those properties can be registered to be editable) and fire CDI events in case of changes

Of course, I could make a PR for this, but first I need the discussion about that.

Copy link

quarkus-bot bot commented Sep 12, 2024

/cc @cescoffier (devui), @phillip-kruger (devui), @radcortez (config)

@maxandersen
Copy link
Member

the editor has the value of allowing you to update the application.properties values. if you have env variable sets then that is effectively you saying "please override whatever app properties states" ... that is not something any config editor can change.

we could maybe warn the user that this change possibly wont take effect as an env var is present?

about suggestion on only allow some changes and fire cdi events - not sure what properties that would be because Quarkus does not in general support dynamic reload of the config as that brings even nastier bugs :)

About iso/utf then that is a thing to change. I don't believe we support anything but UTF-8 anyways - but if smallrye readss your file fine with utf-8, so should quarkus.

@ueberfuhr
Copy link
Contributor Author

ueberfuhr commented Sep 12, 2024

Then maybe the editor/the underlying classes should be improved to work much more stable.

  • the encoding
  • profile-specific entries (%dev)
  • the Form Editor should only allow modification of properties in special circumstances (if properties are not calculated form env vars or expressions, only if they exist in the application.properties or have default values)
  • spaces should not break the correct functionality

This is what happens currently - the first three lines are the original entries, the last three were created by the Form Editor:
image

about suggestion on only allow some changes and fire cdi events - not sure what properties that would
be because Quarkus does not in general support dynamic reload of the config as that brings even nastier bugs :)

Of course, this should not be supported for production, but could be interesting in DevMode. If I change the JUnit test properties (e.g. quarkus.test.include-tags), it would be nice when Continuous Testing could handle this change during runtime.

@maxandersen
Copy link
Member

@ueberfuhr continous testing works as a restart afaik so not relevant there (IMO)

Can you open the specific issues you found - as they looks relevant and fixable but trying to handle them all in one is going to be confusing. The solution isn't to disable the editor IMO :)

@ueberfuhr
Copy link
Contributor Author

Of course, I'll prepare PRs for them.

@melloware
Copy link
Contributor

Agree with @maxandersen I use the editor all the time with great success and have not run into any of the issues you mention above.

So please do not take away editing!

@ueberfuhr ueberfuhr linked a pull request Sep 18, 2024 that will close this issue
@ueberfuhr
Copy link
Contributor Author

@melloware / @maxandersen - I have created a PR (a single one, because multiple PRs would depend on each other) with details about suggested changes. Please have a look to #43354 and test, if this matches your requirements.

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

Successfully merging a pull request may close this issue.

3 participants