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

Consider if content assist can be offered for spring.config.import property keys #536

Closed
philwebb opened this issue Sep 14, 2020 · 15 comments

Comments

@philwebb
Copy link
Member

Spring Boot 2.4 will ship with support that allows you to easily import additional config files. There's a bit of background about the feature in this blog post. Currently the content-assist for spring.config.import will be limited to the hints that we provide in the meta-data JSON file, but we might be able to go further.

The spring.config.import key accepts a list of string location values. Locations are ultimately resolved to provide additional config data. The resolver logic is completely pluggable, but by convention a location prefix will be used to determine how the location is resolved.

Out of the box, we'll have support for configtree: and resource:. The resource: prefix will also be used as the default of no other resolver supports the string.

Some examples:

spring.config.import=/etc/config/myfile.yml # import a file from the filesystem
#---
spring.config.import=resource:/etc/config/myfile.yml # more explicit version of above
#---
spring.config.import=classpath:extra.properties # import a classpath resource
#---
spring.config.import=resource:classpath:extra.properties # more explicit version of above
#---
spring.config.import=configtree:/etc/config/folder # import a config tree

It might be possible to offer some content-assist that would allow users to select files from either their workspace, or from their local disk. I suspect the workspace would be more useful.

For example:

The user types

spring.config.import=

Content assist could provide the usual hints:

+------------+
| classpath: |
| configtree:|
| file:      |
+------------+

It could also provide a richer "select classpath resource..." option. This could bring up a dialog similar to "Open Resource". You could also have a "select filesystem resource..." option.

Another useful feature would be ctrl-click on a location.

For example, given:

spring.config.import=classpath:my.properties

A ctrl-click on classpath:my.properties could open the my.properties file.

@kdvolder
Copy link
Member

I wanted to kick this around a bit to explore how it works.

I created a spring-boot 2.4.0-SNAPSHOT project using initializer. When I put

spring.config.import=blah

Into the application.properties STS editor marks the property as 'unknown'. So I guess this property (or at least metadata for it) doesn't exist on my project's classpath.

Do I need a special/specific dependency or version to be able to use this?

@kdvolder
Copy link
Member

Hmm... strange it works with 2.4.0.M2 but not with 2.4.0.SNAPSHOT. I guess this must be some maven caching issue.

@kdvolder
Copy link
Member

kdvolder commented Sep 15, 2020

This seems somewhat similar to the spring.banner.location property. So I wonder why it is defining the type of the data as String instead of resource.

E.g. in metadata spring.banner.location is defined as:

    {
      "name": "spring.banner.location",
      "type": "org.springframework.core.io.Resource",
      "description": "Banner text resource location.",
      "defaultValue": "classpath:banner.txt"
    },

But for the import property I find instead something like this:

    {
      "name": "spring.config.import",
      "type": "java.util.List<java.lang.String>",
      "description": "Import additional config data.",
      "sourceType": "org.springframework.boot.context.config.ConfigDataProperties"
    },

Why is that not also typed as a Resource?

Reason for asking is, STS already provides some smarts for helping user complete the Resource value. It isn't very good support, and doesn do even half of what you suggested but...

  • it could be imrpoved based on some of your suggestions :-)
  • I don't like to add special logics for a property like 'spring.config.import' based just on the property name itself. Instead I think it should be based on the metadata (e.g. the type of the property could enable specific completion support based on that, or it could be based on a hint declararion).

@philwebb
Copy link
Member Author

Why is that not also typed as a Resource?

The new system is quite flexible and we intent to use it in Spring Cloud to support config sources such as Consul, Zookeeper and Vault. Because of that, it's not necessarily true that a location is always a Resource.

the type of the property could enable specific completion support based on that, or it could be based on a hint declaration.

I'll have to refresh my memory on how those work, but that may indeed be a better option (/cc @snicoll)

@snicoll
Copy link
Member

snicoll commented Sep 16, 2020

The problem with hint is that:

  1. It only works for the value and does not work for prefixes
  2. It isn't possible to contribute hints from multiple places, yet this support is so that multiple implementations can be provided.

On the IDE side, I think an explicit support for the key is needed. And an explicit knowledge of the implementations as well. A next step would be that each implementation of ConfigDataLoader provides additional metadata that the IDE can read to figure this out.

@kdvolder
Copy link
Member

@snicoll I don't understand. Maybe this isn't the same as Resource type, I can understand that much but... Why isn't it possible to just attach a new type of hint to the key spring.config.import that flags that key as 'needs to have special handling as a XXX' where XXX is whatever name we agree to call this kind of special handling.

It only works for the value and does not work for prefixes

I don't understand. Please explain / clarify.

On the IDE side, I think an explicit support for the key is needed. And an explicit knowledge of the implementations as well

Okay, if there is no other way I can of course add a special case for this specific key to trigger 'special support'. But is this really the right answer? The real problem I have is the second part of that statement: "And an explicit knowledge of the implementations as well". I don't see how STS can have this kind of special knowledge, especially if implementation details may change in the future.

@snicoll
Copy link
Member

snicoll commented Sep 16, 2020

.. Why isn't it possible to just attach a new type of hint to the key spring.config.import that flags that key as 'needs to have special handling as a XXX'

We could do that but that special handling only works for spring.config.import. I don't really see the benefit of introducing a hint that only work with a single key.

It only works for the value and does not work for prefixes

I was talking at the existing value hint which is what I thought you both referred to.

I don't see how STS can have this kind of special knowledge, especially if implementation details may change in the future.

Yeah well that's the root of the problem, isn't it? I don't believe it would be possible for us to go full declarative for this either so the most pragmatic approach right now IMO is to have some sort of dedicated behaviour based on available ConfigDataLoader.

@kdvolder
Copy link
Member

I don't really see the benefit of introducing a hint that only work with a single key.

Sure, that's a good argument. And I'll buy that. But isn't that pretty much also the case for property like 'logger.level'?

Anyhow, okay. Let's just assume we handle it as special case. You are right that its not all that different if we recognise the property name directly or just look at whether it has a special hint. In some ways special-casing the property name is actually simpler.

Yeah well that's the root of the problem, isn't it? I don't believe it would be possible for us to go full declarative for this either so the most pragmatic approach right now IMO is to have some sort of dedicated behaviour based on available ConfigDataLoader.

Okay. Regardless of whether or not its flagged by a hint, or triggered by the property name itself, I still need to understand how it should behave. It sounds like you suggest that we inspect the project's classpath and that somehow should help decide the behavior? That's probably do-able. But I'll need help figuring out the exact rules like:

'if this class is on the classpath, then these kinds of completions should be provided'.

Or alternatively, we just provide all these completions/support regardless of whether or not the supporting ConfigDataLoader is actually on the classpath.

I was talking at the existing value hint which is what I thought you both referred to.

I wasn't. I was talking about replacing that with something more similar to how it works for logging propery. I.e. the hint says "for this thing, make suggestions that are typical for logging configuration'. I.e. mark the property with 'this specific key would benefit from this type of special support' and then we of course also would need to spell out in some detail what that actually means.

And sure, we don't have to do it this way, recognizing just the key is not that different from recognizing the hint. But we do still need to concretize exactly what the special handling is supposed to do (what it does is really a different question to how it is activated).

@kdvolder
Copy link
Member

I don't really see the benefit of introducing a hint that only work with a single key.

Maybe if the key's name is changed in a newer version of boot. But I suppose that is very unlikely.

@martinlippert martinlippert added this to the 4.8.1.RELEASE milestone Sep 17, 2020
@snicoll
Copy link
Member

snicoll commented Sep 17, 2020

Sure, that's a good argument. And I'll buy that. But isn't that pretty much also the case for property like 'logger.level'?

Not really, although it's a fine line I reckon. If you need to configure logger levels for whatever reason in your code you can inject a Map<String,String> and then do whatever you need to with the logger info. You can't say that for that feature that can't be touched by users, really.

Or alternatively, we just provide all these completions/support regardless of whether or not the supporting ConfigDataLoader is actually on the classpath.

My gut feeling would give that option a go.

@kdvolder
Copy link
Member

I've thought about this a bit more. Instead of triggering any 'special support' from the property key or from the metadata attached to it, I think instead the simplest thing is probably we trigger it from seeing a prefix/string like classpath:, configtree: or file: in the editor. So from the user, it would be two-step process. First you'd use the current CA proposal already provided by the metadata and you'd get something like this in your editor as a result:

spring.config.import=classpath:

Then when you invoke CA again we (editor CA engine) can notice the 'special' string 'classpath:' and that would trigger it to deliver CA similar to what you'd get if this was explicitly declared as a 'resource'.

@kdvolder
Copy link
Member

kdvolder commented Jan 26, 2022

@philwebb @snicoll Picking this up again. I am trying to find documentation on this feature. Is the above linked blog article still the best location to read/learn about this? I tried looking in the spring-boot-reference docs but so far the only thing that I could find was a one-line entry in the appendix here: https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#application-properties.core.spring.config.import

What I'm looking for is some sort comprehensive description of the various supported syntaxes that are acceptable in the string bound to that property. I assume such a doc exists? I am probably just not looking in the right place so a pointer would be welcome.

@philwebb
Copy link
Member Author

@kdvolder I'm afraid we've not written a developer guide document (beyond that original blog). Most of the details are buried in the javadoc for the code. The system is quite flexible, so I suspect we won't be able to deal with all possible location strings in the IDE. We probably can still make the common strings more useful.

For the code, the there are two main classes that will be useful. The ConfigDataLocation class is a wrapper around the String. It has the optional: prefix support that all locations can use. It also has the hasPrefix method which is the most common way of detecting if a location is for a specific technology. Here's an example where Spring Cloud use it to support configserver: locations.

The other class to look at is ConfigDataLocationResolver. This class can be provided by libraries to actually parse and load the data. The techniques used will depend on the underlying technology.

We've got two important ones in Spring Boot itself. The ConfigTreeConfigDataLocationResolver deals with configtree: locations and StandardConfigDataLocationResolver deals with standard locations (such as file references or classpath: resources).

We've also got some minimal meta-data hints that provide file:, classpath: and configtree: as prefixes.

I'm not 100% sure what we can do the IDE, but perhaps supporting file:, classpath: and configtree: would be a good start. If a location starts with one of those (or optional:<prefix>) we could offer some kind of file/resource selector dialog.

@kdvolder
Copy link
Member

We have implemented some basic support for 'classpath:' prefix. Whenever we see a prefix like 'classpath:...' we'll make completions based on the contents of the project's resource source folder.

I thought about other cases and thought this is probably the only case for which the IDE has enough context information to provide meaningful completions. (I.e. we know what stuff is on the classpath but we don't really know what is on the 'file:' system or the 'config:' tree because I would assume that this actually depends on where the program eventually gets run (e.g. inside k8s / docker container). We might consider implementing some kind of support for 'file' though if that seems useful (opinions welcome, either as comment in this ticket or by raising other tickets with suggestions).

For now I'll just consider this item as complete with just support for 'classpath:'.

@martinlippert
Copy link
Member

I am closing this issue here as we implemented support for the classpath: prefix. We should create new and separate issues for additional improvements.

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

No branches or pull requests

4 participants