GabrielBBaldez commented on PR #11028:
URL: https://github.com/apache/seatunnel/pull/11028#issuecomment-5483185777
Thank you both — and @DanielLeens, thank you for pushing the `dev` merge
yourself this morning. I rebased onto it rather than force-pushing, so
`76a93df9` is intact.
**Issue 1 (blocker) — fixed in code, not just documented.** You were right
that documenting a data-loss path is not shipping a fix. `ShopifySource` now
rejects `pageing` at construction:
```
HTTP-03 The Shopify source does not support the 'pageing' option: it would
be accepted and
then ignored, so only the first page of results would be read while the job
reported success.
Remove 'pageing' from the source configuration.
```
It is covered by a test, and I checked the test actually earns its place —
against the previous code it fails with `Expected HttpConnectorException to be
thrown, but nothing was thrown`, which is precisely the defect. A configuration
without `pageing` still builds, so the normal path is unaffected.
Real `Link`-header cursor support stays a follow-up against
`connector-http-base`, as you suggested.
**Issue 2 — done.** `getSourceClass()` returns `ShopifySource.class`.
**Issue 4 — done, in both locales.** You were right that it was misleading,
and the fix is a little more than a placeholder swap: `${SHOPIFY_ACCESS_TOKEN}`
*is* valid SeaTunnel syntax, just not environment substitution. The docs now
show the `-i SHOPIFY_ACCESS_TOKEN=…` invocation and say what happens without
it, which seemed more useful than removing the variable.
**Issue 7 — done.** Class-level Javadoc on the four classes.
**Issues 3 and 5 — filed as #12025** against `connector-http-base`, with the
scheme check and the `@Data` `toString()` exposure written up together, since
both affect every header-authenticated HTTP connector. I offered to take them
there once you say which direction you prefer for the scheme check — rejecting
outright would break anyone testing against a local mock over plain HTTP, so
that is a maintainer call.
**Issue 6 — left alone deliberately.** Collapsing the two parameter objects
touches how `HttpSource` builds its own `httpParameter`, which is more than
this PR should carry; happy to open it separately if you would like it tracked.
The docs and the `## Pagination` section were updated to match the new
behaviour rather than the old wording — with the option now rejected, "accepted
and ignored" would have been wrong the moment this landed.
The four module tests pass on the rebased head. Thank you for the
thoroughness of both passes; the pagination defect in particular would have
been an unpleasant thing for a user to discover on their own.
--
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]
