sachinnn99 opened a new pull request, #13115:
URL: https://github.com/apache/gravitino/pull/13115
### What changes were proposed in this pull request?
Add a shared `validateExternalTypeString` helper to `JdbcTypeConverter` and
call it from
`fromGravitino` in the MySQL, PostgreSQL and Doris converters.
The helper keeps an external type string inside the single column type slot
it is written into,
rather than trying to prove it names a valid type:
1. Reject null, empty and blank values.
2. Strip single-quoted literals, so quoted content such as the members of
`enum('a','b')` may
contain anything.
3. Restrict the remaining characters to `[\w .,:()<>\[\]=-]`.
4. Reject the `--` line comment sequence.
5. Require `()`, `[]` and `<>` to be balanced and never negative, with every
comma nested inside
a bracket pair.
A rejected value throws `IllegalArgumentException` naming the offending
string.
### Why are the changes needed?
External type catalog strings come from the caller and were interpolated
verbatim into the DDL
that the JDBC catalogs build by string concatenation, with nothing
validating them on the way.
`JsonUtils.readExternalType` only checks that the `catalogString` field is
present, and
`Types.ExternalType.of` stores whatever it is given, including `null` and `"
"`.
The case that matters is a top level comma rather than a semicolon.
`MysqlTableOperations.generateAlterTableSql:410` and
`DorisTableOperations:810` join their
actions with `",\n"` inside a single `ALTER TABLE` statement, and
`PostgreSqlTableOperations:604-614` appends `,\nALTER COLUMN ... SET NOT
NULL` right after the
type, so a value such as `int, DROP COLUMN secret` contributes an action of
its own.
Fix: #11805
### Does this PR introduce _any_ user-facing change?
No API or property changes. An external type that could escape the column
type position is now
rejected with `IllegalArgumentException` instead of reaching the database.
### How was this patch tested?
New `TestJdbcTypeConverterExternalTypeValidation` in `catalog-jdbc-common`
covers the algorithm
with 51 cases. Accepted: the MySQL full declarations (`enum('a','b','c')`,
`set('x','y','z')`,
`bit(8)`, `binary(16)`, `varbinary(100)`), the Doris external types and
malformed parse fallbacks
(`bigint unsigned`, `decimal(a,b)`), PostgreSQL `numeric`/`bit`,
`user-defined`, escaped quotes
(`enum('it''s','ok')`), and the ClickHouse shapes. Rejected: top level
commas, `;`, `--`,
`/* */`, `#`, newlines, unterminated quotes, unbalanced brackets, blank and
null.
Each of the three converter test classes also gets a wiring test that goes
through
`fromGravitino`.
```
./gradlew :catalogs:catalog-jdbc-common:test
:catalogs:catalog-jdbc-mysql:test \
:catalogs:catalog-jdbc-postgresql:test
:catalogs:catalog-jdbc-doris:test -PskipITs
```
catalog-jdbc-common 95 tests, catalog-jdbc-mysql 13, catalog-jdbc-postgresql
47,
catalog-jdbc-doris 36, no failures. `spotlessCheck` is clean.
### Notes for reviewers
@FANNG1 one open question, and two places where I followed your prose over
your example regex.
**Open question: `=`.** You listed `=` among the characters to rule out, but
ClickHouse stores
`Enum8('active'=1,'inactive'=2)` as an external type today
(`TestClickHouseTypeConverter:108`, `CatalogClickHouseIT:2934`). ClickHouse
is not wired up in
this PR, but since the helper lives in the shared base class, excluding `=`
now means widening it
again in the follow-up. My reading is that `=` is safe here: with `;`
outside the set, top level
commas rejected and brackets required balanced, a bare `=` in the column
type position cannot
start a new action or terminate the statement, so at worst it is a syntax
error the database
rejects anyway. Say the word and I will drop it and remove the ClickHouse
rows from the test.
**`-` allowed, `--` rejected as a sequence.** You named `--`, not `-`, so I
read the missing
hyphen in `[\w .,:()<>\[\]]` as a side effect rather than the intent — a
lone hyphen cannot open
a comment. It also matters concretely: `TestMysqlTypeConverter:101` and
`TestPostgreSqlTypeConverter:124` already assert that
`fromGravitino(ExternalType.of("user-defined"))` returns the value
unchanged, so excluding `-`
would fail two currently passing tests. `/* */` and `#` need no special
case, since `/`, `*` and
`#` are outside the character set.
**Commas nest inside `<>` too.** You track `<>` as a bracket pair and say
commas outside brackets
are rejected, so I allowed them inside all three pairs, which keeps generic
types such as
`struct<a:int,b:int>` working.
Scope is the three converters named in the issue. ClickHouse, OceanBase and
Hologres do the same
passthrough and can reuse this helper; I will open a follow-up for them.
Hologres looks more
exposed than the rest: `HologresTableOperations:501` joins with `"\n"` and
each clause carries
its own `;`, so a semicolon there could append an entire statement rather
than an extra action.
Glue builds AWS SDK objects rather than SQL, and StarRocks already throws
for external types, so
neither needs this.
One gap worth flagging: `TestMysqlTableOperations`, which is what produces
the
`enum('a','b','c')` values, is tagged `gravitino-docker-test` and I could
not get it to run
locally, so those literals are covered by unit tests rather than end to end.
--
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]
