Improve DevUI Configuration Editor

[ueberfuhr]

This PR resolves #43229 and the problems described in this issue. Here are the details, what I've changed:

Conceptual Changes

• editing is now only allowed for properties that already exist in application.properties
  • for other properties, there is a button to explicitely add them to application.properties
    
  • this is not possible for environment variables and system properties - and for properties that have a star (*) in their name (wildcards)
• if the calculated property value derives from the raw value in application.properties, there's a warning that updating this property will lead to overwriting the raw value (this might be the case when Maven Resource Filtering is used, or when a property is overwritten by an env var)
  
• if there are profile-specific values in application.properties,
  • they are hidden, if the profile is not active
  • the profile-specific property is preferred for editing, and the default (non-profile-specific is hidden)

Further UI Changes

• renamed the config sources (replaced the technical names from SmallRye Config to more intuitive names)
• put the filter select for config sources left to the filter text field (to get more awareness)
  
• displaying information about the property's config source and (for profile-specific properties) the profile
  
• Fix: Icon "Automatically set by Devservices" was broken
  

Code Changes

• general refactorings to get cleaner code
• reduced count of RPC calls (1 instead of 2) - removed getAllValues() and added information about existence in application.properties to the results of the other call
• UI best practices, e.g. use data- attributes to get an input field (instead of being dependent from the hierarchy, which doesn't work for the checkboxes)
• access (r/w) to application.properties is implemented in a separate class (ApplicationPropertiesService), that is invoked by the RPC service and the processor
  • reading the file is now done the same way like SmallRye Config does (which leads to e.g. a consistent handling of different encoding)
  • re-wrote the algorithm to merge the existing application.properties with single property updates, which is more robust concerning
    • profile-specific properties
    • space characters before and after property keys
    • line breaks in property values
  • extended tests for this

Known Restrictions

• we can only deal with a single application.properties file - using application-<profile>.properties is not supported
• displaying shadowed properties is not possible
• editing wildcard properties is not possible

Fix #19913

[quarkus-bot]

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

• title should not end up with dot

_{This message is automatically generated by a bot.}

[phillip-kruger]

@radcortez will have to look at this.

[melloware]

@ueberfuhr i really like where you went with this. It keeps me happy while adding some great value!

[github-actions]

🎊 PR Preview bbd2c1e has been successfully built and deployed to https://quarkus-pr-main-43354-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[radcortez]

Thank you for the PR!

• this is not possible for environment variables and system properties - and for properties that have a star (*) in their name (wildcards)

We shouldn't remove the ability to save a configuration from either system or env to application.properties. Sys and Env are not recorded during the Quarkus build, so you may be missing the required configuration for your application. In the editor, it is easy to check for those properties and add them. With this change, users will need to do this manually. Also, it seems inconsistent that we allow to change properties coming from other sources.

if there are profile-specific values in application.properties,

• they are hidden, if the profile is not active
• the profile-specific property is preferred for editing, and the default (non-profile-specific is hidden)

We shouldn't remove the ability to edit properties from non-active profiles. For instance, you may want to edit properties from the test profile while running continuous testing.

[ueberfuhr]

@radcortez, this problem is the inconsistence. All configuration properties are loaded by SmallRye, while saving properties is done by the configuration editor's backend. The current (release) behavior is:

Storing System Property user.name in application.properties

1. Open Configuration Editor - user.name is the value of the current user
2. Modify the value and click the save button - the property is saved into application.properties with the new value
3. Refresh the page in the browser - the property is shown with the original value
4. Restart the app - the property is shown with the original value

Storing Environment Variable JAVA_HOME in application.properties

This has the same behaviour. Additionally, we have to be aware of the name mapping. Currently the editor saves the property as JAVA_HOME, but it should be java.home. The property java.home already exists (as System Property).

I can't see any use case where I would need to store a system property or environment variable into application.properties, because they have different scopes and overwrite the values in application.properties, which is confusing IMHO.

Also, it seems inconsistent that we allow to change properties coming from other sources.

Of course, we could also deny to change them, but I'm not sure why SmallRye Config returns unknown config sources, or what "BuildTime RunTime Fixed" means. And what it does matter in relation to application.properties?

• quarkus.http.compress-media-types is BuildTime RunTime Fixed
• quarkus.http.compression-level has unknown ConfigSource
• quarkus.http.body.preallocate-body-buffer is DefaultValue ConfigSource

All of them could be overwritten in application.properties.

[ueberfuhr]

We shouldn't remove the ability to edit properties from non-active profiles. For instance, you may want
to edit properties from the test profile while running continuous testing.

Sure, I could remove the filter. But if we have the property for dev profile, it overwrites the value of the non-profile property. So the last one is not visible in the editor in both old and new version. It would be more consistent to only show the current value property value with a hint to the profile.
So should I remove the filter and show properties from other profiles, although the non-profile property is not visible unless we read the application.properties separately and show also such properties?

[radcortez]

@radcortez, this problem is the inconsistence. All configuration properties are loaded by SmallRye, while saving properties is done by the configuration editor's backend.

I don't see the editor as an application.properties editor (for that, you already have the IDE), but as a way to see every available configuration property and save the ones we want to edit later. If it were only meant to be an editor, then we wouldn't need to list every property; we would only list the file's contents.

Yes, suppose you change a System or Env property. In that case, you will still see the original value because the change is persisted in application.properties, but the configuration may be relevant for colleagues or any other system that don't have your Env.

Of course, we could also deny to change them, but I'm not sure why SmallRye Config returns unknown config sources, or what "BuildTime RunTime Fixed" means. And what it does matter in relation to application.properties?

• quarkus.http.compress-media-types is BuildTime RunTime Fixed
• quarkus.http.compression-level has unknown ConfigSource
• quarkus.http.body.preallocate-body-buffer is DefaultValue ConfigSource

All of them could be overwritten in application.properties.

The BuildTime RunTime Fixed is a source that contains every configuration required for build time that is also available at runtime, but that configuration cannot be changed at runtime. If we limit the ability to edit values from System and Env, then we also need to disable this one because it will behave the same.

In reality, any configuration coming from a source with higher ordinality than application.properties will always override its values, and you will see the inconsistency that you describe. To further confuse things, we are only dealing with the default sources here, but there are additional sources users can add, and extensions can also provide their sources. We may have cases where when you edit foo.bar, you see the edited value updated, and by adding an extension, when you edit foo.bar, you don't see the updated value anymore. I recognize that this may be confusing for the user.

Returning to the proposal, locking the System and Env source makes things even more confusing (I admit I may be biased here). The current editor allows you to do things freely, I guess we can call it inconsistently consistent :) The proposed solution will lock in properties depending on their source. So, I may be editing a property, but if I add it to one of those sources, I can't edit it anymore.

We could take a step back and think about other ways to improve the situation. We know that application.properties is the main file where users add their configuration. We could combine the current editor with a proper view of application.properties. For instance, we could list the entire content of the file, and include that properties are being overriding by something else:

# from application.properties
foo.bar=app
# from system
foo.bar=system

`foo.bar` is being overridden by System Properties with the value `system.`

In this way, you know who is overriding, and the editing becomes clear; that is only for application.properties, and there will not be a value change.

Sure, I could remove the filter. But if we have the property for dev profile, it overwrites the value of the non-profile property. So the last one is not visible in the editor in both old and new version. It would be more consistent to only show the current value property value with a hint to the profile.
So should I remove the filter and show properties from other profiles, although the non-profile property is not visible unless we read the application.properties separately and show also such properties?

We can create a new SmallRyeConfig instance with no profile, which will give us all the hidden active profiles. We do that in other places:

quarkus/core/deployment/src/main/java/io/quarkus/deployment/configuration/BuildTimeConfigurationReader.java

Lines 1033 to 1106 in be4817d

	private Set<String> getAllProperties(final Set<String> registeredRoots) {
	// Collects all properties from allowed sources
	Set<String> sourcesProperties = new HashSet<>();
	for (ConfigSource configSource : config.getConfigSources()) {
	if (configSource instanceof SysPropConfigSource || configSource instanceof EnvConfigSource
	|| "PropertiesConfigSource[source=Build system]".equals(configSource.getName())) {
	for (String property : configSource.getPropertyNames()) {
	String unprofiledProperty = property;
	if (property.startsWith("%")) {
	int profileDot = property.indexOf('.');
	if (profileDot != -1) {
	unprofiledProperty = property.substring(profileDot + 1);
	}
	}
	if (PropertiesUtil.isPropertyInRoots(unprofiledProperty, registeredRoots)) {
	sourcesProperties.add(property);
	}
	}
	} else {
	sourcesProperties.addAll(configSource.getPropertyNames());
	}
	}
	AbstractConfigSource sourceProperties = new AbstractConfigSource("SourceProperties", 100) {
	@Override
	public Set<String> getPropertyNames() {
	return sourcesProperties;
	}
	@Override
	public String getValue(final String propertyName) {
	// Required because some interceptors call getValue when iterating names
	return config.getRawValue(propertyName);
	}
	};
	Set<String> properties = new HashSet<>();
	// We build a new Config to also apply the interceptor chain to generate any additional properties.
	SmallRyeConfigBuilder builder = ConfigUtils.emptyConfigBuilder();
	builder.getSources().clear();
	builder.getSourceProviders().clear();
	builder.setAddDefaultSources(false)
	.withInterceptors(ConfigCompatibility.FrontEnd.nonLoggingInstance(), ConfigCompatibility.BackEnd.instance())
	.addDiscoveredCustomizers()
	.withProfiles(config.getProfiles())
	.withSources(sourceProperties);
	for (String property : builder.build().getPropertyNames()) {
	properties.add(property);
	}
	// We also need an empty profile Config to record the properties that are not on the active profile
	builder = ConfigUtils.emptyConfigBuilder();
	// Do not use a profile, so we can record both profile properties and main properties of the active profile
	builder.getProfiles().add("");
	builder.getSources().clear();
	builder.getSourceProviders().clear();
	builder.setAddDefaultSources(false)
	.withInterceptors(ConfigCompatibility.FrontEnd.nonLoggingInstance(), ConfigCompatibility.BackEnd.instance())
	.addDiscoveredCustomizers()
	.withSources(sourceProperties);
	List<String> profiles = config.getProfiles();
	for (String property : builder.build().getPropertyNames()) {
	String activeProperty = ProfileConfigSourceInterceptor.activeName(property, profiles);
	// keep the profile parent in the original form; if we use the active profile it may mess the profile ordering
	if (activeProperty.equals("quarkus.config.profile.parent") && !activeProperty.equals(property)) {
	properties.remove(activeProperty);
	}
	properties.add(property);
	}
	return properties;
	}

Let's try to nail down the other piece first, and then we can figure out what to do here.

Thanks!

[gsmet]

Where are we for this one? It looked like an interesting improvement. Is there a plan to finalize this?

[radcortez]

As per my previous comments, we shouldn't remove the ability to save a configuration from System or Env or the ability to edit properties from non-active profiles.

But I won't block the changes if everyone agrees that this is acceptable.

[ueberfuhr]

Great and valuable discussion here @radcortez 👍

I don't see the editor as an application.properties editor (for that, you already have the IDE), but as a way to see every available configuration property and save the ones we want to edit later. If it were only meant to be an editor, then we wouldn't need to list every property; we would only list the file's contents.

The Dev UI gives the impression that this should be an editor:

Yes, suppose you change a System or Env property. In that case, you will still see the original value because the change is persisted in application.properties, but the configuration may be relevant for colleagues or any other system that don't have your Env.

So it is a common use case that developers have their own system properties in their development environment, and they want to "share" them with the team?

In case of environment variables, this wouldn't work anyway because of the name mapping (X_Y vs. x.y).
For storing, but also for showing hints about environment variables overriding a property in application.properties, we would need to implement the logic described in the MicroProfile Config Spec. 🥴 (In a perfect world, such logic would be implemented in SmallRye Config itself as part of a kind of tooling interface. 😏 - it feels that we implement much logic here that might not be our responsibility)

The BuildTime RunTime Fixed is a source that contains every configuration required for build time that is also available at runtime, but that configuration cannot be changed at runtime. If we limit the ability to edit values from System and Env, then we also need to disable this one because it will behave the same.

Just to be clear - with "BuildTime", you mean the build time of a developer's app, not the build time of Quarkus itself? So there are properties, that can be changed while running the developer's app`? How would the changes then get hot-deployed?

And, to be honest, I cannot understand why quarkus.appliction.name is fixed, and why quarkus.arc.auto-inject-fields is not. 🙈

In reality, any configuration coming from a source with higher ordinality than application.properties will always override its values, and you will see the inconsistency that you describe. To further confuse things, we are only dealing with the default sources here, but there are additional sources users can add, and extensions can also provide their sources. We may have cases where when you edit foo.bar, you see the edited value updated, and by adding an extension, when you edit foo.bar, you don't see the updated value anymore. I recognize that this may be confusing for the user.

Okay, was not aware of this.

Returning to the proposal, locking the System and Env source makes things even more confusing (I admit I may be biased here). The current editor allows you to do things freely, I guess we can call it inconsistently consistent :) The proposed solution will lock in properties depending on their source. So, I may be editing a property, but if I add it to one of those sources, I can't edit it anymore.

My thoughts were that when we allow editing and saving the value in the Form Editor, the user would expect to value to be applied to the application and the resulting behavior. By locking it, the user should be aware of that it is not applyable. Instead of clicking the save button, the user could copy the property to the "Source Editor" manually.

We could also add a button to save the value to the properties, but without editing the value, just save it with a hint that this would not change anything.

We could take a step back and think about other ways to improve the situation. We know that application.properties is the main file where users add their configuration. We could combine the current editor with a proper view of application.properties. For instance, we could list the entire content of the file, and include that properties are being overriding by something else:

# from application.properties
foo.bar=app
# from system
foo.bar=system

`foo.bar` is being overridden by System Properties with the value `system.`

In this way, you know who is overriding, and the editing becomes clear; that is only for application.properties, and there will not be a value change.

You mean for displaying in the UI? (We could not add the same property twice to the application.properties.)
Would be an idea, but there are also the profile-specific properties, that could be part of the application.properties, that could also be overridden by the environment... 🥴 I guess this could not be implemented in a way that it would be fully correct.

[radcortez]

So it is a common use case that developers have their own system properties in their development environment, and they want to "share" them with the team?

That is one use case, yes. Now, we may discuss whether the use is strong enough to consider it or whether we should drop it. I am fine with dropping it, but I would also like to hear other opinions.

In case of environment variables, this wouldn't work anyway because of the name mapping (X_Y vs. x.y). For storing, but also for showing hints about environment variables overriding a property in application.properties, we would need to implement the logic described in the MicroProfile Config Spec. 🥴 (In a perfect world, such logic would be implemented in SmallRye Config itself as part of a kind of tooling interface. 😏 - it feels that we implement much logic here that might not be our responsibility)

Actually, when you set an env var, the EnvSource automatically generates and lists its dotted format name with the same value. As an example, if you set QUARKUS_HTTP_PORT=9000, you will see that one and also quarkus.http.port=9000 even if you don't have the dotted version. In this case, you could edit the dotted version and get the same effect, but I agree this needs to be more intuitive. Depending on what we end up agreeing on, one possibility is to only include the dotted format names coming from the EnvSource.

Just to be clear - with "BuildTime", you mean the build time of a developer's app, not the build time of Quarkus itself? So there are properties, that can be changed while running the developer's app`? How would the changes then get hot-deployed?

The configuration does not support reloading. Dev mode may reload Quarkus depending on the configuration change, but that cannot be considered a configuration reload. This source is used by configuration required at build time, and that can't be read at runtime, but changing it has no effect at runtime. That is why it is generated by Quarkus and set at the maximum ordinal, so users cannot override the values.

We could also add a button to save the value to the properties, but without editing the value, just save it with a hint that this would not change anything.

It could work. I think that to make it consistent, it would have to be applied to any configuration source higher than application.properties.

You mean for displaying in the UI? (We could not add the same property twice to the application.properties.)
Would be an idea, but there are also the profile-specific properties, that could be part of the application.properties, that could also be overridden by the environment... 🥴 I guess this could not be implemented in a way that it would be fully correct.

Yes. It doesn't have to be there twice. We already load application.properties in the Dev-Ui, and we know from which source a specific configuration is coming, so we only need to match both and display it as an info message or something.

@gsmet @phillip-kruger @maxandersen If you feel otherwise, please let us know, and OK to drop my reservations.

Thank you for your patience.

[phillip-kruger]

@radcortez your call.

[gsmet]

@ueberfuhr this looks like a really good contribution and I would really like us to make progress on it. Any chance you could build a plan from @radcortez 's comment and see if you can get on the same page? Thanks!

[ueberfuhr]

@ueberfuhr this looks like a really good contribution and I would really like us to make progress on it. Any chance you could build a plan from @radcortez 's comment and see if you can get on the same page? Thanks!

Sure, but it would take me some time because of the daily business. 😉 I'll try it for the next 2 weeks.

[ueberfuhr]

BTW, @phillip-kruger / @radcortez / @gsmet is there any place for internal documentation in the Quarkus repository? I mean, we discuss the concepts in this PR, but these discussions will get lost in the future, and there will be no history of changes made to the concept. And I think this is not an architectural decision that could be placed as an ADR.