Nullability annotations
Hi all, Matt made an interesting proposal to use JSpecify nullability annotations in Log4j: https://github.com/apache/logging-parent/issues/88 I am a big fan of nullability annotations but in my professional experience they are worthless, unless the whole team agrees to use them and how to introduce them. IMHO we should: * create a branch that will differ from `main` only for the present of nullability annotations, * depend directly (scope `provided`) on JSpecify, although this might cause issue reports like this one [1], * only merge the changes of one module at a time (starting with `log4j-api`), * start by marking all fields and return types as `@Nullable` and all parameters as `@NonNull`, * only merge the change if a tool (NullAway/Checker Framework) guarantees that the code is NPE-free. * target 3.1.x as the first release with nullability annotations. What do you think? Piotr [1] https://github.com/apache/logging-log4j2/issues/2144
Re: Nullability annotations
If this is a runtime dependency then I am against using it in Log4J api and core. Ralph > On Jan 2, 2024, at 3:17 AM, Piotr P. Karwasz wrote: > > Hi all, > > Matt made an interesting proposal to use JSpecify nullability > annotations in Log4j: > > https://github.com/apache/logging-parent/issues/88 > > I am a big fan of nullability annotations but in my professional > experience they are worthless, unless the whole team agrees to use > them and how to introduce them. > > IMHO we should: > > * create a branch that will differ from `main` only for the present > of nullability annotations, > * depend directly (scope `provided`) on JSpecify, although this might > cause issue reports like this one [1], > * only merge the changes of one module at a time (starting with `log4j-api`), > * start by marking all fields and return types as `@Nullable` and all > parameters as `@NonNull`, > * only merge the change if a tool (NullAway/Checker Framework) > guarantees that the code is NPE-free. > * target 3.1.x as the first release with nullability annotations. > > What do you think? > > Piotr > > [1] https://github.com/apache/logging-log4j2/issues/2144
`a.o.l.l.core.parser` package removal
Hi, While working on PR#2142[1] I noticed that we have an `a.o.l.l.core.parser` package that depends on Jackson. Since Log4j itself never parses log events, I would propose to remove it from `log4j-core` and optionally move it somewhere else (Chainsaw or Flume?). My main concern is vulnerability exposure: * I would like to prevent CVEs like CVE-2019-17571[2] from being published against `log4j-core` in the future. Dealing with CVEs that say "code that we never use is vulnerable to..." bring a lot of useless PR/documentation work: we'll need to explain to users how to mitigate a vulnerability that is almost never exploitable and our users will also have to evaluate the exploitability of the CVE in their own applications, * in some not so far future we'll need to publish VEX records to comply with regulation. Every time Jackson will publish a deserialization vulnerability, we'll need to state that we are vulnerable. What do you think? Piotr [1] https://github.com/apache/logging-log4j2/pull/2142 [2] https://www.cvedetails.com/cve/CVE-2019-17571/
Re: Nullability annotations
Hi Ralph, On Tue, 2 Jan 2024 at 11:59, Apache wrote: > > If this is a runtime dependency then I am against using it in Log4J api and > core. It is an annotation-only library, with a retention of `RUNTIME`. However annotations should not cause `NoClassDefFoundError`s. Piotr
Re: Nullability annotations
On Tue, Jan 2, 2024 at 1:00 PM Piotr P. Karwasz wrote: > > Hi Ralph, > > On Tue, 2 Jan 2024 at 11:59, Apache wrote: > > > > If this is a runtime dependency then I am against using it in Log4J api and > > core. > > It is an annotation-only library, with a retention of `RUNTIME`. > However annotations should not cause `NoClassDefFoundError`s. I would like to confirm that from experiments in Apache Commons. Still don't understand, why, but such runtime dependencies really aren't missing at runtime. Or, in other words: Plain users won't be bothered. Jochen -- The woman was born in a full-blown thunderstorm. She probably told it to be quiet. It probably did. (Robert Jordan, Winter's heart)
Re: `a.o.l.l.core.parser` package removal
Why is it included if it isn’t used? Ralph > On Jan 2, 2024, at 4:09 AM, Piotr P. Karwasz wrote: > > Hi, > > While working on PR#2142[1] I noticed that we have an > `a.o.l.l.core.parser` package that depends on Jackson. > > Since Log4j itself never parses log events, I would propose to remove > it from `log4j-core` and optionally move it somewhere else (Chainsaw > or Flume?). > > My main concern is vulnerability exposure: > > * I would like to prevent CVEs like CVE-2019-17571[2] from being > published against `log4j-core` in the future. Dealing with CVEs that > say "code that we never use is vulnerable to..." bring a lot of > useless PR/documentation work: we'll need to explain to users how to > mitigate a vulnerability that is almost never exploitable and our > users will also have to evaluate the exploitability of the CVE in > their own applications, > * in some not so far future we'll need to publish VEX records to > comply with regulation. Every time Jackson will publish a > deserialization vulnerability, we'll need to state that we are > vulnerable. > > What do you think? > > Piotr > > [1] https://github.com/apache/logging-log4j2/pull/2142 > [2] https://www.cvedetails.com/cve/CVE-2019-17571/
Re: Nullability annotations
Nullability annotations are trivial to remove. I’ve added some basic aliases for them in main. As it stands, they’re copies of the four JSpecify annotations with those annotations applied to them along with equivalent JSR 305 meta-annotations to make the annotations function the same in existing tooling. The annotations are in log4j-api (in a new package that hasn’t been published before), and the JSR and JSpecify annotations are explicitly excluded from the OSGi and JPMS module imports to ensure they’re optional (being annotations after all). I’ve annotated a few things, but before bothering with more widespread updates, I figure we should discuss what to do there before spending a lot of time updating things. The annotation dependencies are all “provided” scope (we have some of these already there for SpotBugs-related things). If we want to reuse the same thing across components, though, then we’d likely want a separate jar for this (or to directly use JSpecify as said jar). I personally like and use nullability annotations wherever possible in Spinnaker as we use a combination of Java, Kotlin, and other languages, and Kotlin has built-in support for various nullability annotations to bridge over to its nullable-based type system (i.e., Kotlin can differentiate between types like `List?`, `List`, and `List?`, which would correspond to Java versions like `@Nullable List`, `List<@Nullable T>`, and `@Nullable List<@Nullable T>` respectively). By adopting the JSpecify convention, it’s easy to mark either a package or class as `@NullUnmarked` which means everything in scope is non-null unless otherwise annotated as `@Nullable`. The branch approach to adding this is extremely likely to end up in merge hell after short order. I’ve had this happen to my own branches where I tried to incrementally add nullability annotations. What I’d suggest is ensuring everything in log4j-api is properly annotated first, then the major plugin and core APIs/SPIs, and then the rest. Then we can take advantage of the static analysis frameworks as a regular check to ensure no new NPEs are introduced. > On Jan 2, 2024, at 4:17 AM, Piotr P. Karwasz wrote: > > Hi all, > > Matt made an interesting proposal to use JSpecify nullability > annotations in Log4j: > > https://github.com/apache/logging-parent/issues/88 > > I am a big fan of nullability annotations but in my professional > experience they are worthless, unless the whole team agrees to use > them and how to introduce them. > > IMHO we should: > > * create a branch that will differ from `main` only for the present > of nullability annotations, > * depend directly (scope `provided`) on JSpecify, although this might > cause issue reports like this one [1], > * only merge the changes of one module at a time (starting with `log4j-api`), > * start by marking all fields and return types as `@Nullable` and all > parameters as `@NonNull`, > * only merge the change if a tool (NullAway/Checker Framework) > guarantees that the code is NPE-free. > * target 3.1.x as the first release with nullability annotations. > > What do you think? > > Piotr > > [1] https://github.com/apache/logging-log4j2/issues/2144
Re: (logging-log4j2) branch main updated: Fix compilation error
Thanks for fixing this. This formatting change got swept up in a branch of mine
you already reviewed.
> On Jan 2, 2024, at 10:10 AM, pkarw...@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> pkarwasz pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new 4789503a4e Fix compilation error
> 4789503a4e is described below
>
> commit 4789503a4e31268c7a523cea7be09a53a836a9c8
> Author: Piotr P. Karwasz
> AuthorDate: Tue Jan 2 17:09:50 2024 +0100
>
>Fix compilation error
> ---
> .../test/java/org/apache/logging/log4j/mongodb4/MongoDb4Resolver.java | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/log4j-mongodb4/src/test/java/org/apache/logging/log4j/mongodb4/MongoDb4Resolver.java
>
> b/log4j-mongodb4/src/test/java/org/apache/logging/log4j/mongodb4/MongoDb4Resolver.java
> index 03ca4188b4..640b1e8489 100644
> ---
> a/log4j-mongodb4/src/test/java/org/apache/logging/log4j/mongodb4/MongoDb4Resolver.java
> +++
> b/log4j-mongodb4/src/test/java/org/apache/logging/log4j/mongodb4/MongoDb4Resolver.java
> @@ -121,7 +121,7 @@ public class MongoDb4Resolver extends
> TypeBasedParameterResolver im
> @Override
> public MongoClient resolveParameter(ParameterContext parameterContext,
> ExtensionContext extensionContext)
> throws ParameterResolutionException {
> -return
> ExtensionContextAnchor.getRequiredAttribute(MongoClientHolder.class,
> MongoClientHolder.class, extensionContext)
> +return ExtensionContextAnchor.getAttribute(MongoClientHolder.class,
> MongoClientHolder.class, extensionContext)
> .get();
> }
>
>
Re: `a.o.l.l.core.parser` package removal
Hi Ralph, On Tue, 2 Jan 2024 at 13:21, Ralph Goers wrote: > > Why is it included if it isn’t used? Apparently this was added by Mikael in: https://issues.apache.org/jira/browse/LOG4J2-1986 There were no significant changes/bug fixes after the initial submission. Piotr
Re: `a.o.l.l.core.parser` package removal
Yeah - Given that ticket it is obvious to me that this class should be moved to another module. I don’t think moving it to Chainsaw or Flume is the correct thing to do. For now, I would put it into log4j-samples, either under log4j-server or in its own module. Note that a Flume event consists of a Map of “headers” and a body. The body is the result of calling layout.getFormattedMessage() and the headers generally would contain values from the ThreadContextMap as well as fields from the Map if the LogEvent contains a MapMessage. So in many cases parsing the message isn’t necessary since the fields are usually already available as headers. Ralph > On Jan 2, 2024, at 3:30 PM, Piotr P. Karwasz wrote: > > Hi Ralph, > > On Tue, 2 Jan 2024 at 13:21, Ralph Goers wrote: >> >> Why is it included if it isn’t used? > > Apparently this was added by Mikael in: > https://issues.apache.org/jira/browse/LOG4J2-1986 > > There were no significant changes/bug fixes after the initial submission. > > Piotr
