ramanathan1504 commented on PR #4156:
URL: https://github.com/apache/logging-log4j2/pull/4156#issuecomment-5128109308
Thanks for the quick turnaround @vpelikh. Both of my earlier points are
confirmed fixed:
**`JdbcAppender`** — I ran the full reactor build off your branch and all 27
modules are green, with zero `does not have a public setter` diagnostics
anywhere:
```
[INFO] Apache Log4j JDBC ................................. SUCCESS
[INFO] BUILD SUCCESS
```
**Spotless** — `./mvnw spotless:check` is clean across the whole reactor.
`PluginProcessorPublicSetterTest` passes 3/3, and the existing
`PluginProcessorTest` / `GraalVmProcessorTest` still pass.
So the blocking issues are gone. A few things came up while I was reading
the port more closely.
---
### 1. The new test leaves generated files in the source tree
This is the one I'd like fixed before merge. Running just the new test on a
clean checkout:
```
$ ./mvnw -pl log4j-plugin-processor -Dtest=PluginProcessorPublicSetterTest
surefire:test
$ git status --porcelain
?? log4j-plugin-processor/example/
??
log4j-plugin-processor/org.apache.logging.log4j.plugins.model.PluginService
```
Those are the processor's real outputs (`example/plugins/Log4jPlugins.java`
and the service registration), written relative to the working directory
because the compilation task doesn't specify an output location.
The cause is that `@AfterEach` cleans up `Log4j2Plugins.dat` — that's the
**2.x** artifact name. `Log4j2Plugins` doesn't appear anywhere in the 3.x
processor; 3.x generates a `Log4jPlugins` class plus a `META-INF/services`
entry instead. So the cleanup was carried over verbatim from the 2.x test and
deletes a file that is never created, while missing the two that are.
Simplest fix is to give the compiler task an output directory under
`@TempDir` (`-d` / `-s`) so nothing lands in the module directory at all. That
would also let you drop the copy of `FakePluginPublicSetter.java.source` into
`src/test/resources/` — writing a `.java` into the source tree and deleting it
afterwards is a bit fragile if a test fails partway.
### 2. `@SupportedAnnotationTypes` diverges from 2.x more than it needs to
The PR notes say the annotation-types list had to change "because 3.x has
two `@PluginBuilderAttribute` variants". I don't think that follows. 2.x keeps
`@SupportedAnnotationTypes` scoped to `Plugin` and resolves the attribute
annotation inside `process()`:
```java
final Set<? extends Element> pluginAttributeBuilderElements =
roundEnv.getElementsAnnotatedWith(elementUtils.getTypeElement(PLUGIN_BUILDER_ATTRIBUTE_ANNOTATION));
processBuilderAttribute(pluginAttributeBuilderElements);
```
Handling two variants there is just two `getTypeElement` lookups — it
doesn't require adding them to `@SupportedAnnotationTypes` or restructuring
`process()` into an FQN if/else chain.
Two consequences of the current approach worth weighing:
- The processor is now invoked in rounds where no `@Plugin` is present at
all, so it validates `@PluginBuilderAttribute` fields in classes that aren't
plugin builders. That's a wider net than 2.x casts. It may well be what you
want, but it's a behavioural change from the code being ported and should be a
deliberate call.
- The two FQNs are hardcoded in the annotation list *and* again in the
`process()` dispatch, while `internal/Annotations.java` already owns exactly
these strings (`QUALIFIER_TYPE_NAMES` lists both `PluginBuilderAttribute`
variants). That file also carries a note that its annotations "must also be
declared in `GraalVmProcessor`" — there's an existing coupling here that the
new code sidesteps rather than joins.
Could you either follow the 2.x shape, or reuse `Annotations`?
### 3. You fixed a real 2.x bug — please say so (and test it)
Worth calling out because it's a genuine improvement, not just a port. 2.x
guards the loop with:
```java
if (methodName.toLowerCase(Locale.ROOT).startsWith("set") &&
parameters.size() == 1)
```
so the `withXxx` half of its `followsNamePattern` check is unreachable dead
code — a `withFoo` setter would never satisfy 2.x's validator. Your version
guards on `set` **or** `with`, which makes `withXxx` actually work.
That's a good catch and should be mentioned in the PR description as an
intentional deviation. It also deserves a test case — `FakePluginPublicSetter`
currently only exercises `setXxx`.
### 4. Suppress vs. fix — and a collision with #4153
Five pre-existing violations are silenced with `@SuppressWarnings`. I'd like
a sentence on each, because the point of the check is to surface exactly these:
- **`StringMatchFilter.text`** — suppressed because the setter is named
`setMatchString`. This collides with #4153, which renames that setter to
`setText`; once it lands the suppression is dead. The two PRs also touch
adjacent lines of the same file, so whichever merges second will conflict.
Let's agree an order — my preference is #4153 first, then drop the suppression
here.
- **`Rfc5424Layout.enterpriseNumber`** — no setter at all, and there's a
sibling `ein` field. That smells like a genuine latent bug rather than a false
positive. Can it be fixed instead of muted?
- **`SocketPerformancePreferences`** (3 fields) — void setters, the same
shape as the `JdbcAppender` ones you *fixed* by returning `B`. I suspect the
difference is that this isn't a fluent builder (`newBuilder()` returns the
class itself), which is a fine reason — just please state it, so the
inconsistency doesn't read as arbitrary.
### 5. Should the severity be configurable?
Not a defect in this PR, just a design question. 2.x routes every processor
diagnostic through a helper gated by the
`log4j.plugin.processor.minAllowedMessageKind` option, specifically so build
environments can dial processor messages down. `main`'s `PluginProcessor` has
never had that option, so you haven't removed anything — but this PR introduces
the first hard `ERROR` that can break a downstream plugin author's compile,
with only the per-field `@SuppressWarnings` as an escape hatch. Worth deciding
whether the option should come across too, or whether `@SuppressWarnings` is
considered sufficient.
### Minor
- `javax.tools.Diagnostic.Kind.ERROR` is fully qualified, but
`javax.tools.Diagnostic.Kind` is already imported and used as `Kind.NOTE`
elsewhere in the same file.
- `methodName.toLowerCase(Locale.ROOT).startsWith("set")` is redundant with
the exact-equality `followsNamePattern` check right below it.
- `expectedFieldNameInASetter` strips a leading `is` unconditionally. That's
correct for `ColumnConfig`'s `isClob` / `isUnicode` / `isEventTimestamp`, which
is presumably why it exists, but a field named e.g. `isolationLevel` would be
told to add `setOlationLevel`. Inherited from 2.x, so not yours — a short
comment would save the next reader.
- The test class is `public class`; `PluginProcessorTest` next to it is
package-private.
- Test names say `warnWhen...` / `ignoreWarning...` / `noWarning...`, but
the diagnostic is an `ERROR`.
- `assertThat(errorDiagnostics).allMatch(x -> !...)` reads more directly as
`noneMatch(...)`.
- No coverage for the other two conditions in `foundPublicSetter`: a setter
that exists but isn't `public`, and one whose return type isn't assignable to
the builder.
### Changelog
No entry needed here, and there isn't one — that's correct. Consistent with
our other 2.x → `main` ports (#4152, #4154, #4157, #4128), none of which added
one: the fix already has its entry on `2.x` and the behaviour has never shipped
in a released 3.x.
One thing to be conscious of: changing `setImmediateFail` /
`setReconnectIntervalMillis` from `void` to `B` is a binary-incompatible
signature change. Since 3.0.0 is unreleased nothing downstream can break, so
I'm not asking for anything — just flagging it as a deliberate call rather than
an accidental one.
---
Item 1 is the only thing I'd consider blocking. Everything else is a
question or a nit, and I'm happy to be talked out of any of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
