SEPURI-SAI-KRISHNA opened a new pull request, #13014: URL: https://github.com/apache/seatunnel/pull/13014
Yes, contributing to Apache SeaTunnel. Closes #12620. This is the focused test @SEZ9 and @DanielLeens asked for on that issue, pinning the case-conversion locale contract for `seatunnel-transforms-v2` now that the zero baseline actually exists on `dev`. ## Why the contract needs a guard `"id".toUpperCase()` returns `"İD"` under a Turkish or Azerbaijani default locale, because those locales map `i` to `U+0130`. A keyword comparison that works on one worker therefore stops matching on another, with no error. #12618 removed 11 such sites and #12495 removed the remaining 13, so the module is at zero today. Nothing stops the next one being added. ## The four asks, and where each is satisfied | ask | where | | --- | --- | | Pin the zero baseline, with matching a newline inside the parentheses cannot defeat | `caseConversionNeverUsesTheDefaultLocale` | | Name the `SeaTunnelDataContext` exception in the test, and comment the call site | `LOCALE_GET_DEFAULT_EXCEPTION` plus `theOnlyDefaultLocaleLookupIsTheDocumentedCalciteException`, and the comment in `SeaTunnelDataContext` | | Record the `StringFunction` LOWER/UPPER decision as rationale | class javadoc | | Use the existing test setup, no new style-check dependency, no broad CI regex rule | `seatunnel-ci-tools`, where `javaparser-core` is already a dependency; the scan is limited to one module | ## Two deliberate deviations, both of which I think are improvements **It parses instead of pattern matching.** The ask was for multiline-aware matching. This uses the JavaParser AST that `ImportClassCheckTest` and `ChineseCharacterCheckTest` in this module already use, because a bare call is exactly a `MethodCallExpr` with an empty argument list. That is not a cosmetic difference. A text scan also matches a comment that merely mentions the method, and this module already contains one: `NumericFunction.java:282` explains the Turkish dotless i in prose. The AST does not see it. Both behaviours are covered by the mutation runs below. **It lives in `seatunnel-ci-tools`, not in `seatunnel-transforms-v2/src/test`.** This one matters more than it looks. The `code-style` job that runs `seatunnel-ci-tools` carries no `if:` condition, unlike the connector and engine lanes which are gated on `needs.changes.outputs.*`. So the guard runs on every pull request. Placed inside `transforms-v2`, it would only run when that module's own lane was triggered, and a regression arriving by another route would not be seen. Happy to move it if you would rather it sat with the module, but the guard is weaker there. ## Mutation evidence A guard that passes proves nothing on its own, so each check was shown to fail for the right reason, and the pair that must not fire was shown not to. | mutation | expected | observed | | --- | --- | --- | | bare `toUpperCase()` with a newline between the parentheses | fail | failed, reporting `StringFunction.java:875 in.toUpperCase()` | | a comment mentioning `.toUpperCase()` and `.toLowerCase()` | no failure | stayed green | | `java.util.Locale.getDefault()` outside the allowed file | fail | see below | The third one is worth reporting rather than quietly fixing. It did **not** fail at first. The check keyed on the literal scope `"Locale"`, so the fully qualified spelling of the same lookup walked straight through. That is now handled by `isLocaleType`, the mutation reruns red, and the helper carries a comment saying why it exists so the narrowing is not reintroduced. ## Scope, stated so it is not mistaken for more - Case conversion in `src/main` only. A bare call in that module's test sources is not covered, since the contract is about production code. - Date and time formatting stays out, as agreed: `DateTimeFormatter.ofPattern` is behavioural work on #12975 and #12976, and `ZetaDateTimeFormat`'s patterns are numeric only. - Other locale-sensitive APIs such as `String.format` and `Collator` are outside the contract as scoped, and the class javadoc says so rather than leaving it implied. - The control asserts a floor of 20 qualified calls against 33 present today. A floor rather than an exact count, because pinning the count would fail on every legitimate addition. Its only job is to prove the scan reaches real call sites, so that a zero means "none present" and not "nothing was read". ## On the carve-out The "preserve the casing the user asked for" exception is empty by decision, not by absence. SQL `LOWER` and `UPPER` at `StringFunction:264` and `:272` are the one place that could have claimed it, because they return row values rather than message text, and #12495 settled it by pinning both to `Locale.ROOT`. The javadoc states it that way because it is the point a future reader is most likely to challenge. ## Tests - `./mvnw spotless:apply` clean on both touched modules. - The exact CI command, `./mvnw -B -T 1 clean test -D"license.skipAddThirdParty"=true -pl seatunnel-ci-tools -am`, run with `MAVEN_OPTS=-Xmx512m` to match the workflow: **BUILD SUCCESS, 37 tests, 0 failures, 0 errors**. The new class contributes 3 and completes in 0.33s. - The heap was checked deliberately, since #11623 previously had to fix an OOM in this module. The scan holds 233 parsed sources and runs in the same JVM as `ImportClassCheckTest`, which parses the whole repository, with no OOM at 512m. - The production change is a comment only, so no behaviour changes and no documentation is affected. -- 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]
