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

Enhancement request: make org.osjava.sj.root not mandatory in jndi.properties #16

Closed
giamma opened this issue Sep 4, 2019 · 3 comments

Comments

@giamma
Copy link
Contributor

giamma commented Sep 4, 2019

When moving the application, the root folder of Simple-JNDI most of the times needs to be relocated, so I believe it would be good to make it sufficient to specify the value via system properties, while now it's also mandatory to have it in jndi.properties.

Currently, I can specify pretty much all configuration values that would need to be set in jndi.properties via system properties, with the exception of the root path. In fact, I have a jndi.properties file which only contains the root, with a dummy value that I will be later overwriting via system property.

Still, a root declaration in jndi.properties is mandatory because, even if the code already supports that the root is overwritten via system property in SimpleJNDI.overwriteEnvironmentWithSystemProperties(), if the root is not specified in the properties file the code will fail before reaching the above method when the SimpleJndiContextFactory is initialized in line 73, because contextsByRoot is a concurrent hashmap that does not allow a null key and throws a NPE:

          `final String root = (String) environment.get(SimpleJndi.ROOT);`
          `final Context ctx = contextsByRoot.get(root);`
          `if (ctx != null) {`

The code after the map lookup seems to be able to deal with a null Context, so a change like the following should work:

       ` final String root = (String) environment.get(SimpleJndi.ROOT);`
       ` final Context ctx = root != null? contextsByRoot.get(root) : null;`
        `if (ctx != null) {`

If you are OK with the idea I can raise a pull request.

Edit: fix typo and formatting

h-thurow added a commit that referenced this issue Sep 8, 2019
@h-thurow
Copy link
Owner

h-thurow commented Sep 8, 2019

Just released 0.18.2

@giamma
Copy link
Contributor Author

giamma commented Sep 10, 2019

Thanks a lot, I will give it a try as soon as possible.

@giamma
Copy link
Contributor Author

giamma commented Sep 10, 2019

I confirm it works, thank you. You can close this issue.

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

No branches or pull requests

2 participants