Model and completion for property expressions

[datho7561]

In many MicroProfile implementations, ${other.property} can be used to refer to the value of an already defined property when defining the value of another property.

This PR modifies the property file model to allow representing these property expressions. It also adds completion for defined properties when completion is opened after typing ${ as a part of a property value.

This PR was migrated from quarkus-ls.

Signed-off-by: David Thompson davthomp@redhat.com

[datho7561]

The tests appear to be working, but I identified this regression:

[datho7561]

The completions NPE less frequently and should never suggest a property expression that would create a circular dependency. I still need to write some unit tests for the new completions.

[xorye]

I think it would be great to add $ to this list so that completion options are immediately proposed when $ is typed: https://github.com/eclipse/lsp4mp/blob/97fa6bb483a9611c23a3b9f8a81a775623becbc6/microprofile.ls/org.eclipse.lsp4mp.ls/src/main/java/org/eclipse/lsp4mp/settings/capabilities/ServerCapabilitiesConstants.java#L48

[xorye]

I get a ClassCastException when I try the completion in an application.properties file that contains a comment:

#test
quarkus.application.name=name
quarkus.application.version=${|}

(I tried completion on the |)

Error:

Jul. 17, 2020 3:26:46 P.M. org.eclipse.lsp4j.jsonrpc.RemoteEndpoint fallbackResponseError
SEVERE: Internal error: java.lang.ClassCastException: class org.eclipse.lsp4mp.model.Comments cannot be cast to class org.eclipse.lsp4mp.model.Property (org.eclipse.lsp4mp.model.Comments and org.eclipse.lsp4mp.model.Property are in unnamed module of loader 'app')
java.util.concurrent.CompletionException: java.lang.ClassCastException: class org.eclipse.lsp4mp.model.Comments cannot be cast to class org.eclipse.lsp4mp.model.Property (org.eclipse.lsp4mp.model.Comments and org.eclipse.lsp4mp.model.Property are in unnamed module of loader 'app')
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:319)
	at java.base/java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1238)
	at java.base/java.util.concurrent.CompletableFuture$BiApply.tryFire(CompletableFuture.java:1205)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)
Caused by: java.lang.ClassCastException: class org.eclipse.lsp4mp.model.Comments cannot be cast to class org.eclipse.lsp4mp.model.Property (org.eclipse.lsp4mp.model.Comments and org.eclipse.lsp4mp.model.Property are in unnamed module of loader 'app')
	at org.eclipse.lsp4mp.services.MicroProfileCompletions.collectPropertyValueExpressionSuggestions(MicroProfileCompletions.java:412)
	at org.eclipse.lsp4mp.services.MicroProfileCompletions.doComplete(MicroProfileCompletions.java:113)
	at org.eclipse.lsp4mp.services.MicroProfileLanguageService.doComplete(MicroProfileLanguageService.java:84)
	at org.eclipse.lsp4mp.ls.ApplicationPropertiesTextDocumentService.lambda$completion$1(ApplicationPropertiesTextDocumentService.java:162)
	at java.base/java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1236)
	... 7 more

[datho7561]

I think it would be great to add $ to this list so that completion options are immediately proposed when $ is typed:

Added

[datho7561]

I get a ClassCastException when I try the completion in an application.properties file that contains a comment:

Fixed; I added this as a unit test

[datho7561]

Latest commit now suggests properties that appear in the properties file but haven't been defined elsewhere.

[xorye]

It seems like @ConfigItem properties like quarkus.kafka-streams.application-id can have property expressions in their default values (${quarkus.application.name}) and have it resolved properly.

But for config properties defined with @ConfigProperty, if the default value contains a property expression, the property expression doesn't get resolved.

ie, if I have:

    @ConfigProperty(name = "greeting.message", defaultValue = "${greet}")
    String message;

and in application.properties:

greet=hi

I end up with greeting.message=${greet} instead of greeting.message=hi.

My question is whether or not we should take into account the dependencies for projectInfo properties. If we do, it would be ideal if we only consider the ConfigItem properties.

[datho7561]

It seems like @ConfigItem properties like quarkus.kafka-streams.application-id can have property expressions in their default values (${quarkus.application.name}) and have it resolved properly.

Oops. This PR doesn't take this into account. I assumed that the property expressions could only be used within the properties file. The dependencies are only built on properties referenced in the file. So if you did completion here:

quarkus.application.name = ${|

in a project that requires that Kafka configuration value, I would expect quarkus.kafka-streams.application-id as one of the completion items, which if selected for completion, would form a cyclic dependency.

It might be worthwhile to check this, since the cyclic dependency would invalidate the config file.

[datho7561]

I did some testing, and it looks like Helidon doesn't resolve default values for property expressions:

# Application properties. This is the default greeting
app.greeting=Hello from ${combination.property},

# Microprofile server properties
server.port=8080
server.host=0.0.0.0

combination.property = ${server.host}:${server.port} ~ ${mp.openapi.scan.disable}

Output when accessing the resource, where app.greeting is a part of the message:

"Hello from 0.0.0.0:8080 ~ ${mp.openapi.scan.disable}, World!"

I'll test with Quarkus next to see what happens.

[datho7561]

Doesn't work in Quarkus either:

# this is my configuration for quarkus
greeting.message=hello with ${quarkus.debug.reflection}, ${greeting.ve#getable[0].salad}
greeting.name=${greeting.message}
greeting.suffix=EXCLAMATION_MARK

greeting.ve#getable[0].salad = 42

When trying to launch the application with ./mvnw quarkus:dev it errors, despite quarkus.debug.reflection begin false by default:

Caused by: java.util.NoSuchElementException: Property quarkus.debug.reflection not found

[xorye]

In Quarkus, default values seem to be resolved for runtime properties like quarkus.http.port
and not buildtime properties

[xorye]

After discussing with @datho7561, maybe this PR should follow the property expressions defined here: microprofile/microprofile-config#577 instead of the Quarkus property expressions defined here https://quarkus.io/guides/config#using-property-expressions?

microprofile/microprofile-config#577 defines property expressions in the MP spec, but it's not merged yet.

If we decide to follow the proposed Microprofile spec, I think we should handle these cases:

in a different PR, in case this PR is getting too big/complicated.

@fbricon @angelozerr how does this sound?

[angelozerr]

@datho7561 please fix conflicts

[datho7561]

rebased

[angelozerr]

When I have circular dependency with a like:

a=${a}
b=$|

When I try to open completion on b value with $, I have the following error:

[Error - 09:00:12] Request textDocument/completion failed.
  Message: Internal error.
  Code: -32603 
java.util.concurrent.CompletionException: java.lang.IllegalArgumentException: Cannot add self-loop edge on node a, as self-loops are not allowed. To construct a graph that allows self-loops, call allowsSelfLoops(true) on the Builder.
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:319)
	at java.base/java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1238)
	at java.base/java.util.concurrent.CompletableFuture$BiApply.tryFire(CompletableFuture.java:1205)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)
Caused by: java.lang.IllegalArgumentException: Cannot add self-loop edge on node a, as self-loops are not allowed. To construct a graph that allows self-loops, call allowsSelfLoops(true) on the Builder.
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:217)
	at com.google.common.graph.ConfigurableMutableValueGraph.putEdgeValue(ConfigurableMutableValueGraph.java:82)
	at com.google.common.graph.ConfigurableMutableGraph.putEdge(ConfigurableMutableGraph.java:51)
	at org.eclipse.lsp4mp.model.PropertyGraph.<init>(PropertyGraph.java:67)
	at org.eclipse.lsp4mp.services.MicroProfileCompletions.collectPropertyValueExpressionSuggestions(MicroProfileCompletions.java:403)
	at org.eclipse.lsp4mp.services.MicroProfileCompletions.doComplete(MicroProfileCompletions.java:108)
	at org.eclipse.lsp4mp.services.MicroProfileLanguageService.doComplete(MicroProfileLanguageService.java:90)
	at org.eclipse.lsp4mp.ls.ApplicationPropertiesTextDocumentService.lambda$completion$1(ApplicationPropertiesTextDocumentService.java:166)
	at java.base/java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1236)
	... 7 more

[datho7561]

When I have circular dependency with a like:

a=${a}
b=$|

Do you think that it would be a good idea to suggest a as a completion option in this case?

[angelozerr]

Do you think that it would be a good idea to suggest a as a completion option in this case?

I think so. It's not the part of this PR, but we should have an error for a which higlight value ${a} to day that there is a circular problem.

[datho7561]

I opened #61 for this case

[angelozerr]

Congrats @datho7561! It's a very impressive PR.