CAMEL-22640: Add NullAway validation profile and fix null annotations in camel-api#23021
CAMEL-22640: Add NullAway validation profile and fix null annotations in camel-api#23021gnodet wants to merge 15 commits into
Conversation
Add @NullMarked (JSpecify 1.0.0) to all packages in camel-api, making non-null the default. Annotate ~200 API interfaces and classes with @nullable where parameters or return values can legitimately be null, covering the core API surface including CamelContext, Exchange, Message, Component, Endpoint, TypeConverter, and all SPI interfaces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Scan actual implementations (not just Javadoc) for return null, null field values, and caller null-checks. Adds missing @nullable annotations and moves @NullMarked to package-level only. Key additions found by implementation scanning: - Exchange.getPattern() can be null (field uninitialized) - Message.getExchange() can be null (field starts null) - Route.getOnException/navigate() can return null - DelegateProcessor.getProcessor() can be null (LazyStartProducer) - Various transient exception fields nullable after deserialization - ConsumerTemplate.receiveBody() without timeout can return null - CamelContext.getComponent() overloads can return null - ExtendedCamelContext.getErrorHandlerFactory() can be null - Multiple SPI methods found nullable through impl analysis Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CamelExchangeException: accept @nullable exchange (field already nullable) - CamelExecutionException: accept @nullable exchange (called with null from ExchangeHelper) - ExchangeTimedOutException: accept @nullable exchange (consistent with parent) - Transformer: setFrom/setTo accept @nullable (fields already nullable, called with null from reifiers) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…null-annotations # Conflicts: # core/camel-api/src/main/java/org/apache/camel/spi/Transformer.java # core/camel-api/src/main/java/org/apache/camel/vault/AwsVaultConfiguration.java # core/camel-api/src/main/java/org/apache/camel/vault/AzureVaultConfiguration.java # core/camel-api/src/main/java/org/apache/camel/vault/CyberArkVaultConfiguration.java # core/camel-api/src/main/java/org/apache/camel/vault/GcpVaultConfiguration.java # core/camel-api/src/main/java/org/apache/camel/vault/HashicorpVaultConfiguration.java # core/camel-api/src/main/java/org/apache/camel/vault/IBMSecretsManagerVaultConfiguration.java # core/camel-api/src/main/java/org/apache/camel/vault/SpringCloudConfigConfiguration.java # parent/pom.xml
…null-annotations # Conflicts: # core/camel-api/src/main/java/org/apache/camel/NamedNode.java # core/camel-api/src/main/java/org/apache/camel/spi/ModelDumpLine.java
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add nullcheck Maven profile with Error Prone + NullAway (JDK 21+) - Fix 134 NullAway warnings in camel-api annotations - Make HasCamelContext.getCamelContext() return @nullable - Fix BulkTypeConverters to match TypeConverter interface nullability - Fix JSSE classes null safety (BaseSSLContextParameters, etc.) - Add null guards for @nullable dereferences - Update Velocity template for generated code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…validation # Conflicts: # core/camel-api/src/main/java/org/apache/camel/CamelContext.java # core/camel-api/src/main/java/org/apache/camel/package-info.java
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Remove JDK activation so the profile only activates via -Pnullcheck, not automatically on JDK 21+ CI builds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sync with origin/main and fix fully-qualified TimeUnit import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I assume you can close your old PR |
|
There are uncommitted changes |
Replace java.util.concurrent.TimeUnit.SECONDS with imported TimeUnit to pass the OpenRewrite uncommitted-changes CI check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
It may be good to add a note in the 4.21 upgrade guide that camel-api has dependency on jspecify JAR |
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
last release of jspecify was 2 years ago.
Is the benefit really that huge to introduce a dependency on a project which doesn't seem very active?
- Mark jspecify dependency as <optional>true</optional> in camel-api so it is not pulled transitively into consumer projects - Add note to 4.21 upgrade guide about the new jspecify dependency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@davsclaus Good point — I've added a note to the 4.21 upgrade guide and also made the jspecify dependency Claude Code on behalf of Guillaume Nodet |
|
@apupier JSpecify 1.0.0 is a stable release — the project only contains 4 annotation types (`@Nullable`, `@NonNull`, `@NullMarked`, `@NullUnmarked`), so there's very little reason for frequent releases. It's backed by Google, JetBrains, and others, and is becoming the de facto standard for null annotations in the Java ecosystem (supported by NullAway, Error Prone, Kotlin, IntelliJ, etc.). That said, I've made the dependency `true` so it's not pulled transitively into consumer projects. Annotations gracefully degrade if the JAR isn't on the classpath — they simply become invisible to reflection. Claude Code on behalf of Guillaume Nodet |
|
🧪 CI tested the following changed modules:
Build reactor — dependencies compiled but only changed modules were tested (4 modules)
|
Ensures jspecify annotations are on the classpath for downstream modules when compiling with -Pnullcheck. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| public OAuthClientConfig setClientSecret(String clientSecret) { | ||
| this.clientSecret = clientSecret; | ||
| this.clientSecret = Objects.requireNonNull(clientSecret, "clientSecret"); | ||
| return this; |
There was a problem hiding this comment.
the clientSecret is nullable, why do we requireNonNull?
There was a problem hiding this comment.
For this constructor, yes. But the no-arg constructor does not set the clientSecret field, which ends up being null, hence the @nullable.
|
|
||
| protected Transformer(String name) { | ||
| this.name = name; | ||
| this.name = Objects.requireNonNull(name, "name"); |
There was a problem hiding this comment.
the name is marked as Nullable but here there is a requireNonNull
There was a problem hiding this comment.
For this constructor, yes. But the no-arg constructor does not set the name field, which ends up being null, hence the @Nullable.
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
most of (all?) requireNonNullMessage are not giving any information. Either remove them or provide a useful message.
To be honest, I feel like this is the kind of changes that shouldn't be done in a bulk change and reviewed carefully at each place. I checked only few files and found inconsistencies
This PR does not modify current semantics. So most fields are annotated with I could improve the |
…validation # Conflicts: # core/camel-api/src/main/java/org/apache/camel/NamedRoute.java
| public abstract class ResumeStrategyConfiguration { | ||
| private Cacheable.FillPolicy cacheFillPolicy; | ||
| private ResumeCache<?> resumeCache; | ||
| private Cacheable.@Nullable FillPolicy cacheFillPolicy; |
| * @return the fill policy to use | ||
| */ | ||
| public Cacheable.FillPolicy getCacheFillPolicy() { | ||
| public Cacheable.@Nullable FillPolicy getCacheFillPolicy() { |
|
|
||
| @Override | ||
| default <T> T mandatoryConvertTo(Class<T> type, Object value) | ||
| default <T> T mandatoryConvertTo(Class<T> type, @Nullable Object value) |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
|
|
||
| @Override | ||
| default <T> T convertTo(Class<T> type, Exchange exchange, Object value) throws TypeConversionException { | ||
| default <T> @Nullable T convertTo(Class<T> type, @Nullable Exchange exchange, @Nullable Object value) |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
|
|
||
| @Override | ||
| default <T> T convertTo(Class<T> type, Object value) throws TypeConversionException { | ||
| default <T> @Nullable T convertTo(Class<T> type, @Nullable Object value) throws TypeConversionException { |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
| @SuppressWarnings("NullAway") | ||
| default <T> T mandatoryConvertTo(Class<?> from, Class<T> to, @Nullable Exchange exchange, @Nullable Object value) | ||
| throws TypeConversionException, NoTypeConversionAvailableException { | ||
| Object t = convertTo(from, to, exchange, value); |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
|
|
||
| @Override | ||
| default <T> T tryConvertTo(Class<T> type, Object value) { | ||
| default <T> @Nullable T tryConvertTo(Class<T> type, @Nullable Object value) { |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
|
|
||
| @Override | ||
| default <T> T tryConvertTo(Class<T> type, Exchange exchange, Object value) { | ||
| default <T> @Nullable T tryConvertTo(Class<T> type, @Nullable Exchange exchange, @Nullable Object value) { |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
| * @return the converted value, or <tt>null</tt> if not possible to convert | ||
| */ | ||
| default <T> T tryConvertTo(Class<?> from, Class<T> to, Exchange exchange, Object value) throws TypeConversionException { | ||
| default <T> @Nullable T tryConvertTo(Class<?> from, Class<T> to, @Nullable Exchange exchange, @Nullable Object value) |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
| * @throws TypeConversionException is thrown if error during type conversion | ||
| */ | ||
| <T> T convertTo(Class<?> from, Class<T> to, Exchange exchange, Object value) throws TypeConversionException; | ||
| <T> @Nullable T convertTo(Class<?> from, Class<T> to, @Nullable Exchange exchange, @Nullable Object value) |
There was a problem hiding this comment.
the value is not null - you cannot converter a null value so this should be changed
| class Builder<T extends Component> { | ||
| private final Class<T> type; | ||
| private BiPredicate<String, Component> condition; | ||
| private @Nullable BiPredicate<String, Component> condition; |
There was a problem hiding this comment.
why is this nullable. The point of the customizer is to call the condition
| class Builder<T extends DataFormat> { | ||
| private final Class<T> type; | ||
| private BiPredicate<String, DataFormat> condition; | ||
| private @Nullable BiPredicate<String, DataFormat> condition; |
There was a problem hiding this comment.
same here as last comment
| void onThreadPoolAdd( | ||
| CamelContext camelContext, ThreadPoolExecutor threadPool, String id, | ||
| String sourceId, String routeId, String threadPoolProfileId); | ||
| CamelContext camelContext, ThreadPoolExecutor threadPool, @Nullable String id, |
There was a problem hiding this comment.
we should likely enforce that some of these are not null as creating a thread pool we should have id / source id to identify it.
| default void onThreadPoolAdd( | ||
| CamelContext camelContext, ExecutorService executorService, String id, | ||
| String sourceId, String routeId, String threadPoolProfileId) { | ||
| CamelContext camelContext, ExecutorService executorService, @Nullable String id, |
There was a problem hiding this comment.
we should likely enforce that some of these are not null as creating a thread pool we should have id / source id to identify it.
3 participants
CAMEL-22640
Summary
Add comprehensive JSpecify null safety annotations to
camel-api, and a NullAway compile-time validation profile to enforce them.Changes
JSpecify null annotations on
camel-api(283 files)org.jspecify:jspecifydependency tocamel-apicamel-apias@NullMarkedviapackage-info.javafiles (org.apache.camel,o.a.c.spi,o.a.c.vault,o.a.c.support.jsse,o.a.c.health,o.a.c.resume, etc.)@Nullableannotations across the public API surface (interfaces, abstract classes, exception constructors, return types, method parameters) to accurately document nullability contractsCamelContext— nullable returns forgetComponent(),hasComponent(),getRoute(),getProcessor(),resolveDataFormat(),resolveTransformer(),resolveValidator(),getVariable(),getGlobalOption(), etc.Exchange/Message— nullable bodies, headers, propertiesAggregationStrategy— nullable old/new exchange parameters and returnspi.*— nullable returns on resolvers, factories, registries, repositoriesvault.*— nullable optional configuration fieldsresume.*— nullable serialization results@Nullablevalues are dereferenced (e.g.,HealthCheckHelper,RoutesLoader,GroovyScriptCompiler, JSSE parameter classes)ExchangeConstantProviderto include@NullableNullAway validation profile (
parent/pom.xml)nullcheckMaven profile using NullAway via Error Prone@NullMarkedpackages--add-exportsflags)mvn install -Pnullcheck -DskipTestsTest fix
AggregateProcessorTestto match updatedAggregationStrategysignatureVerification