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

Feature request: PropertiesDefaultProvider at first try to load properties from classpath #2102

Closed
rimuln opened this issue Sep 1, 2023 · 4 comments

Comments

@rimuln
Copy link

rimuln commented Sep 1, 2023

Would be possible enhance this class to load properties from resources too?

Actual state:

  1. try to load from user.home
  2. try to load from environment

Wanted state:

  1. try to load from classpath via getResourceAsStream
  2. try to load from user.home
  3. try to load from environment

or allow configure IDefaultValueProviders as ordered list?

@remkop
Copy link
Owner

remkop commented Sep 1, 2023

That sounds like a good enhancement!
One caveat is that I am very hesitant to modify existing default behavior.
So my preference would be to load from classpath after trying the current methods, not before.

My time to work on picocli is extremely limited.
Will you be able to provide a pull request for this?

@remkop remkop added this to the 4.7.6 milestone Sep 1, 2023
@rimuln
Copy link
Author

rimuln commented Sep 4, 2023

I understood your opinion. But the oposite order is for us problematic.
In general.
You as developer distribute application with some default settings that could be bundled in jar. Then you expect that user is able override this default settings from outside.

If you pass the loading defaults from classpath as last in order then you always override user defined settings.
And if jar doesn't contains the settings then I would say the default logic is unchanged.

But if I look on it from implementation. You are right first is current code and after this is load from classpath. So theoreticaly this way:

        private static Properties loadProperties(CommandSpec commandSpec) {
            if (commandSpec == null) { return null; }
            Properties p = System.getProperties();
            for (String name : commandSpec.names()) {
                String propertiesFileName = "." + name + ".properties";
                URL resource = CommandLine.class.getClassLoader().getResource(propertiesFileName);
                String path = p.getProperty("picocli.defaults." + name + ".path");
                File defaultPath = new File(p.getProperty("user.home"), propertiesFileName);
                File file = path == null ? defaultPath : new File(path);
                if (file.canRead()) {
                    return createProperties(file, commandSpec);
                } else if (resource != null) {
                    file = new File(resource.getFile());
                    if (file.canRead()) {
                        return createProperties(file, commandSpec);
                    }
                }
            }
            return loadProperties(commandSpec.parent());
        }

@remkop
Copy link
Owner

remkop commented Sep 4, 2023

I think we’re on the same page.

We want applications to be able to bundle default values in their jar, but users need to be able to override the location and specify their own defaults.

I’m on my phone and can’t check the code but the documentation doesn’t seem to mention the ability to specify a path via an environment variable. I don’t oppose this but we’d need to implement, test and document this also.

Do you want to provide a pull request for this?
Looking at the example code in your previous comment, I noticed two things:

  • Let’s postpone the call to Class.getResource (or Classloader.getResource until it’s clear that the previous lookup methods (e.g. user home) failed
  • Which class to use for the Class.getResource call? Your example uses picocli.CommandLine.class, but that class loader may not be able to find a resource in the application jar (which is a different jar than the picocli jar), in some circumstances (unsure about JPMS for example). It may be better to use the class of the user object of the command.
  • The example doesn’t try to load from a path specified in an environment variable. This should probably be the first place to try, then user home, and finally the application jar.
  • Interesting question: should picocli emit a WARN message (via its tracing API) if an environment variable is set but no resource bundle could be found in the specified location? (I hesitate to throw an exception for this scenario, especially since the bundle can potentially be found in the fallback locations…)

Thoughts?

rimuln pushed a commit to rimuln/picocli that referenced this issue Sep 5, 2023
@rimuln
Copy link
Author

rimuln commented Sep 5, 2023

ad 1) ok no problem

ad 2) yes this come to my mind to. In our case it is all in one fat jar made by maven, but I see in docu that picocli.jar could be separate jar so wasn't sure how it works. The command class is 100% better, just I didn't look yet how get it from CommandSpec object.

ad 3) not sure what you mean. The original logic try to load from -Dpicocli.defaults..path, and if not present then from user.home This is still present.

ad 4) I expect no exception as not always user of picocli want provide some defaults on classpath. I see some warning in createProperties will not be enough?

In general I created PR #2107 to discuss directly on code

rimuln pushed a commit to rimuln/picocli that referenced this issue Sep 18, 2023
… from classpath

- check if commandSpec.userObject is null
rimuln pushed a commit to rimuln/picocli that referenced this issue Sep 18, 2023
… from classpath

- addded test for read default values from resource classpath
remkop pushed a commit that referenced this issue Sep 18, 2023
…classpath

- check if commandSpec.userObject is null
remkop pushed a commit that referenced this issue Sep 18, 2023
…classpath

- addded test for read default values from resource classpath
@rimuln rimuln closed this as completed Sep 18, 2023
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

2 participants