Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move validation constraints to log4j-plugins-api #2172

Closed
wants to merge 4 commits into from
Closed

Move validation constraints to log4j-plugins-api #2172

wants to merge 4 commits into from

Conversation

ppkarwasz
Copy link
Contributor

Remark: the target of this PR is a feature branch.

Plugin annotations are employed by log4j-docgen to generate documentation (cf. #1956)

Since the tool is compiled using Java 8 bytecode, we need to split log4j-plugins into:

  • an API part that contains mostly annotations and interfaces,
  • an implementation part that can use all the improvements of Java 17.

In this PR:

  • we move the constraint annotation into a new artifact,
  • we make the value property of the @Constraint annotation optional,
  • we introduce a ConstraintValidatorFactory that produces the appropriate ConstraintValidator for each annotation. This way we don't need to reference ConstraintValidator implementations in the constraint annotations.
  • we add the 2.x constraint annotations that were lost in main.

This PR leaves the o.a.l.l.plugins package split between two artifacts, so it can not be merged to main.

@ppkarwasz
Split validators between l-plugins-api and l-plugins
e6610e8 
Plugin annotations are employed by `log4j-docgen` to generate
documentation.

Since the tool is compiled using Java 8 bytecode, we need to split
`log4j-plugins` into:

 * an API part that contains mostly annotations and interfaces,
 * an implementation part that can use all the improvements of Java 17.

In this commit:

 * we move the constraint annotation into a new artifact,
 * we make the `value` property of the `@Constraint` annotation
   optional,
 * we introduce a `ConstraintValidatorFactory` that produces the
   appropriate `ConstraintValidator` for each annotation. This way
   we don't need to reference `ConstraintValidator` implementations in
   the constraint annotations.
@ppkarwasz
Add old 2.x plugin validation annotations
cfd9f23 
The old `2.x` annotations might still be used by some old plugins.
@ppkarwasz ppkarwasz requested review from jvz and vy and removed request for jvz January 8, 2024 20:30
jvz
jvz reviewed Jan 8, 2024
View reviewed changes
log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/DefaultInstanceFactory.java
Comment on lines +107 to +109
this.bindings.put(
Key.forClass(ConstraintValidatorFactory.class),
() -> new DefaultConstraintValidatorFactory(Integer::valueOf));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this works for now, though we may be able to move most (or all) of these default bindings to a ConfigurableInstanceFactoryPostProcessor as I noted in a todo comment.

jvz
jvz approved these changes Jan 8, 2024
View reviewed changes
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I see some type erasure issues to clean up later, but no big deal.

@vy
Copy link
Member

vy commented Jan 9, 2024 •
edited
Loading

I find the current log4j-plugins et al. extremely huge. We almost re-implemented our own Guice, Dagger, etc. clone. Splitting the API and the implementation would be the cherry on top of this pie. Can't we rather add a

<maven.compiler.release>8</maven.compiler.release>

line to log4j-plugins/pom.xml and be done with it? (We can revert syntactic changes incompatible with Java 8 in log4j-plugins.) @jvz, @ppkarwasz, would you be okay with this one-line resolution?

@ppkarwasz ppkarwasz self-assigned this Jan 9, 2024
@ppkarwasz
Copy link
Contributor Author

From my perspective it is more interesting to check if the API part (interfaces and annotations) do not depend on their implementations. And as it turns out they do.

A use can for an independent log4j-plugins-api would be a RecyclerFactory that only depends on log4j-api and log4j-plugins-api and can still provide a component usable in log4j-core.

Remark: the Java 17 bytecode currently used by log4j-plugins is not a blocker for log4j-docgen. The latter can still be used as an annotation processor to compile Java 8 bytecode if a JDK 17 compiler is used. However many projects still compile Java 8 bytecode using ... JDK 8. ;-)

@vy
Copy link
Member

vy commented Jan 9, 2024

the Java 17 bytecode currently used by log4j-plugins is not a blocker for log4j-docgen

Then this PR simply complicates log4j-plugins more than it already is without any benefit, hence -1 from me.

@ppkarwasz ppkarwasz mentioned this pull request Jan 10, 2024
Closed
@ppkarwasz ppkarwasz mentioned this pull request Jan 17, 2024
Merged
@ppkarwasz ppkarwasz deleted the branch apache:split-plugins-api February 15, 2024 15:25
@ppkarwasz ppkarwasz closed this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvz jvz jvz approved these changes

@vy vy Awaiting requested review from vy

Assignees

@ppkarwasz ppkarwasz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ppkarwasz @vy @jvz