You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/04/23 12:36:04 UTC
[PR] Reject invalid entity names in the REST layer [polaris]
adutra opened a new pull request, #4282:
URL: https://github.com/apache/polaris/pull/4282
Disallow slash, empty, and leading/trailing whitespace in names submitted to the create-namespace, create/register/rename-table, create/rename-view, and create-generic-table endpoints.
Policies already enforce a stricter pattern via Open-API generated bean validation, so they are left untouched.
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3138803533
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
No, it won't work anymore, that's the whole point of this PR:
```
spark-sql ()> create namespace ns1;
Time taken: 0.204 seconds
spark-sql ()> create table ns1.`my/table1` (id int);
26/04/24 17:57:00 ERROR SparkSQLDriver: Failed in [create table ns1.`my/table1` (id int)]
java.lang.IllegalArgumentException: Entity name must not contain '/': my/table1
at org.apache.iceberg.rest.ErrorHandlers$DefaultErrorHandler.accept(ErrorHandlers.java:232)
at org.apache.iceberg.rest.ErrorHandlers$TableErrorHandler.accept(ErrorHandlers.java:124)
at org.apache.iceberg.rest.ErrorHandlers$TableErrorHandler.accept(ErrorHandlers.java:108)
at org.apache.iceberg.rest.HTTPClient.throwFailure(HTTPClient.java:240)
at org.apache.iceberg.rest.HTTPClient.execute(HTTPClient.java:336)
at org.apache.iceberg.rest.HTTPClient.execute(HTTPClient.java:297)
at org.apache.iceberg.rest.BaseHTTPClient.post(BaseHTTPClient.java:100)
at org.apache.iceberg.rest.RESTSessionCatalog$Builder.create(RESTSessionCatalog.java:773)
```
> I'd also suggest to add integration or regression tests for this.
This PR has integration tests. How do you see a regression test for this fix looking like?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3138877839
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
I added a note the the changelog, FYI.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3134971804
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
This works in Spark per my test in the main branch. Does it still work after the change?
```
create table `my/table1` (id int);
```
If not, it is a behavior change. We should be roll out carefully.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3138803533
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
No, it won't work anymore, that's the whole point of this PR:
```
spark-sql ()> create table `my/table1` (id int);
26/04/24 17:44:15 ERROR SparkSQLDriver: Failed in [create table `my/table1` (id int)]
org.apache.iceberg.exceptions.NoSuchTableException: Invalid table identifier: my/table1
at org.apache.iceberg.rest.RESTSessionCatalog.checkIdentifierIsValid(RESTSessionCatalog.java:1035)
at org.apache.iceberg.rest.RESTSessionCatalog$Builder.<init>(RESTSessionCatalog.java:687)
at
```
> I'd also suggest to add integration or regression tests for this.
This PR has integration tests. How do you see a regression test for this fix looking like?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3138803533
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
No, it won't work anymore, that's the whole point of this PR:
```
spark-sql ()> create table `my/table1` (id int);
26/04/24 17:44:15 ERROR SparkSQLDriver: Failed in [create table `my/table1` (id int)]
org.apache.iceberg.exceptions.NoSuchTableException: Invalid table identifier: my/table1
at org.apache.iceberg.rest.RESTSessionCatalog.checkIdentifierIsValid(RESTSessionCatalog.java:1035)
at org.apache.iceberg.rest.RESTSessionCatalog$Builder.<init>(RESTSessionCatalog.java:687)
at
```
> 'd also suggest to add integration or regression tests for this.
This PR has integration tests. How do you see a regression test for this fix looking like?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3139281402
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
I see the point of the trailing/leading white space, but why the slash? I mean, I would never name a table with a slash, but also don't want to be accused of murdering children ๐
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3134833832
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
+ throw new IllegalArgumentException("Entity name must not contain '/': " + name);
+ }
+ if (!name.equals(name.strip())) {
+ throw new IllegalArgumentException(
+ "Entity name must not have leading or trailing whitespace: " + name);
+ }
+ }
+
+ /** Validates each level of a namespace. */
+ public static void validateNamespace(Namespace namespace) {
+ for (String level : namespace.levels()) {
+ validateName(level);
+ }
+ }
+
+ /** Validates the namespace and name of an identifier. */
Review Comment:
nit: this comment seems unnecessary, I think the method name itself is good enough here.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3134838925
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
Review Comment:
I believe we should do that for `renameTable` as well.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3134909835
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
Do we need to consider the quoted use case? For example, "my/table" vs my/table, the former seems valid. cc @dennishuo, @collado-mike, @gh-yzou
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3130981801
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -74,13 +75,15 @@ public Response createGenericTable(
String polarisGenericTableAccessDelegation,
RealmContext realmContext,
SecurityContext securityContext) {
+ EntityNameValidator.validateName(createGenericTableRequest.getName());
Review Comment:
No namespace validation here?
Mean, sure, the namespace should already exist.
THinking about defense-in-depth here
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
+ throw new IllegalArgumentException("Entity name must not contain '/': " + name);
+ }
+ if (!name.equals(name.strip())) {
+ throw new IllegalArgumentException(
+ "Entity name must not have leading or trailing whitespace: " + name);
+ }
+ }
+
+ /** Validates each level of a namespace. */
+ public static void validateNamespace(Namespace namespace) {
Review Comment:
Nit: maybe also a function taking a `Namespace` and a `String name`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #4282:
URL: https://github.com/apache/polaris/pull/4282#issuecomment-4342708927
> The PR looks good to me, so +1 - it deserves a rebase/conflict-resolution though.
How about other characters? I [suggested](https://lists.apache.org/thread/g54czqdfw2r7k7cxrtfd84wtd52kxps4) to extend the checks to:
- Control characters
- Backslash `\`
- Path segments equal to `.` or `..`
- Commonly discouraged symbols: `* ? " < > | #`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3134971804
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
This works in Spark per my test in the main branch. Does it still work after the change?
```
create table `my/table1` (id int);
```
If not, it is a behavior change. We should be roll out carefully. I'd also suggest to add integration or regression tests for this. I think unit test itself isn't enough.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3131807668
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogAdapter.java:
##########
@@ -74,13 +75,15 @@ public Response createGenericTable(
String polarisGenericTableAccessDelegation,
RealmContext realmContext,
SecurityContext securityContext) {
+ EntityNameValidator.validateName(createGenericTableRequest.getName());
Review Comment:
Thought about that too, but indeed since the ns must pre-exist, I skipped validation.
Let me add that back, just to err on the safe side.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3138738057
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
Review Comment:
It's done already:
```java
EntityNameValidator.validateIdentifier(renameTableRequest.destination());
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4282:
URL: https://github.com/apache/polaris/pull/4282#discussion_r3148480258
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/EntityNameValidator.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.catalog.validation;
+
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+/**
+ * Validates entity names provided by clients at the REST layer (tables, views, namespaces, generic
+ * tables, ...). A valid name:
+ *
+ * <ul>
+ * <li>is not null or empty;
+ * <li>does not contain a forward slash ({@code /});
+ * <li>does not start or end with whitespace.
+ * </ul>
+ */
+public final class EntityNameValidator {
+
+ private EntityNameValidator() {}
+
+ /** Validates a single entity name (table, view, namespace level, ...). */
+ public static void validateName(String name) {
+ if (name == null || name.isEmpty()) {
+ throw new IllegalArgumentException("Entity name must not be empty");
+ }
+ if (name.indexOf('/') >= 0) {
Review Comment:
The issue is that `/` can have implications on the prefix hierarchy.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #4282:
URL: https://github.com/apache/polaris/pull/4282#issuecomment-4355802016
Merged.
I think we could further discuss forbidding the following characters in a follow-up PR:
- Control characters 0x00โ0x1F and 0x7F. GCS forbids line breaks; S3 says they "require special handling"; Azure rejects them.
- Backslash `\`. Azure forbids it, S3 advises against, and since it's the Windows/POSIX path-separator, it's "risky".
- Path segments equal to `.` or `..` GCS and Azure forbid; S3 seems to have trouble with that.
- `: * ? " < > |` GCS strongly discourages these.
- `#` GCS discourages, and it's the URL fragment delimiter, so "risky" as well.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid entity names in the REST layer [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #4282:
URL: https://github.com/apache/polaris/pull/4282
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org