[motlin]

What's changed?

Added Log4j 1.x MDC migration support to the SLF4J recipes:

• Wraps non-String MDC.put(String, Object) values with String.valueOf(...) before migrating to SLF4J.
• Converts MDC.getContext() to MDC.getCopyOfContextMap().
• Retypes affected Hashtable declarations/returns/assignments to Map<String, String>.

What's your motivation?

Log4j 1.x and SLF4J differ in MDC API types: Log4j accepts arbitrary object values and returns Hashtable from getContext(), while SLF4J expects String values and returns a Map. A direct type migration can leave uncompilable code.

Anything in particular you'd like reviewers to focus on?

Working on the Log4j 1.x recipes can be tricky because they work through Log4j 2.x as a stepping stone. I want to make sure I'm not breaking anything for Log4j 2.x users, but don't use it myself.

Anyone you would like to review specifically?

@timtebeek probably?

[timtebeek]

Code review — high effort (self-review of this branch)

Reviewed the two new recipes + YAML/CSV wiring + tests. Six findings, most severe first.

Correctness

1. returnsGetContext() descends into nested/local classes — Log4j1MdcGetContextToCopyOfContextMap.java (returnsGetContext). Only visitLambda/visitNewClass short-circuit, so a return MDC.getContext() inside a local/nested class wrongly flags the enclosing Hashtable-returning method for retyping.

   Hashtable outer() {
       class Local { Hashtable inner() { return MDC.getContext(); } }
       return someOtherHashtable; // outer() gets retyped to Map<String,String> — wrong
   }

2. Parameter-sync loop clobbers unrelated Map generics — visitMethodDeclaration. Runs on every method in any getContext-using file and rewrites the method-type entry of any Map-typed parameter to the raw shallow MAP_TYPE, dropping <K,V>. An unrelated void handle(Map<String,Integer> config) loses its generics. It also drops <String,String> even on the intended retyped parameter.

3. Qualified field assignment missed — the assignment pre-scan only handles J.Identifier LHS, so this.field = MDC.getContext() (a J.FieldAccess) never retypes the field, leaving Map assigned to a Hashtable field (won't compile, even in the composite).

4. Multi-variable sibling widening — retype triggers on the whole declaration if any variable qualifies, so Hashtable a = MDC.getContext(), other = new Hashtable(); silently widens other to Map, breaking later Hashtable-specific calls on it.

5. Cross-file caller breakage (plausible) — retyping a non-override public method signature isn't propagated to callers in files the recipe never visits (no getContext call → UsesMethod precondition skips them). The description only documents the override case.

Cleanup

6. Hand-rolled type expression (plausible) — mapStringString manually assembles J.ParameterizedType/JContainer/Space instead of the idiomatic JavaTemplate path sibling recipes use; fragile and reimplements framework logic.

Checked and cleared: @Getter final String display/description style matches all sibling recipes; composite fixpoint/cycle ordering and recipes.csv count deltas are consistent; log4j 1.2 MDC signatures are correct. Standalone recipe emitting org.apache.log4j.MDC.getCopyOfContextMap() until composed with ChangeType is by design (documented).

[timtebeek]

Fixes pushed (db48d83)

Addressed the three clear correctness findings, with a regression test each:

• 1 nested-class returns — returnsGetContext() now also short-circuits J.ClassDeclaration, so a return MDC.getContext() inside a nested/local class no longer retypes the enclosing method. Test: returnInNestedClassDoesNotRetypeEnclosingMethod.
• 2 parameter-sync over-broad — the method-type sync now only rewrites entries still typed Hashtable whose source parameter is now Map, so unrelated Map<K,V> parameters keep their parameterized attribution. Test: unrelatedMapParameterIsLeftAlone.
• 3 this.field = missed — the assignment pre-scan now handles J.FieldAccess targets, so a field assigned via this.field = MDC.getContext() is retyped. Test: retypeFieldAssignedViaThis. (Minor: the now-unused java.util.Hashtable import is left behind because the this.field reference keeps stale attribution — harmless, pinned in the test.)

Deferred, with rationale:

• 4 multi-variable sibling widening — a correct fix requires splitting Hashtable a = MDC.getContext(), other = new Hashtable(); into two declarations (one type can't cover both). Narrow edge case; leaving as a known limitation for now.
• 5 cross-file caller breakage — inherent to a single-file, UsesMethod-gated recipe; callers in unvisited files can't be reached. This is the same class of manual-fix caveat the description already flags for signature changes.
• 6 hand-rolled mapStringString — works and is tested; building a standalone parameterized type expression via JavaTemplate in declaration-type position is awkward and riskier than the current LST construction. Left as-is.

All 20 tests in the four affected suites pass.

[timtebeek]

Thanks a lot for the help here! Took a bit of time to trickle through some small improvements, but looks ok otherwise!

[motlin]

Thank you again! It looks like you're using a helpful Openrewrite AI skill. I wonder if it's public? I'd like to catch as much as I can before raising PRs.