ramanathan1504 commented on PR #4153:
URL: https://github.com/apache/logging-log4j2/pull/4153#issuecomment-5128109091
Thanks @vpelikh, and sorry for the slow turnaround. I checked this out
locally and both of my asks are done:
- @JWT007's four commits are here with authorship intact.
- All three review threads on #3510 are addressed. I pulled every thread
(including resolved/outdated) to be sure I wasn't missing any — there are
exactly three, and each is handled.
On the misspelled-plugin point in particular, I wanted to confirm it was
fixed for the right reason rather than by coincidence, so I ran the test and
checked the status logger output:
```
ERROR No text provided for StringMatchFilter: text
ERROR Could not configure plugin element StringMatchFilter:
ConstraintValidationException: Validation failed for text
(source: private java.lang.String
org.apache.logging.log4j.core.filter.StringMatchFilter$Builder.text)
```
That is `StringMatchFilter`'s own `@Required` firing, which is exactly what
@ppkarwasz was asking for. `StringMatchFilterTest` passes 6/6 and
`spotless:check` is clean.
Two things I would like changed before merge, and I think both of them make
the PR *smaller*.
---
### 1. Please drop the changelog entry
We don't add changelog entries in `main` for fixes ported from `2.x`. The
four most recent ports all added zero changelog files:
- `4dee591` Port `InternalLoggerRegistry` (#4157)
- `554d8f9` Fix RegexFilter NPE when `useRawMsg` is null (#3265 port) (#4152)
- `463f9c0` Reopen log file when rollover is unsuccessful (#4154)
- `3e0b07f` Port `NamedInstantPattern` (#4128)
`#4152` is the closest analogue — same `core.filter` package, same "port a
2.x NPE fix" shape, merged just before this one, no entry.
The reason is that the fix already ships in
`src/changelog/2.25.0/3153_fix_StringMatchFilter_guardNPE.xml` on `2.x`, and
this bug has never appeared in a released `3.x` — 3.0.0 isn't out yet. There is
nothing for the 3.0.0 release notes to announce as "fixed"; the code is simply
correct on first release. That's also why `main` has no `.2.x.x` directory:
`.3.x.x` is reserved for changes that are genuinely 3.x-only.
So `src/changelog/.3.x.x/3509_fix_StringMatchFilter_Validation.xml` can go.
### 2. Please restore `ATTR_MATCH` and `setMatchString`
The diff removes `public static final String ATTR_MATCH` and renames
`setMatchString(String)` to `setText(String)` with no deprecated alias. Neither
is required to make the port work on `3.x`, and the `2.x` change this ports
from deliberately did neither — it kept `ATTR_MATCH`, and kept `setMatchString`
as `@Deprecated since 2.25.0` alongside the new `setText`:
```java
/**
* @deprecated since 2.25.0, use {@link #setText} instead.
*/
@Deprecated
public StringMatchFilter.Builder setMatchString(final String text) { ... }
```
This is the kind of change to existing code @vy asked us to keep out of
feature PRs. It also leaves `StringMatchFilter` inconsistent with
`LevelMatchFilter`, which still has `ATTR_MATCH`. Nothing in-tree references
either symbol, so there's no build breakage — it's a downstream-API and
release-notes concern.
To be clear about where I think the line is, these adaptations are all
correct and I'm not asking you to touch them:
- `core.util.Assert` → `plugins.util.Assert` (no `core.util.Assert` in
`main`)
- the `@Required` import path
- `core.util.Builder` → `plugins.util.Builder` (`core.util.Builder` is
`@deprecated` in `main` — "Present only for compatibility with Log4j 2 2.x
plugins")
- the explicit `text == null` check in `build()` in place of `2.x`'s
`isValid()` (`main`'s `AbstractFilterBuilder` has no `isValid()`)
- the jspecify `@NonNull` annotation
Those are genuine 3.x adaptations. The API removal isn't.
If you'd rather keep the removal, it fits naturally in the follow-up PR you
already planned for the `equalsImpl`/`Assert` refactors — with its own
`type="removed"` changelog entry there, since *that* would be a real 3.x-only
user-visible change.
With both of these applied the PR is a faithful port in four files.
---
### PR description
The description says the tests cover "the post-`stop()` guard", but there's
no such test — the six are builder-ok, `build()` with unset text,
`setText(null)` → NPE, `setText("")` → IAE, config-ok and config-nok. The "Why"
section also attributes the bug to an NPE "after `stop()`", but the `filter()`
bodies were reverted and are unchanged here, so nothing about `stop()` is
touched.
That's fine as a scoping decision, but since #3509 is titled "guard
potential NPEs **and** builder validation/API", could you state explicitly in
the description whether the NPE-guard half is deferred? Otherwise it looks like
this PR closes #3509 when it only covers half of it.
### Minor
- `testFilterBuilderFailsWithNullText` actually tests *unset* text, not
`null` — it reads confusingly right next to
`testFilterBuilderFailsWithExceptionOnNullText`. Maybe
`testFilterBuilderFailsWithUnsetText`.
- The test class mixes static imports (`assertNull`, `assertNotNull`) with
qualified calls (`Assertions.assertNull`, `Assertions.assertThrows`). Worth
picking one.
- The two test fixtures are named `-3153-` (the 2.x issue) while the 3.x
issue is #3509. Harmless, just slightly confusing to a future reader.
- Behaviour change worth keeping in mind: `<StringMatchFilter/>` with no
`text` previously defaulted to `""` and matched every message via
`contains("")`. It's now dropped with an error. That's the intended fix and I
agree with it — flagging only so it's a conscious call.
---
@ppkarwasz — one thread on #3510 ended on your question about whether to
change `Assert.requireNonEmpty` itself. @vpelikh resolved it at the call site
instead (`Objects.requireNonNull` then `Assert.requireNonEmpty`), which keeps
the null/empty distinction you wanted without touching the shared utility. That
seems right to me given @vy's guidance, but it's your thread — happy to defer
if you'd still prefer changing `Assert`.
I'll close #3510 and resolve its threads once this lands.
--
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]
