DanielLeens commented on PR #11814:
URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5812745144
Thanks both — and @goutamadwant, thanks for catching this. I want to correct
something I got wrong in my own comment from earlier today rather than let it
stand.
**F8 correction.** I previously said the emitted `Content-Type` was
byte-identical with no `; charset=utf-8` suffix, reasoning only from
`MediaType.parse("application/json")` carrying no charset parameter. That
reasoning was incomplete — I looked at the `MediaType` alone and didn't trace
what the `RequestBody` factory does with it. Re-checking the actual call at
`JobEventHttpReportHandler.java:196` (`RequestBody.create(events,
httpMediaType)`, the `String` + `MediaType` overload): when the supplied
`MediaType` has no charset, okhttp3's `RequestBody.create(String, MediaType)`
sets one to UTF-8 and rewrites the content type to include `; charset=utf-8`
before it's ever attached to the request. That's exactly what
`JobEventHttpReportHandlerTest.java:190` asserts (`"application/json;
charset=utf-8"`), so the test is correct and my prior claim was not.
I also checked the pre-PR behavior directly rather than taking
@goutamadwant's word for it: at the merge-base commit (before this PR),
`JobEventHttpReportHandler` used `com.squareup.okhttp:mockwebserver:2.7.5` (see
the old `pom.xml`) and called `RequestBody.create(httpMediaType, events)` — the
equivalent okhttp 2.x overload, which has the identical well-known behavior of
appending `; charset=utf-8` when the passed `MediaType` doesn't specify one. So
this is preserved legacy behavior, not a new incompatibility introduced by the
migration — @goutamadwant's answer is correct on both counts (current behavior
and why it matches the legacy client).
**F5 and F7, independently re-verified against `ec6a7f9affc` (no new commit,
so this is the same source I reviewed before):**
- F5: `testRetryAfterHttpFailure` (`JobEventHttpReportHandlerTest.java`)
calls `handler.report()` twice explicitly rather than waiting on the
`Duration.ofDays(1)` interval to fire, bounds both `takeRequest` calls at `10,
TimeUnit.SECONDS`, and closes both request `Buffer`s via try-with-resources.
Matches the description.
- F7: `createHttpClient()` still builds with
`.followRedirects(false).followSslRedirects(false)`, and
`testDoesNotFollowRedirects` exercises it with a real 307 response. Unchanged
from what I confirmed in earlier rounds.
With F8 now correctly resolved (preserved, not a new incompatibility) and
F5/F7 re-confirmed directly against source, I don't have anything further open
from my side. No new commit has landed since `ec6a7f9affc`, which is the head
my `APPROVED` review already covers, so that approval stands — this comment is
a factual correction plus re-verification, not a new review.
--
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]
