DanielLeens commented on PR #11940:
URL: https://github.com/apache/seatunnel/pull/11940#issuecomment-5477050801
Thanks for the deep pass, @SEZ9 — this is a thorough review and I
re-verified every one of your eight findings against the current head
(`e5465cd9b3`, unchanged since your review) before replying, rather than taking
the list at face value. Two of them are real and worth picking up; the other
six I don't think should block merge, for concrete reasons below. Sorry for the
length, but given this touches my own approval I want to show the actual
evidence rather than just assert a conclusion.
**Confirmed, worth picking up (non-blocking, good follow-ups):**
- **Issue 5** (redact `client_secret` by default): agreed, this is a good
hardening suggestion. The connector doesn't currently strip it, and the docs
already flag the risk without acting on it. Worth an `omit_client_secret`
option as you suggest, though I'd treat it as a follow-up rather than a merge
blocker since it's a data-hygiene improvement on top of already-documented
behavior, not a defect that breaks the stated V1 scope.
- **Issue 8** (`${STRIPE_SECRET_KEY}` doc note): agreed, a one-line note on
`-i` variable substitution would save users a confusing HOCON parse error.
Cheap, worth doing.
**Rechecked and I don't think these hold up as stated:**
- **Issue 4 (Serializable)**: this one's already fixed by the code as
written — `StripeSourceParameter extends HttpParameter`
(`StripeSourceParameter.java:31`), and `HttpParameter implements Serializable`
(`connector-http-base/.../HttpParameter.java:29`). The suggested fix ("make
`StripeSourceParameter` extend `HttpParameter`, already Serializable")
describes something that's already true in the current source, so there's no
`NotSerializableException` risk here.
- **Issues 6 & 7 (`columnLength=0`, `nullable=false` on `content`)**: I
checked this against the shared `HttpSource` base class rather than in
isolation. `HttpSource.java:236-246` (the default schema every generic
`connector-http-*` source falls back to) builds the *exact same*
`PhysicalColumn.of("content", BasicType.STRING_TYPE, 0, false, null, ...)` —
same length-0, same non-null. `StripeSource` isn't deviating from convention
here, it's copying the established shared default byte-for-byte. If
`0`/non-null is genuinely risky for auto-create sinks, that's a
`connector-http-base`-wide question affecting every HTTP-family source using
this fallback schema, not something specific to introduce or fix in this PR
alone.
- **Issue 3 (`getBoundedness()` NPE)**: same shape of finding — I checked
`AirtableSource.getBoundedness()` (`AirtableSource.java:66-72`) and it has the
identical no-null-check `jobContext.getJobMode()` idiom with the identical raw
`UnsupportedOperationException`. More importantly, I traced the real call path:
`MultipleTableJobConfigParser.java:428-429` calls `source.setJobContext(...)`
immediately before `FactoryUtil.ensureJobModeMatch(...)` (which is what
actually invokes `getBoundedness()` during job parsing) — so on the real Zeta
submission path, `jobContext` is always set before `getBoundedness()` runs, and
the NPE isn't reachable through normal use. It's a pre-existing, codebase-wide
pattern shared by at least Airtable and Stripe, not a regression this PR
introduces — I'd leave both the null-guard and the message-quality improvement
as a shared-pattern cleanup rather than blocking this specific connector on it.
- **Issue 1 (secret-key leak via logs/exceptions)**: I checked both layers.
`StripeSourceReader.requestFailed()` (`:202-224`) builds its exception from
`response.getContent()` (redacted for the secret key, then truncated to 1024
chars) and `response.getCode()` only — it never touches headers. That redaction
is exactly what commit `25cd9c149` ("Redact Stripe keys from API errors") added
in response to earlier feedback. I also checked the shared
`HttpClientProvider`'s own retry-failure logging
(`connector-http-base/.../HttpClientProvider.java:104-115`) — it logs only the
attempt number and the exception cause (typically an `IOException` like
connection-reset), not headers or request params. I don't see an active leak
path in the current code; masking the option at job-config-print time is a
reasonable extra layer of defense, but the "doesn't include it in logs" claim
in the docs does appear to hold today, not just in one test assertion.
- **Issue 2 (`retry` has no default vs. `rate_limit_max_retries` defaulting
to 3)**: `retry` (`HttpCommonOptions.RETRY`, `connector-http-base`) has no
default for every HTTP-family connector, not just Stripe — it's shared
base-module config. Defaulting it to a non-zero value specifically inside
`StripeSourceOptions` would make Stripe's transport-retry behavior inconsistent
with every sibling `connector-http-*` module rather than fixing a
Stripe-specific gap. If the no-default is genuinely a footgun, that's worth
raising as a `connector-http-base` policy change (affecting all HTTP connectors
uniformly), not a one-off override in this PR.
**Net**: my 2026-08-26 approval stands — the connector logic, tests, and
docs are sound, and none of the re-checked items are defects introduced by this
PR specifically. I'd ask @goutamadwant to pick up Issues 5 and 8 as good, cheap
follow-ups (either in this PR or a fast follow-up), but I don't think
1/2/3/4/6/7 should hold up merge given they're either already mitigated in the
current code, already fixed by inheritance, or pre-existing shared-base-class
conventions used by multiple already-merged sibling connectors.
@SEZ9 — genuinely appreciate the rigor here, and Issues 5/8 are real
improvements I'd missed. Let me know if you have counter-evidence on any of the
other six that I've missed in this pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
