The Wayback Machine - https://web.archive.org/web/20201121230130/https://github.com/spring-projects/sts4/issues/536
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

Open
philwebb opened this issue Sep 14, 2020 · 11 comments
Open
Labels
Milestone

Comments

@philwebb
Copy link
Member

@philwebb philwebb commented Sep 14, 2020

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

@kdvolder kdvolder commented Sep 15, 2020

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

@kdvolder kdvolder commented Sep 15, 2020

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 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

@philwebb philwebb commented Sep 16, 2020

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 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

@kdvolder kdvolder commented Sep 16, 2020

@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 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

@kdvolder kdvolder commented Sep 16, 2020

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

@kdvolder kdvolder commented Sep 16, 2020

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 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

@kdvolder kdvolder commented Sep 25, 2020

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'.

@martinlippert martinlippert modified the milestones: 4.8.1.RELEASE, 4.8.2.RELEASE, 4.9.0.RELEASE Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.