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

ENH: Enable ServiceConfig to use alternate ClassLoader - was ServiceConfig add setClassloader api #476

Closed
icode opened this issue Dec 2, 2015 · 11 comments
Assignees
Milestone

Comments

@icode
Copy link
Contributor

icode commented Dec 2, 2015

My framework change classloader load in project class, but ServiceConfig not use my classloader because is not in project, it's in jar. So, i use config ebean.currentUserProvider get user throw class can not case exception.

@rbygrave
Copy link
Member

rbygrave commented Dec 3, 2015

If you go serverConfig.setClassLoader(...) why not go serverConfig.setCurrentUserProvider(...) ?

This does not make sense to me.

@icode
Copy link
Contributor Author

icode commented Dec 3, 2015

serverConfig.setClassLoader(...) can set in framework, get from currentThread

@rbygrave
Copy link
Member

rbygrave commented Dec 3, 2015

Nope, still not clear. "get from currentThread" ... what?

Please put some effort into trying to give a meaningful explanation of the
change you desire and how it will be used.

On 3 December 2015 at 21:53, icode notifications@github.com wrote:

serverConfig.setClassLoader(...) can set in framework, get from
currentThread


Reply to this email directly or view it on GitHub
#476 (comment)
.

@icode
Copy link
Contributor Author

icode commented Dec 3, 2015

In my framework not my app call serverConfig.setClassLoader(Thread.currentThread().getContextClassLoader())

@rbygrave
Copy link
Member

rbygrave commented Dec 3, 2015

Right.

So no I'm not going to add serverConfig.setClassLoader(...) .... (at least
not yet) because that is not the underlying problem.

On 3 December 2015 at 22:03, icode notifications@github.com wrote:

In my framework not my app call
serverConfig.setClassLoader(Thread.currentThread().getContextClassLoader())


Reply to this email directly or view it on GitHub
#476 (comment)
.

@icode
Copy link
Contributor Author

icode commented Dec 3, 2015

Now , in my framework can not configure ebean from config file, like play1, change classloader in dev model, so ebean ClassUtils,newInstance get a error class

@rbygrave
Copy link
Member

rbygrave commented Dec 3, 2015

I understand. It is just that your proposed solution
(serverConfig.setClassLoader(...)) is not a good solution, a better
solution exists.

On 3 December 2015 at 22:15, icode notifications@github.com wrote:

Now , in my framework can not configure ebean from config file, like
play1, change classloader in dev model, so ebean ClassUtils,newInstance
get a error class


Reply to this email directly or view it on GitHub
#476 (comment)
.

@rbygrave
Copy link
Member

rbygrave commented Dec 7, 2015

Right so this issue needs a "Reboot" taking it back to the original problem.

Original Problem:

A ClassNotFoundException can occur when the "current class loader" is not the correct classLoader to use when creating instances of CurrentUserProvider / NamingConvention etc.
ServerConfig.createInstance(...) uses ClassUtil.newInstance(...) uses the current classLoader and there is not sufficient flexibility to use other classLoaders or mechanisms.

Potential Fixes / Changes:

  • Make ServerConfig.createInstance(...) method protected such that it can be overwritten.
  • Change the current default implementation in ClassUtil / ServerConfig.createInstance(...) such that it checks for a uses Thread.currentThread().getContextClassLoader()
  • Extract an interface that "provides" instances (for example, that uses a Dependency injection context to provide instances).

Currently I see the suggestion of setting a classLoader on ServerConfig is an option but not the best solution in terms of providing the most flexibility. The start will be to implement the first 2 fixes / changes and then review.

@rbygrave rbygrave changed the title ServiceConfig add setClassloader api ENH: Enable ServiceConfig to use alternate ClassLoader - was ServiceConfig add setClassloader api Dec 7, 2015
@icode
Copy link
Contributor Author

icode commented Dec 7, 2015

Yeah, you are right. I just to describe the problem. My English is not good, so I can only say so.

rbygrave added a commit that referenced this issue Dec 7, 2015
…erviceConfig add setClassloader api --- part 1 : protected methods
rbygrave added a commit that referenced this issue Dec 7, 2015
…erviceConfig add setClassloader api --- part 2 : Use current context classLoader
@rbygrave
Copy link
Member

rbygrave commented Dec 7, 2015

No problem. I have pushed 2 commits that make some methods protected and will use the context classLoader. If you can build from master you can give it a try.

I'm going to continue to look into the code more with a view to do a bit of cleanup and refactoring.

rbygrave added a commit that referenced this issue Dec 7, 2015
…erviceConfig add setClassloader api --- part 3: Refactor with ClassLoadConfig
@rbygrave
Copy link
Member

rbygrave commented Dec 7, 2015

The last commit there refactored the internals and puts ClassLoadConfig on ServerConfig. The ClassLoadConfig has most of the classLoader specific functions including detecting types to support (Joda, Java time, Jackson etc) and creating new instances.

Out of the box ClassLoadConfig will try to use the current context classLoader and then fallback to the caller one. In addition it can be created with an explicit classLoader to use.

So I think this does what we need it to do. Potentially it could be converted into an interface in time but it might be sufficient as is (with the ability to override methods as desired).

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