You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@polaris.apache.org by "youngrae81 (via GitHub)" <gi...@apache.org> on 2026/10/29 02:45:07 UTC

[PR] Reject invalid external catalog connection configs with 400 instead of 500 [polaris]

youngrae81 opened a new pull request, #5909:
URL: https://github.com/apache/polaris/pull/5909

   ## Summary
   
   `PolarisServiceImpl` validates the `connectionConfigInfo` of an `EXTERNAL` catalog before it reaches `PolarisAdminService`, but two client input errors escaped as `500 Internal Server Error`:
   
   - a connection or authentication type outside `SUPPORTED_CATALOG_CONNECTION_TYPES` / `SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES` was reported with `IllegalStateException`;
   - a request without `authenticationParameters` (the spec only requires `connectionType`) hit a `NullPointerException`.
   
   The allow-list checks now throw `IllegalArgumentException`, which `IcebergExceptionMapper` already maps to `400`, and a missing `authenticationParameters` is rejected first with `authenticationParameters is required for an external catalog`. No behaviour changes for valid requests; the messages for the allow-list cases are unchanged.
   
   Tests:
   - `PolarisServiceImplTest`: the two existing allow-list tests now expect `IllegalArgumentException`, plus a new test for the missing `authenticationParameters`. All three failed before the change (`IllegalStateException` x2, `NullPointerException`).
   - `PolarisManagementServiceIntegrationTest`: new test sending the two requests from the issue and asserting `400` and the message. `ManagementServiceIT` (in-memory) passes, 53 tests.
   
   The scheme-less `HIVE` `uri` NPE in `ConnectionConfigInfoDpo` is handled separately in #5901, and the `HADOOP` `uri` validation is tracked in #5905.
   
   Fixes #5908
   
   ## Checklist
   
   - [x] 🛡️Don't disclose security issues! (contact security@apache.org)
   - [x] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #5908
   - [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] 💡 Added comments for complex logic (none needed)
   - [x] 🧾 Updated `CHANGELOG.md` (if needed)
   - [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed) — not needed, error handling only
   
   AI assistance was used for drafting; the change and tests were run and verified locally.


-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "flyrain (via GitHub) via issues" <is...@polaris.apache.org>.

flyrain merged PR #5909:
URL: https://github.com/apache/polaris/pull/5909


-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "youngrae81 (via GitHub) via issues" <is...@polaris.apache.org>.

youngrae81 commented on PR #5909:
URL: https://github.com/apache/polaris/pull/5909#issuecomment-6650351517

   Thanks! 
   Started a thread on dev@: https://lists.apache.org/thread/0rvw3g9gf5g43km8r0lknpldjf6x4ff2


-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.

dimas-b commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4429546976


##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -202,12 +202,15 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI
             .map(s -> s.toUpperCase(Locale.ROOT))
             .toList();
     if (!supportedConnectionTypes.contains(connectionType)) {
-      throw new IllegalStateException("Unsupported connection type: " + connectionType);
+      throw new IllegalArgumentException("Unsupported connection type: " + connectionType);
     }
   }
 
   private void validateAuthenticationParameters(AuthenticationParameters authenticationParameters) {
-
+    if (authenticationParameters == null) {
+      throw new IllegalArgumentException(
+          "authenticationParameters is required for an external catalog");

Review Comment:
   I agree that those generated builders could be done better 🤷 
   
   Let's at least document this required in the spec. WDYT?



-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.

nandorKollar commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4417413606


##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -202,12 +202,15 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI
             .map(s -> s.toUpperCase(Locale.ROOT))
             .toList();
     if (!supportedConnectionTypes.contains(connectionType)) {
-      throw new IllegalStateException("Unsupported connection type: " + connectionType);
+      throw new IllegalArgumentException("Unsupported connection type: " + connectionType);
     }
   }
 
   private void validateAuthenticationParameters(AuthenticationParameters authenticationParameters) {
-
+    if (authenticationParameters == null) {
+      throw new IllegalArgumentException(
+          "authenticationParameters is required for an external catalog");

Review Comment:
   This sounds like a valid problem, I think the spec should also make it explicit, that this is a required field.



-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "youngrae81 (via GitHub) via issues" <is...@polaris.apache.org>.

youngrae81 commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4429854025


##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -202,12 +202,15 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI
             .map(s -> s.toUpperCase(Locale.ROOT))
             .toList();
     if (!supportedConnectionTypes.contains(connectionType)) {
-      throw new IllegalStateException("Unsupported connection type: " + connectionType);
+      throw new IllegalArgumentException("Unsupported connection type: " + connectionType);
     }
   }
 
   private void validateAuthenticationParameters(AuthenticationParameters authenticationParameters) {
-
+    if (authenticationParameters == null) {
+      throw new IllegalArgumentException(
+          "authenticationParameters is required for an external catalog");

Review Comment:
   Done: the latest commit adds a sentence to the `ConnectionConfigInfo` description in the spec saying that `authenticationParameters` is required when creating an external catalog and that a request without it is rejected with 400. That keeps the generated builders unchanged; the schema-level `required` entry can follow separately.
   
   On the NPE: this PR already replaces it. Before the change, a missing `authenticationParameters` reached `validateAuthenticationParameters` and dereferenced null, which surfaced as a 500; the null guard at the top of that method now turns it into a 400 with "authenticationParameters is required for an external catalog".
   `PolarisServiceImplTest.testValidateExternalCatalog_MissingAuthenticationParameters` covers the unit path and `testCreateExternalCatalogWithInvalidConnectionConfigReturnsBadRequest` the REST path. If you would rather see the check live deeper, for example in the DPO conversion, I am happy to move it. 



-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.

orca-review-bot commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4416793208


##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -202,12 +202,15 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI
             .map(s -> s.toUpperCase(Locale.ROOT))
             .toList();
     if (!supportedConnectionTypes.contains(connectionType)) {
-      throw new IllegalStateException("Unsupported connection type: " + connectionType);
+      throw new IllegalArgumentException("Unsupported connection type: " + connectionType);
     }
   }
 
   private void validateAuthenticationParameters(AuthenticationParameters authenticationParameters) {
-
+    if (authenticationParameters == null) {
+      throw new IllegalArgumentException(
+          "authenticationParameters is required for an external catalog");

Review Comment:
   The server now requires this field, but the management spec still marks it optional. Clients generated from the spec won't know until they get a 400. Can we add `authenticationParameters` to `required` in polaris-management-service.yml, here or in a follow-up?
   
   <sub>🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.</sub>



-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "youngrae81 (via GitHub) via issues" <is...@polaris.apache.org>.

youngrae81 commented on PR #5909:
URL: https://github.com/apache/polaris/pull/5909#issuecomment-6633668952

   @dimas-b gentle ping: the spec description is added as suggested, and the NPE case is already turned into a 400 by this PR. Anything else you would like changed?


-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "youngrae81 (via GitHub) via issues" <is...@polaris.apache.org>.

youngrae81 commented on PR #5909:
URL: https://github.com/apache/polaris/pull/5909#issuecomment-6654405335

   Following @jbonofre's suggestion on the dev@ thread, the latest commit declares a `400` response on `createCatalog` in the Management API spec. The endpoint already returned 400 for an invalid storage configuration and, with this PR, for an invalid connection configuration, but the spec did not list it.
   No generated model or interface changes.


-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "youngrae81 (via GitHub)" <gi...@apache.org>.

youngrae81 commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4419092263


##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -202,12 +202,15 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI
             .map(s -> s.toUpperCase(Locale.ROOT))
             .toList();
     if (!supportedConnectionTypes.contains(connectionType)) {
-      throw new IllegalStateException("Unsupported connection type: " + connectionType);
+      throw new IllegalArgumentException("Unsupported connection type: " + connectionType);
     }
   }
 
   private void validateAuthenticationParameters(AuthenticationParameters authenticationParameters) {
-
+    if (authenticationParameters == null) {
+      throw new IllegalArgumentException(
+          "authenticationParameters is required for an external catalog");

Review Comment:
   Agreed that the spec should say it. 
   I tried it on this branch: adding `authenticationParameters` to `required` on `ConnectionConfigInfo` changes the generated builders (`builder(connectionType)` becomes `builder(connectionType, authenticationParameters)`), which touches the four DPO `asConnectionConfigInfoModel` implementations and about 15 test call sites across polaris-core, runtime/service, integration-tests and cloudTest. 
   That is mostly mechanical, but it would turn this 4-file fix into a 20-file change, so I would rather keep this PR to the 400 fix and open the spec change as a follow-up PR right after this one (I will link it here).
   The null guard here stays useful either way, since the spec-level `@NotNull` only covers the REST path. 
   Does that work for you?



-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.

dimas-b commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4429554196


##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -202,12 +202,15 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI
             .map(s -> s.toUpperCase(Locale.ROOT))
             .toList();
     if (!supportedConnectionTypes.contains(connectionType)) {
-      throw new IllegalStateException("Unsupported connection type: " + connectionType);
+      throw new IllegalArgumentException("Unsupported connection type: " + connectionType);
     }
   }
 
   private void validateAuthenticationParameters(AuthenticationParameters authenticationParameters) {
-
+    if (authenticationParameters == null) {
+      throw new IllegalArgumentException(
+          "authenticationParameters is required for an external catalog");

Review Comment:
   Alternatively, can we fix the `NullPointerException` caused by missing `authenticationParameters`?



-- 
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 external catalog connection configs with 400 instead of 500 [polaris]

Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.

dimas-b commented on code in PR #5909:
URL: https://github.com/apache/polaris/pull/5909#discussion_r4483097416


##########
spec/polaris-management-service.yml:
##########
@@ -67,6 +67,8 @@ paths:
             application/json:
               schema:
                 $ref: "#/components/schemas/Catalog"
+        400:
+          description: "The request is invalid, for example an unsupported or incomplete storage or connection configuration"

Review Comment:
   Nice catch, @jbonofre @youngrae81 !



-- 
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