Provide hover/completion support for property references in server.xml

[cherylking]

When a property or variable is referenced in server.xml like ${xyz}, provide completion support for defined properties/variables. That should include any variable defined in server.xml, as well as the bootstrap.properties and server.env files. No hover support will be provided.

This is a future enhancement and not part of the MVP. Slide 28 of UFO.

[cherylking]

Good Liberty doc for reference

[cherylking]

I think we should go ahead and provide hover support too. We already have to find all the variables, why not resolve their values when possible and display them?

Update: I am flip flopping on hover support. If we cannot guarantee that our processing of the config files match that of Liberty itself, I think it is misleading to provide hover support displaying variable/property reference values. Perhaps we can simply list which file(s) we find that var/prop defined in with clickable links to navigate to them instead? And if we find the var/prop nowhere in the project config files, then display a diagnostic?

[dshimo]

@evie-lau and I have discussed several strategies for tackling this feature. Firstly, we've decided for the MVP to build this feature into LCLS as a starting point. We reasoned it would be simpler to implement the logic in LCLS and then additionally provide xml support, versus managing the logic across both language servers. We built a proof of concept that confirms to us that the IDE features, such as diagnostic and hover, aren't negatively impacted by this redundancy in file watching.

The implementation for completion would minimally require only a couple of things: LCLS would require new context awareness for completion triggered within ${}, and then a way to obtain the map of variables which we can obtain from ci.common.

Hover has room for interpretation. As Cheryl has voiced, we cannot guarantee that the values we process matches Liberty's. On one end, we complete ci.common's processing to match Liberty's behavior and then maybe only provide support on built projects. LCLS will also have to implement tracking 'included' server configs to recognize enabled or disabled sources. On the other end, we do nothing to avoid displaying misinformation. Displaying a list of variables and their sources provides convenience, but ultimately leaves the responsibility of managing variables to the user.

A concern all paths will have to consider is how often do we refresh our variables? Will a letter change make us reprocess everything? Hover and completion can be computed lazily, but if diagnostics track variables vigilantly we won't see a benefit.

[dshimo]

We're sticking through with the decision of providing full feature support for built servers while providing a message or diagnostic to warn users that they may not receive all features until they build their server. There may be an opportunity to associate a quick fix with this diagnostic to build the project for them, but would require a little more research.

Today's discussion brought in LMP/LGP more attention. The main highlight centered around utilizing the liberty-plugin-config.xml as a source of information. Using this file, we can capitalize on dev mode to provide all our necessary information with less worry of showing conflicting information with Liberty. For non-dev mode users, we can smartly monitor key files for changes that may get updated with the next server build. There may be opportunity here to expand upon the want for a new LMP/LGP lite goal that minimally updates the build information for our other tools.

[scottkurz]

2024-01-10 DXDI UPDATE

It took me most of the call and the next day's to really catch up. I won't bother trying to summarize (since the last comment from @dshimo already did).

Let me just tack on a few questions and observations

1. In saying we can't guarantee a "match" with Liberty ... are we just noting that values can come from env vars and the server command line? I think we are basically trying to "match" every value that can be determined from the source repo and the Maven/Gradle build, right?

2. Did we settle on the severity for an undefined variable reference ? I feel like "warning" would be the most obvious. It's what WDT uses. I think there could be a case for "info" since some values will come from env vars etc. but I think at least in Eclipse, enough users are OK with ignoring warnings.

3. Just a reminder that in Eclipse the approach used in lsp4mp to provide a hyperlink elsewhere in the workspace doesn't work all that well. It just opens a link in the browser. See Clicking MP config props link in hover opens file:///... URL in browser rather than opening file in IDE source editor liberty-tools-eclipse#191. Presumably this would be a way to do the same for following variable ref values back to their source.

4. Speaking of WDT, it does have its own behavior which intersects this "does dev mode need to be running to provide full IDE support?" question. It does, for example, watch for a server config change, and update its validation, even when the server is not started, and will even call Maven goals to update the project. I imagine this builds off the Eclipse WTP function via some aspect of the "publish" operation. To me, it looks like there are a number of issues with this function (though I'm no expert). Still, I think it is worth noting the precedent that you don't have to start the server in order to get assistance with the project, which sets some degree of an expectation.

[dshimo]

1. You have the correct idea. We're trying to provide the same functionality as Liberty without using their direct logic. It would be nice to have an API so we wouldn't need to write the logic twice, but that's a different problem.
2. Warning is a great place to start.

[cherylking]

A few notes as we pick this back up:

1. The logic in ci.common has been updated to process all config files and variables as closely as possible to how the Liberty runtime does it.
2. The ServerConfigDocument will have a constructor that takes three Files for the installDir, userDir and serverDir for the Liberty installation in the target/build directory. These locations can be determined from the liberty-plugin-config.xml file.
3. We will move the getLibertyDirectoryPropertyFiles method from LMP/LGP to ci.common so the ServerConfigDocument constructor that takes three Files can instantiate that.
4. Once the ServerConfigDocument object is instantiated, it will have all the vars/props available in getProperties and getDefaultProperties methods.
5. The VariableUtility class has a static public method getPropertyValue that takes a property name, two Properties objects for the properties and default properties, and the Map of libertyDirPropFiles. This can be used to resolve property references in server config files.