You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "dimas-b (via GitHub)" <gi...@apache.org> on 2024/12/31 23:22:04 UTC
[PR] Convert FileIOIntegrationTest to a unit test [polaris]
dimas-b opened a new pull request, #598:
URL: https://github.com/apache/polaris/pull/598
This makes exception handling tests for direct and more light-weight. Previously `FileIOIntegrationTest` used the full DW application stack just to exercise some custom test code in runtime.
* Validate the basic fact of storage-level exception propagation to the REST API layer in `FileIOExceptionsTest`.
* Validate relevant I/O exception message handling and status codes in `IcebergExceptionMapperTest`.
* `IcebergExceptionMapper` is implicitly engages in runtime by the Web App framework based on its annotations.
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
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] Convert FileIOIntegrationTest to a unit test [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #598:
URL: https://github.com/apache/polaris/pull/598#discussion_r1900736168
##########
dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/IcebergExceptionMapperTest.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.dropwizard;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.azure.core.exception.AzureException;
+import com.google.cloud.storage.StorageException;
+import jakarta.ws.rs.core.Response;
+import java.util.stream.Stream;
+import org.apache.polaris.service.exception.IcebergExceptionMapper;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import software.amazon.awssdk.services.s3.model.S3Exception;
+
+class IcebergExceptionMapperTest {
+
+ static Stream<Arguments> fileIOExceptionMapping() {
+ return Stream.of(
+ Arguments.of(new AzureException("Unknown"), 500),
+ Arguments.of(new AzureException("Forbidden"), 403),
+ Arguments.of(new AzureException("FORBIDDEN"), 403),
+ Arguments.of(new AzureException("Not Authorized"), 403),
+ Arguments.of(new AzureException("Access Denied"), 403),
+ Arguments.of(S3Exception.builder().message("Access denied").build(), 403),
+ Arguments.of(new StorageException(1, "access denied"), 403));
Review Comment:
While at it, wdyt about adding the missing test cases?
```suggestion
Arguments.of(new NoSuchNamespaceException("Unknown"), 404),
Arguments.of(new NoSuchIcebergTableException("Unknown"), 404),
Arguments.of(new NoSuchTableException("Unknown"), 404),
Arguments.of(new NoSuchViewException("Unknown"), 404),
Arguments.of(new NotFoundException("Unknown"), 404),
Arguments.of(new AlreadyExistsException("Unknown"), 409),
Arguments.of(new CommitFailedException("Unknown"), 409),
Arguments.of(new UnprocessableEntityException("Unknown"), 422),
Arguments.of(new CherrypickAncestorCommitException(123L), 400),
Arguments.of(new CommitStateUnknownException("Unknown", new RuntimeException()), 400),
Arguments.of(new DuplicateWAPCommitException("Unknown"), 400),
Arguments.of(new ForbiddenException("Unknown"), 403),
Arguments.of(new jakarta.ws.rs.ForbiddenException("Unknown"), 403),
Arguments.of(new NotAuthorizedException("Unknown"), 401),
Arguments.of(new NamespaceNotEmptyException("Unknown"), 400),
Arguments.of(new ValidationException("Unknown"), 400),
Arguments.of(new ServiceUnavailableException("Unknown"), 503),
Arguments.of(new RuntimeIOException("Unknown"), 503),
Arguments.of(new ServiceUnavailableException("Unknown"), 503),
Arguments.of(new RESTException("Unknown"), 503),
Arguments.of(new IllegalArgumentException("Unknown"), 400),
Arguments.of(new UnsupportedOperationException("Unknown"), 406),
Arguments.of(S3Exception.builder().message("Unknown").build(), 500),
Arguments.of(S3Exception.builder().message("Forbidden").build(), 403),
Arguments.of(S3Exception.builder().message("FORBIDDEN").build(), 403),
Arguments.of(S3Exception.builder().message("Not Authorized").build(), 403),
Arguments.of(S3Exception.builder().message("Access Denied").build(), 403),
Arguments.of(
S3Exception.builder()
.message("Unknown")
.cause(new RuntimeException("Forbidden"))
.build(),
403),
Arguments.of(new AzureException("Unknown"), 500),
Arguments.of(new AzureException("Forbidden"), 403),
Arguments.of(new AzureException("FORBIDDEN"), 403),
Arguments.of(new AzureException("Not Authorized"), 403),
Arguments.of(new AzureException("Access Denied"), 403),
Arguments.of(new AzureException("Unknown", new RuntimeException("Forbidden")), 403),
Arguments.of(new StorageException(1, "Unknown"), 500),
Arguments.of(new StorageException(1, "Forbidden"), 403),
Arguments.of(new StorageException(1, "FORBIDDEN"), 403),
Arguments.of(new StorageException(1, "Not Authorized"), 403),
Arguments.of(new StorageException(1, "Access Denied"), 403),
Arguments.of(new StorageException(1, "Unknown", new RuntimeException("Forbidden")), 403));
```
--
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] Convert FileIOIntegrationTest to a unit test [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #598:
URL: https://github.com/apache/polaris/pull/598#discussion_r1900291531
##########
dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/catalog/io/FileIOIntegrationTest.java:
##########
@@ -1,263 +0,0 @@
-/*
- * 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.dropwizard.catalog.io;
-
-import static org.apache.iceberg.types.Types.NestedField.required;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
-import com.azure.core.exception.AzureException;
-import com.google.cloud.storage.StorageException;
-import com.google.common.collect.Iterators;
-import io.dropwizard.testing.ConfigOverride;
-import io.dropwizard.testing.ResourceHelpers;
-import io.dropwizard.testing.junit5.DropwizardAppExtension;
-import io.dropwizard.testing.junit5.DropwizardExtensionsSupport;
-import java.util.Collection;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.function.Supplier;
-import java.util.stream.Collectors;
-import java.util.stream.IntStream;
-import java.util.stream.Stream;
-import org.apache.iceberg.Schema;
-import org.apache.iceberg.Table;
-import org.apache.iceberg.catalog.Namespace;
-import org.apache.iceberg.catalog.TableIdentifier;
-import org.apache.iceberg.exceptions.ForbiddenException;
-import org.apache.iceberg.inmemory.InMemoryFileIO;
-import org.apache.iceberg.io.FileIO;
-import org.apache.iceberg.rest.RESTCatalog;
-import org.apache.iceberg.types.Types;
-import org.apache.polaris.core.admin.model.Catalog;
-import org.apache.polaris.core.admin.model.FileStorageConfigInfo;
-import org.apache.polaris.core.admin.model.PolarisCatalog;
-import org.apache.polaris.core.admin.model.StorageConfigInfo;
-import org.apache.polaris.service.catalog.io.FileIOFactory;
-import org.apache.polaris.service.dropwizard.PolarisApplication;
-import org.apache.polaris.service.dropwizard.catalog.TestUtil;
-import org.apache.polaris.service.dropwizard.config.PolarisApplicationConfig;
-import org.apache.polaris.service.dropwizard.test.PolarisConnectionExtension;
-import org.apache.polaris.service.dropwizard.test.PolarisRealm;
-import org.apache.polaris.service.dropwizard.test.SnowmanCredentialsExtension;
-import org.apache.polaris.service.dropwizard.test.TestEnvironmentExtension;
-import org.apache.polaris.service.exception.IcebergExceptionMapper;
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
-import org.junit.jupiter.params.provider.MethodSource;
-import software.amazon.awssdk.services.s3.model.S3Exception;
-
-/** Collection of File IO integration tests */
-@ExtendWith({
- DropwizardExtensionsSupport.class,
- TestEnvironmentExtension.class,
- PolarisConnectionExtension.class,
- SnowmanCredentialsExtension.class
-})
-public class FileIOIntegrationTest {
- private static final DropwizardAppExtension<PolarisApplicationConfig> EXT =
- new DropwizardAppExtension<>(
- PolarisApplication.class,
- ResourceHelpers.resourceFilePath("polaris-server-integrationtest.yml"),
- ConfigOverride.config(
- "server.applicationConnectors[0].port",
- "0"), // Bind to random port to support parallelism
- ConfigOverride.config("server.adminConnectors[0].port", "0"),
- ConfigOverride.config("io.factoryType", "test"));
-
- private static final String catalogBaseLocation = "file:/tmp/buckets/my-bucket/path/to/data";
- private static TestFileIOFactory ioFactory;
- private static RESTCatalog restCatalog;
- private static Table table;
-
- @BeforeAll
- public static void beforeAll(
- PolarisConnectionExtension.PolarisToken adminToken,
- SnowmanCredentialsExtension.SnowmanCredentials snowmanCredentials,
- @PolarisRealm String realm) {
- ioFactory = (TestFileIOFactory) EXT.getConfiguration().findService(FileIOFactory.class);
-
- FileStorageConfigInfo storageConfigInfo =
- FileStorageConfigInfo.builder()
- .setStorageType(StorageConfigInfo.StorageTypeEnum.FILE)
- .setAllowedLocations(List.of(catalogBaseLocation))
- .build();
- Catalog catalog =
- PolarisCatalog.builder()
- .setType(Catalog.TypeEnum.INTERNAL)
- .setName("mycatalog")
- .setProperties(
- org.apache.polaris.core.admin.model.CatalogProperties.builder(catalogBaseLocation)
- .build())
- .setStorageConfigInfo(storageConfigInfo)
- .build();
-
- restCatalog =
- TestUtil.createSnowmanManagedCatalog(
- EXT, adminToken, snowmanCredentials, realm, catalog, Map.of());
-
- Namespace namespace = Namespace.of("myns");
- restCatalog.createNamespace(namespace);
-
- String tableName = "mytable";
- table =
- restCatalog
- .buildTable(
- TableIdentifier.of(namespace, tableName),
- new Schema(required(3, "id", Types.IntegerType.get(), "mydoc")))
- .create();
- }
-
- @Test
- void testGetLengthExceptionSupplier() {
Review Comment:
This test case has been dropped completely, but in reality it used to test just other test code :shrug:
--
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] Convert FileIOIntegrationTest to a unit test [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #598:
URL: https://github.com/apache/polaris/pull/598#discussion_r1901117624
##########
dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/IcebergExceptionMapperTest.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.dropwizard;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.azure.core.exception.AzureException;
+import com.google.cloud.storage.StorageException;
+import jakarta.ws.rs.core.Response;
+import java.util.stream.Stream;
+import org.apache.polaris.service.exception.IcebergExceptionMapper;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import software.amazon.awssdk.services.s3.model.S3Exception;
+
+class IcebergExceptionMapperTest {
+
+ static Stream<Arguments> fileIOExceptionMapping() {
+ return Stream.of(
+ Arguments.of(new AzureException("Unknown"), 500),
+ Arguments.of(new AzureException("Forbidden"), 403),
+ Arguments.of(new AzureException("FORBIDDEN"), 403),
+ Arguments.of(new AzureException("Not Authorized"), 403),
+ Arguments.of(new AzureException("Access Denied"), 403),
+ Arguments.of(S3Exception.builder().message("Access denied").build(), 403),
+ Arguments.of(new StorageException(1, "access denied"), 403));
Review Comment:
fair 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] Convert FileIOIntegrationTest to a unit test [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #598:
URL: https://github.com/apache/polaris/pull/598#discussion_r1901112980
##########
dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/IcebergExceptionMapperTest.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.dropwizard;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.azure.core.exception.AzureException;
+import com.google.cloud.storage.StorageException;
+import jakarta.ws.rs.core.Response;
+import java.util.stream.Stream;
+import org.apache.polaris.service.exception.IcebergExceptionMapper;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import software.amazon.awssdk.services.s3.model.S3Exception;
+
+class IcebergExceptionMapperTest {
+
+ static Stream<Arguments> fileIOExceptionMapping() {
+ return Stream.of(
+ Arguments.of(new AzureException("Unknown"), 500),
+ Arguments.of(new AzureException("Forbidden"), 403),
+ Arguments.of(new AzureException("FORBIDDEN"), 403),
+ Arguments.of(new AzureException("Not Authorized"), 403),
+ Arguments.of(new AzureException("Access Denied"), 403),
+ Arguments.of(S3Exception.builder().message("Access denied").build(), 403),
+ Arguments.of(new StorageException(1, "access denied"), 403));
Review Comment:
I've added a test case for the "unexpected" exception. As for trivial exception type to error code mapping, I'm not sure adding test case is worth the effort. It would essentially duplicate the mapping in the `switch` inside `IcebergExceptionMapper`, but there's no "logic" to be tested there. I tend to think the mapping production code plus code review is sufficient for the error code mapping correctness.
Also, enumerating messages for every storage exception type does not add coverage because the code treats them the same way as far as message interpretation is concerned.
--
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] Convert FileIOIntegrationTest to a unit test [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #598:
URL: https://github.com/apache/polaris/pull/598
--
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