DanielLeens commented on PR #12443:
URL: https://github.com/apache/seatunnel/pull/12443#issuecomment-6116675941
Thanks @SEZ9 for the follow-up. I re-checked each point against head
`871c6448a8d1` (my review of 10-10 was posted before your latest comment, so
some of this overlaps with it).
- **F1 / F5 (final shape):** Confirmed.
`ConfigMapPathUtils.quoteInvalidPathKeys` is applied at
`ConfigBuilder.java:110`, `ConfigShadeUtils.java:229` and
`ReadonlyConfig.java:79`. The config-adapter guard was removed in this head:
that site (`ConfigBuilder.java:241`) is not on the REST submit path and behaves
exactly as on `dev`, so I read it as an intentional scope reduction. I asked
the author for one sentence in the description listing it (and
`MetadataProviderManager` / `AssertSink`) as out of scope.
- **F2 (docs):** EN/ZH `rest-api-v2.md` now separate JSON path expressions
from quoted HOCON literal keys. I suggested one small precision tweak (the
"literal" wording is exact only for keys the path parser rejects); it is
non-blocking.
- **F3 / F4 / F6 (tests):** All three are covered in `RestUtilTest` and
exercise the quoting branch, not just keys the parser already accepts:
- (a) escape round-trip:
`buildConfigShouldPreserveEscapedAndReservedKeysThroughConfigShade` asserts
`a"b\c` survives encrypt then decrypt.
- (b) top-level quoted key:
`buildConfigShouldPreserveTopLevelInvalidPathKey` (`^t_nova_.*$` at the root,
through `decryptConfig`).
- (c) `->` and reserved/padded keys:
`buildConfigShouldKeepValidPathExpressions` (`a->b` stays a nested path), the
same escape test (`$`, `:`, `${FOO}`), and
`ConfigBuilderTest.testWhitespacePaddedKeysTrimmedUnlessGuardQuotesWholeKey`.
- Real HTTP/HOCON coverage:
`RestApiSubmitJobConfigShadeDecryptTest#testHoconDecryptPreservesQuotedFieldMapperKey`
and `RestApiIT#testSubmitJobWithHoconFormatAndQuotedLiteralKey`.
- **F7 (title):** Thanks for withdrawing it.
**CI:** on the fork run for this head, unit tests and `RestApiIT` (including
the new HOCON case) are green. The remaining red jobs
(`BackpressureSlowSinkIT`, `TestFilterRowKindIT`, and a Maven wrapper download
404 in `jdbc-connectors-it-part-5`) are unrelated to the config-conversion code
here, so a failed-jobs-only rerun on the fork is the next step. I have no other
open source-level concerns on this thread.
--
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]
