adutra commented on code in PR #4629:
URL: https://github.com/apache/polaris/pull/4629#discussion_r3372115735
##########
gradle/libs.versions.toml:
##########
@@ -78,6 +78,8 @@ jakarta-inject-api = { module =
"jakarta.inject:jakarta.inject-api", version = "
jakarta-servlet-api = { module = "jakarta.servlet:jakarta.servlet-api",
version = "6.1.0" }
jakarta-validation-api = { module =
"jakarta.validation:jakarta.validation-api", version = "3.1.1" }
jakarta-ws-rs-api = { module = "jakarta.ws.rs:jakarta.ws.rs-api", version =
"4.0.0" }
+hibernate-validator = { module =
"org.hibernate.validator:hibernate-validator", version = "8.0.1.Final" }
Review Comment:
Hibernate Validator is referenced in the Quarkus BOM
`io.quarkus.platform:quarkus-bom:3.36.0`:
```
org.hibernate.validator:hibernate-validator:9.1.0.Final (c)
```
It is also brought transitively by `io.quarkus:quarkus-rest-jackson`:
```
+--- io.quarkus:quarkus-rest-jackson -> 3.36.0
| +--- org.hibernate.validator:hibernate-validator:9.1.0.Final
| | +--- jakarta.validation:jakarta.validation-api:3.1.1
```
I am a little worried that by declaring this dependency as a top-level one,
versions may drift. Let's at least try to use the same one from Quarkus (9.1.0)
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -235,13 +235,7 @@ public void testCreateCatalogWithInvalidName() {
String longInvalidName = newRandomString(MAX_IDENTIFIER_LENGTH + 1);
List<String> invalidCatalogNames =
- Arrays.asList(
- longInvalidName,
- "",
- "system$catalog1",
- "SYSTEM$TestCatalog",
- "System$test_catalog",
- " SysTeM$ test catalog");
+ Arrays.asList(longInvalidName, "", "system", "SYSTEM", "System",
"SysTeM");
Review Comment:
The existing test cases seem to imply that `\$` was in fact an escaped
dollar sign.
Your change is changing that logic to a logic where only the word `system`
is detected (case-insensitive, and ignoring surrounding whitespace).
Looking at the git blame of these lines, I found this text in the commit
message:
```
commit 4fb3b6c19a8a8a4961b777ad32dbe1b87d5efe94
Author: Evgeny Zubatov <[email protected]>
Date: Thu Jul 25 14:02:30 2024 -0700
Adding annotation and enforcing size limits for Principal, Role, Catalog
and Catalog Role names.
Also blocking "SYSTEM$" prefix from being used in names.
Adding case-insensitive regex rule to block "SYSTEM$"
```
So it seems that the intent is to block `SYSTEM$`.
@collado-mike do you know more?
##########
api/management-model/build.gradle.kts:
##########
@@ -36,6 +36,10 @@ dependencies {
testImplementation("org.junit.jupiter:junit-jupiter")
testImplementation(platform(libs.jackson.bom))
testImplementation("com.fasterxml.jackson.core:jackson-databind")
+ testImplementation(libs.jakarta.validation.api)
+ testImplementation(libs.hibernate.validator)
+ testImplementation(libs.expressly)
Review Comment:
Where is expressly being used?
--
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]
