Skip to content

Conversation

hazendaz
Copy link
Member

No description provided.

@hazendaz hazendaz requested a review from jeffgbutler November 24, 2021 01:55
@hazendaz
Copy link
Member Author

@jeffgbutler log4j1 is critically vulnerable thus the point of this. Github has been notifying us for the longest on this. Its removed from I think every single other mybatis project now. Preference given to slf4j. What I don't know is context on what this was used for here as it seems odd on the surface having so many loggers. So what I did was just put a preference on slf4j. This may be entirely incorrect thinking. Can you take a look at this?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.046% when pulling dffcda6 on hazendaz:develop into a2db936 on mybatis:master.

@jeffgbutler
Copy link
Member

@hazendaz All these logger implementations exist in the Generator because I basically copied the logging code from MyBatis3 core into the generator years ago. So the log factory will try to use whatever log implementation is on the classpath. I changed the preference to SLF4J quite a long time ago and got no push back (although anyone using Log4J v1 experienced no issues).

This code does the log implementation selection:

static {
tryImplementation(new Slf4jLoggingLogFactory());
tryImplementation(new JakartaCommonsLoggingLogFactory());
tryImplementation(new Log4j2LoggingLogFactory());
tryImplementation(new Log4jLoggingLogFactory());
tryImplementation(new Jdk14LoggingLogFactory());
tryImplementation(new NoLoggingLogFactory());
}

I guess I think that it might be best to just remove support for Log4J v1 altogether. As I understand it, this PR would trick the log4j v1 code into using SLF4j, but if slf4j is already on the classpath then it will be selected in preference to log4j v1 anyway.

WDYT?

@hazendaz
Copy link
Member Author

@jeffgbutler That all makes sense now. The support for log4j1 is going to be removed from the core. There is a PR out there that was done back in 2018 that needs some work but that will get done most likely by end of year and make next release. Mybatis doesn't have a direct dependency on log4j1 though but I think we kind of called that removal 3 years ago and just never followed through. So I think answer here is what you suggest, smaller PR that just removes the log4j1 support and doesn't try to manipulate everything else. I can work something up this weekend to replace this one so you can see how that looks.

@jeffgbutler
Copy link
Member

@hazendaz I went ahead and did the PR to remove log4j v1 (#779). It was in the eclipse build too which can get a little tricky to deal with if you're not used to it.

@hazendaz
Copy link
Member Author

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants