Repository navigation
camel-aws2-transcribe: deprecate the protocol option, which has no effect - #29706
Dhairya-007 wants to merge 4 commits into
Conversation
…fect Transcribe2Configuration declares a protocol option that nothing reads. The Transcribe client is built by the shared AwsClientBuilderUtil, which takes the region, the endpoint override and the proxy settings off AwsCommonConfiguration; protocol is not part of that interface, and getProtocol() is called only by the generated configurer. Setting protocol=HTTP on an aws2-transcribe endpoint is accepted and does nothing. Its description made that worse: the setter carried the javadoc of setProxyProtocol, "To define a proxy protocol when instantiating the Transcribe client", so the option read as if it configured the proxy - which is what proxyProtocol, three fields below, actually does. Deprecate it with a note, as camel-docling's apiKeyHeader was deprecated in this release, and give it a description that no longer claims to configure a proxy. The catalog metadata, both DSL builder factories and the 4.24 upgrade guide follow.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The protocol field is correctly annotated with @Deprecated and @Metadata(deprecationNote=...), but the getter and setter methods are missing the same deprecation markers. Per Camel convention (see e.g. AWS2S3VectorsConfiguration), both the accessor methods should carry @Deprecated and a @deprecated Javadoc tag.
Additionally, the new Javadoc text "The protocol to use when instantiating the Transcribe client" is misleading for a no-op option — it implies the value is actually used. Consider something like "The protocol to use when instantiating the Transcribe client (deprecated, this option has no effect)" or simply retain the original wording with a deprecation note.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…pache/camel/component/aws2/transcribe/Transcribe2Configuration.java Co-authored-by: Guillaume Nodet - AI Bot <gnodet+bot@gmail.com>
…pache/camel/component/aws2/transcribe/Transcribe2Configuration.java Co-authored-by: Guillaume Nodet - AI Bot <gnodet+bot@gmail.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the @Deprecated/@deprecated finding has been addressed. One issue remains.
The fix introduced a duplicate Javadoc block on both the getter and setter: each method now has two consecutive /** ... */ blocks immediately before it. The Java language and all Javadoc tooling only recognize the last Javadoc comment immediately preceding a declaration — the first block is an orphan that will silently be ignored. The resulting class-level documentation will show just the second block, but the stray first block is misleading noise in the source.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| /** | ||
| * To define a proxy protocol when instantiating the Transcribe client | ||
| * The protocol to use when instantiating the Transcribe client | ||
| */ | ||
| /** | ||
| * The protocol to use when instantiating the Transcribe client | ||
| * | ||
| * @deprecated Not used: nothing reads this option when the Transcribe client is built | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
/** ... */ comments before getProtocol(). Java only associates the last Javadoc block immediately preceding a declaration — the first one is a stray orphan that will be silently ignored by javadoc and IDEs. Remove it and keep only the second block (the one with @deprecated).
| /** | |
| * To define a proxy protocol when instantiating the Transcribe client | |
| * The protocol to use when instantiating the Transcribe client | |
| */ | |
| /** | |
| * The protocol to use when instantiating the Transcribe client | |
| * | |
| * @deprecated Not used: nothing reads this option when the Transcribe client is built | |
| */ | |
| @Deprecated | |
| /** | |
| * The protocol to use when instantiating the Transcribe client. | |
| * | |
| * @deprecated Not used: nothing reads this option when the Transcribe client is built | |
| */ | |
| @Deprecated |
| /** | ||
| * To define a proxy protocol when instantiating the Transcribe client | ||
| * The protocol to use when instantiating the Transcribe client | ||
| */ | ||
| /** | ||
| * The protocol to use when instantiating the Transcribe client | ||
| * | ||
| * @deprecated Not used: nothing reads this option when the Transcribe client is built | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
getProtocol(): two consecutive /** ... */ before setProtocol(). Remove the stray first block.
| /** | |
| * To define a proxy protocol when instantiating the Transcribe client | |
| * The protocol to use when instantiating the Transcribe client | |
| */ | |
| /** | |
| * The protocol to use when instantiating the Transcribe client | |
| * | |
| * @deprecated Not used: nothing reads this option when the Transcribe client is built | |
| */ | |
| @Deprecated | |
| /** | |
| * The protocol to use when instantiating the Transcribe client. | |
| * | |
| * @deprecated Not used: nothing reads this option when the Transcribe client is built | |
| */ | |
| @Deprecated |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: correct deprecation of a dead option, one leftover from the suggestion commits to clean up.
I checked that protocol is really unused: apart from the generated Transcribe2EndpointConfigurer/Transcribe2ComponentConfigurer, nothing calls getProtocol(), and Transcribe2ClientFactory and the shared AWS client builder only apply proxyProtocol. The field, accessors, both JSON copies, both DSL factories and the upgrade-guide entry are all in place, and fixing the borrowed proxyProtocol description is a nice catch.
The one thing left: applying the review suggestions left two Javadoc blocks on each accessor. In Transcribe2Configuration.java, the old /** The protocol to use when instantiating the Transcribe client */ blocks right before the new ones on getProtocol() (line 267) and setProtocol() (line 280) are now orphaned. Could you delete those two and run mvn formatter:format impsort:sort on the module once, since GitHub suggestion commits are not formatted? Heads-up: #29709 appends an upgrade-guide entry at the same spot, so one of the two will need a small rebase.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 of 723 tested, 23 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (5 direct + 8 downstream), skip tests for 23 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (23)
All tested modules (36 modules, 5m 45s total)Total reactor time: 5m 45s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Clean, complete deprecation, with one small thing to tidy.
The call is right: protocol is dead. The Transcribe client comes from the shared AwsClientBuilderUtil, which reads region, endpoint override and the proxy settings off AwsCommonConfiguration, and that interface exposes getProxyProtocol(), not getProtocol(). So the option was never applied, and its description was the proxyProtocol one copied over, which made it read as if it configured the proxy. Deprecating it and correcting the description to stop claiming a proxy role is the right pair of changes.
The regeneration is complete: deprecated: true plus the deprecationNote land in both the component META-INF JSON and the camel-catalog copy, and the component and endpoint DSL factories carry the note too. The upgrade-guide entry points users at proxyProtocol with proxyHost and proxyPort, which is the option that actually reaches the client builder. Good.
One cleanup: getProtocol() and setProtocol(...) each end up with two /** ... */ blocks back to back, the first without the @deprecated tag and the second with it. Javadoc keeps only the block immediately above the member, so the first is dropped and left as a dangling comment. That is the same orphaned-Javadoc pattern your own #29641 swept out of the tree, so it is worth collapsing each pair into the single block that carries the @deprecated tag.
Claude Code on behalf of oscerd
…plicated Applying the review suggestions through GitHub left two consecutive /** */ blocks on getProtocol and setProtocol, the first without the @deprecated tag. Javadoc keeps only the block immediately above the member, so the first one is an orphan - the same defect apache#29641 swept out of the tree. Each pair is now the single block that carries the tag. The generated description is unchanged: the generator reads the last block before the member, and its first line is the same.
|
Fixed — and thanks for spotting it; the irony of leaving orphaned Javadoc behind is not lost on me, given #29641. Both pairs on /**
* The protocol to use when instantiating the Transcribe client
*
* @deprecated Not used: nothing reads this option when the Transcribe client is built
*/
@Deprecated
public Protocol getProtocol() {I checked the file for any other adjacent The generated metadata is unchanged by this, which is worth stating explicitly since I cannot run the regen here: the schema generator reads the last Javadoc block before the member, and that block's first line is the same text as the orphan's, so the description in both JSON copies and in the two DSL factories stays byte-identical. I re-read them after the edit to confirm. On On the rebase heads-up — agreed, #29706 and #29709 both append to |
Problem
Transcribe2Configurationdeclares aprotocoloption:Nothing reads it. The Transcribe client is built by the shared
AwsClientBuilderUtil, which reads the region, the endpoint override and the proxy settings off theAwsCommonConfigurationinterface — and that interface has noprotocol, onlygetProxyProtocol(). Across all ofcomponents/camel-aws, the only callers ofTranscribe2Configuration.getProtocol()are the generatedTranscribe2EndpointConfigurerandTranscribe2ComponentConfigurer. Settingprotocol=HTTPon anaws2-transcribeendpoint is accepted and does nothing.It is also the only
Protocol protocolfield in the whole aws2 family.aws2-sqshas aprotocoloption too, but aStringone that it genuinely uses to build the queue URL scheme.The description made it worse.
setProtocolcarriedsetProxyProtocol's javadoc:so the option read as if it configured the proxy — which is exactly what
proxyProtocol, declared ten lines below it, really does and which the shared builder really applies. A user following the documentation would setprotocoland get no proxy and no warning.Fix
Deprecate it with a
deprecationNote, followingcamel-docling'sapiKeyHeaderin #29591 — the same situation, deprecated the same way in this release — and give it a description that no longer claims to configure a proxy.Changed:
Transcribe2Configuration—@Deprecated+@Metadata(deprecationNote = ...), and the getter/setter javadoc.aws2-transcribe.jsonand itscamel-catalogcopy —"deprecated": trueplus the note, on both the component property and the endpoint parameter. The two files are byte-identical afterwards.Transcribe2EndpointBuilderFactory(2 methods) andAws2TranscribeComponentBuilderFactory(1 method) —@Deprecatedand the re-wrapped javadoc.camel-4x-upgrade-guide-4_24.adoc— the entry a deprecation takes, pointing users atproxyProtocol/proxyHost/proxyPort.The generated files were updated by hand, since I cannot run maven here; I took the exact shape from the
apiKeyHeadercommit (5de82a9) and re-wrapped the DSL javadoc with the generator's own width (69 characters of text at indent 9), checking first that the wrap reproduces the untouched lines. If the regen job still finds a difference I will take its patch.Tests
No behaviour changes, so there is nothing new to assert: the option did nothing before and does nothing now, it is only labelled honestly. The existing
aws2-transcribetests cover the client building that this does not touch.