(chores): fix SonarCloud S1612 across multiple modules#22304
(chores): fix SonarCloud S1612 across multiple modules#22304orpiske wants to merge 1 commit intoapache:mainfrom
Conversation
Replace lambdas with method references where applicable (S1612). Covers core, components, dsl, test-infra, and tooling modules. Some cases involving assertDoesNotThrow ambiguity or Camel DSL body() method overloads were left as lambdas since method references cause type inference ambiguity in those contexts.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Review: (chores): fix SonarCloud S1612 across multiple modules
Summary
All ~220 lambda-to-method-reference conversions were individually verified and are semantically correct — no behavioral changes, no type inference ambiguity, and no checked exception issues. The refactoring is well-executed overall.
However, the PR includes unrelated changes in a generated file that must be addressed before merging.
Detailed Analysis
Correctness: Every method reference was checked against its original lambda for semantic equivalence. Key patterns verified:
x instanceof Type→Type.class::isInstance: all target types match the originalinstanceofchecks() -> obj.method()→obj::methodinassertThrows/assertDoesNotThrow: all valid —Executabledeclaresthrows Throwable, so checked-exception methods are compatible(carrier, key, value) -> carrier.put(key, value)→SpanContextPropagationInjector::putin tracing code: correctly resolved as unbound instance method reference matchingTextMapSetter<SpanContextPropagationInjector>- Constructor references (
SSLContextParameters::new,SalesforceEndpointConfig::new): all no-arg, safe - All 9 newly added imports are used and correct
Thread safety: No concerns — all conversions are purely syntactic.
Test coverage: This is a refactoring PR with no behavioral changes; existing tests provide adequate coverage.
Issues Found
| Severity | Issue |
|---|---|
| Blocking | camelYamlDsl.json contains ~1,500 lines of unrelated generated schema changes (see inline) |
| Minor | Comparable::compareTo should be String::compareTo in ZooKeeperGroup.java (see inline suggestion) |
| Nit | Several explanatory comments silently removed in Kafka/Elasticsearch tests (see inline) |
Claude Code on behalf of Guillaume Nodet
| } | ||
| } ] | ||
| }, | ||
| "org.apache.camel.model.SagaActionUriDefinition" : { |
There was a problem hiding this comment.
Blocking: This generated file contains ~1,500 lines of changes that are not part of the S1612 lambda-to-method-reference refactoring:
- Removal of
groovyJson/GroovyJSonDataFormatentries (3 locations) - Addition of
SagaActionUriDefinition(new schema definition, shown here) SagaDefinition.compensationandcompletionchanged fromtype: stringto$ref: SagaActionUriDefinition- Addition of
org.apache.camel.model.app.*andorg.apache.camel.model.cloud.*definitions WireTapDefinitionschema expansion/rewrite
These are regeneration artifacts from building against a branch state that has diverged from main. Including unrelated generated file changes in a focused refactoring PR makes review harder and risks sneaking in unintended schema changes.
Please revert this file:
git checkout main -- dsl/camel-yaml-dsl/camel-yaml-dsl/src/generated/resources/schema/camelYamlDsl.jsonOr regenerate it cleanly against the PR's target branch.
| ensurePath.ensure(client.getZookeeperClient()); | ||
| List<String> children = client.getChildren().usingWatcher(childrenWatcher).forPath(path); | ||
| children.sort((String left, String right) -> left.compareTo(right)); | ||
| children.sort(Comparable::compareTo); |
There was a problem hiding this comment.
Minor: Comparable::compareTo uses a raw type method reference. Since children is List<String>, String::compareTo is more precise and avoids potential unchecked/raw type warnings.
| children.sort(Comparable::compareTo); | |
| children.sort(String::compareTo); |
| // retry the message with an error | ||
| doCommitOffset(exchange); | ||
| }); | ||
| .process(KafkaBreakOnFirstErrorWithBatchUsingKafkaManualCommitIT.this::doCommitOffset); |
There was a problem hiding this comment.
Nit: The lambda-to-method-reference conversion here also removed an explanatory comment that documented why the manual commit is needed:
// if we don't commit
// camel will continuously
// retry the message with an error
Similar comment removals occur in:
ElasticsearchRestClientComponentIT.java("Assert that the direct:get-by-id endpoint received the exception")KafkaBatchingProcessingBreakOnFirstErrorManualCommitIT.java("Manual commit on success")
These comments explain intent, not implementation, and remain valuable even with method references. Consider preserving them above the .process() call:
// if we don't commit, camel will continuously retry the message with an error
.process(KafkaBreakOnFirstErrorWithBatchUsingKafkaManualCommitIT.this::doCommitOffset);2 participants
Summary
Claude Code on behalf of Otavio Piske
Details
SonarCloud rule S1612 flags lambdas that can be replaced with method references for improved readability. Examples:
x -> x.method()→Type::method() -> obj.method()→obj::methodx -> x instanceof Type→Type.class::isInstance() -> new Type()→Type::newA few cases were intentionally left as lambdas where method references cause type inference ambiguity (e.g.,
assertDoesNotThrowwith methods that throw checked exceptions, or Camel DSLbody()overloads).Test plan