Ensure Spring LogFactory contains all public methods from Apache LogFactory

[maartenc]

Hi,

we encountered a similar issue as #21835, but a bit different use-case.
The fix for #21835 was not sufficient for this situation.

Our Spring application uses other libraries that have constructs like the snippet below.
These libraries were compiled against the original Apache Commons Logging API.

public class Foo {

  static {
    LogFactory.getFactory().setAttribute("org.apache.commons.logging.Log", "org.apache.commons.logging.impl.Jdk14Logger");
  }

}

If we use this class in our Spring application (which includes spring-jcl), we encounter the following error when the Foo class gets loaded:

java.lang.NoSuchMethodError: 'void org.apache.commons.logging.LogFactory.setAttribute(java.lang.String, java.lang.Object)'
       at Foo.<clinit>

The reason for this error is that the Spring-version of the LogFactory class does not specify this setAttribute(...) method, which results in this NoSuchMethodError when some code tries to call it.

I've created a pull request which I believe will solve the issue.
This PR adds all missing public methods to the Spring LogFactory class. To maximize code re-use, I've extracted the existing 'attribute-logic' from LogFactoryService into a new class (LogFactoryImpl).

Maarten

[pivotal-cla]

@maartenc Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@maartenc Thank you for signing the Contributor License Agreement!

[jhoeller]

@maartenc this pull request looks good for a start, thanks for raising it! I didn't expect those attribute methods to be called for non-service usage but we can certainly declare them on the regular LogFactory class nevertheless, ideally in deprecated form.

However, my preference would be for a minimal no-op implementation on the core LogFactory class itself, just supporting actual attribute storage in LogFactoryService, never keeping an instance and its attributes map unnecessarily. So to declare those LogFactory methods as non-abstract but mark them as deprecated and stub them out with no-ops, ignoring set/remove calls and never returning anything from the getters. No need for a LogFactoryImpl class then, not even for a static LogFactory instance in a field, and LogFactoryService would keep the map-based implementation that it has already.

Would this work for your purposes? If you'd like to update the PR accordingly, please rebase it onto the 6.0.x branch. I'd be happy to roll it into 6.0.11 and backport it to 5.3.29 then. Otherwise I can also implement the approach above on my side for you to test.

[maartenc]

Hi @jhoeller , thanks for the review!
I've update the pull request based on your remarks.
Could you take another look?

Regarding the rebase to the 6.0.x branch: could you tell me how I can do this?
I'm not a regular git user, I've tried a few things, but I couldn't get it done.

[jhoeller]

No worries about the target branch, I can also merge this into main for a start and then apply it to 6.0.x and 5.3.x in a custom fashion. I might add a few static methods such as releaseAll() that Commons Logging seems to have as well, double-checking that the public signature is 100% equivalent (beyond the original limited goals of spring-jcl) even if our methods are no-op.

(In general, changing the target branch can e.g. be done on GitHub through the "Edit" button and then a dropdown box for the branch name, but preferably rebasing locally first and then force-pushing the rebased commit to the PR. Since we are going to squash your three commits into one anyway, it won't make much difference here, so no need to spend a lot of time with it.)

[maartenc]

Thanks for your follow-up @jhoeller !