Re: `a.o.l.l.core.parser` package removal

[Piotr P. Karwasz]

Hi,

On Tue, 2 Jan 2024 at 12:08, Piotr P. Karwasz  wrote:
> 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?).

Apparently each time I want to remove something, it turns out it is used:

https://github.com/apache/logging-log4j2/issues/2162

Piotr

Re: `a.o.l.l.core.parser` package removal

[Volkan Yazıcı]

I support the idea of removing the Jackson-based `LogEvent` parser. Back in
the days when `JsonLayout` was the only JSON-formatted layout, it might
have made sense to provide deserialization support next to serialization.
With `JsonTemplateLayout`, where the JSON structure is controlled by the
user, the deserialization companion is not only impossible, but also
doesn't make sense anymore.

@Piotr, do you propose removing it in `2.x` or `main`? My preference would
be both.

On Tue, Jan 2, 2024 at 12:09 PM 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: (logging-log4j2) 01/02: Delete generated module descriptors before recompilation

[Volkan Yazıcı]

> A previous `generate-module-descriptors` execution leaves
> `target/classes/module-info.class` files. These interfere with
> the way the `maven-compiler-plugin` works.

Piotr, could you elaborate on this interference? What exactly is the
problem we are trying to address here?

I have the impression we are working around a `maven-compiler-plugin` bug.
If it is so, is there also an associated `m-c-p` ticket?

On Sat, Dec 30, 2023 at 3:33 PM  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
>
> commit 7117838e7b973c4c150365678149138b642cd672
> Author: Piotr P. Karwasz 
> AuthorDate: Fri Dec 22 12:53:32 2023 +0100
>
> Delete generated module descriptors before recompilation
> ---
>  log4j-parent/pom.xml | 29 +
>  1 file changed, 29 insertions(+)
>
> diff --git a/log4j-parent/pom.xml b/log4j-parent/pom.xml
> index f024dceb36..8866501c47 100644
> --- a/log4j-parent/pom.xml
> +++ b/log4j-parent/pom.xml
> @@ -932,6 +932,35 @@
>
>  
>
> +  
> +  
> +org.apache.maven.plugins
> +maven-clean-plugin
> +
> +  
> +delete-module-descriptors
> +
> +  clean
> +
> +process-sources
> +
> +  true
> +  
> +
> +  ${project.build.outputDirectory}
> +  
> +module-info.class
> +  
> +
> +  
> +
> +  
> +
> +  
> +
>
>  org.apache.maven.plugins
>  maven-surefire-plugin
>
>

Re: `a.o.l.l.core.parser` package removal

[Piotr P. Karwasz]

Hi Volkan,

On Fri, 5 Jan 2024 at 11:25, Volkan Yazıcı  wrote:
> @Piotr, do you propose removing it in `2.x` or `main`? My preference would
> be both.

Unfortunately we never marked it as an internal package, so this would
break binary compatibility.

We should deprecate it in 2.23.0, together with all the classes that
were removed from `log4j-core` in 3.x (that includes those moved to
other modules, since their package name changed).

Piotr

Re: (logging-log4j2) 01/02: Delete generated module descriptors before recompilation

[Piotr P. Karwasz]

Hi Volkan,

On Fri, 5 Jan 2024 at 11:30, Volkan Yazıcı  wrote:
>
> > A previous `generate-module-descriptors` execution leaves
> > `target/classes/module-info.class` files. These interfere with
> > the way the `maven-compiler-plugin` works.
>
> Piotr, could you elaborate on this interference? What exactly is the
> problem we are trying to address here?
>
> I have the impression we are working around a `maven-compiler-plugin` bug.
> If it is so, is there also an associated `m-c-p` ticket?

This might even be a Java compiler bug. I have a minimal reproducible
example of the problem here:

https://github.com/copernik-eu/bug-reproducibility/tree/main/javac-module-info-bug

I'll deal with Oracle's web bug submission process later.

Piotr

Re: `a.o.l.l.core.parser` package removal

[Volkan Yazıcı]

I understand your compatibility concerns, but we are not bound by it. For
instance, in `2.22.0`, we removed `FastDateParser`, which was public too.
If I am not mistaken, `LogEventParser` is not documented anywhere.

All in all, I am fine with either removing or deprecating it.

On Fri, Jan 5, 2024 at 11:47 AM Piotr P. Karwasz 
wrote:

> Hi Volkan,
>
> On Fri, 5 Jan 2024 at 11:25, Volkan Yazıcı  wrote:
> > @Piotr, do you propose removing it in `2.x` or `main`? My preference
> would
> > be both.
>
> Unfortunately we never marked it as an internal package, so this would
> break binary compatibility.
>
> We should deprecate it in 2.23.0, together with all the classes that
> were removed from `log4j-core` in 3.x (that includes those moved to
> other modules, since their package name changed).
>
> Piotr
>

Re: PropertyKey abstraction

[Volkan Yazıcı]

I agree with all points Piotr raises. I am in favor of reverting back to
the system we have in `2.x`, because the new property system has more
disadvantages than benefits in its current state and it hinders the ongoing
operation of sync'ing `2.x` and `main`, and generating API docs from
sources.

Recently I added two new properties to `main` for recyclers. Compared to
`2.x`, I find the new way of doing things unnecessarily complicated,
without any tangible benefit, and undocumented. The new way is also not
practiced everywhere. One can see this from property-related warnings
during build.

The property system rewrite was carried out as a part of the
property-toggled modules – both of which are undocumented. The fact that,
as Piotr suggests, we can revert the property system rewrite yet keep the
property-toggled modules shows that the former could have been addressed in
a separate PR.

I was the one who tipped Spring's @ConfigurationProperties

to Piotr. That is already a well-established practice and compatible with
the existing property system we have in `2.x`. Unless we offer something
better, I don't see a reason why we shouldn't simply reuse it.

On Thu, Jan 4, 2024 at 10:13 AM Piotr P. Karwasz 
wrote:

> Hi all,
>
> Looking at the `PropertyKey` abstraction in `3.x`, I was wondering
> what advantages it brings over simple string constants. From my
> perspective it has more cons than pros.
>
> Pros:
>
>  * it is typesafe; a committer must create an instance of it to use it;
>
> Cons:
>
>  * it is hard to list for documentation purposes. It would be easier
> to document all available properties if they were declared as:
>
> /**
>  * Makes Foo into bars.
>  */
> @Log4jProperty
> private static final String FOO_BAR = "Foo.bar";
>
>   Such a definition can be trivially detected by an annotation
> processor, whereas currently I would need to call `PropertyKey#getKey`
> to even find the name of the property.
>
>  * the split into "component" and "name" isn't really used in real
> code, so `PropertyKey` is basically a wrapper for a single String,
>  * IMHO the `getSystemKey` method does not belong in `PropertyKey`. It
> is something that is used only by those property sources that are
> global in the JVM (like env variables or system properties). Other
> property sources like `TestPropertySource` have no use of it.
>  * since a `PropertyKey` is not a compile-time constant, we have
> several `Constant` classes that contain string constants, so that we
> can use them in tests (e.g. in the `@SetSystemProperty` annotation).
>
> If you don't have any objections, I would prefer to refactor all these
> `PropertyKey` implementations to simple classes that contain only
> constant string fields.
>
> In a **longer** term it would be nice to reintroduce type safety, but
> for the **values** of properties instead of their keys. We could copy
> the way Spring does it, create classes like:
>
> /**
>  * Configures a https://logging.apache.org/log4j/3.x/recycler.html
>  */
> @Log4jPropertySource(prefix = "Recycler")
> public class RecyclerConfig {
>
> /**
>  * Type of recycler.
>  */
> public String type;
>
> /**
>  * Size of the recyclers queue.
>  */
> public int size;
> }
>
> and we could retrieve all the properties at once with
> `PropertyEnvironment#getProperties(Recycler.class)`.
>
> What do you think?
>