gh-yzou commented on code in PR #955: URL: https://github.com/apache/polaris/pull/955#discussion_r1945482480
########## api/management-model/src/test/java/org/apache/polaris/core/admin/model/README.md: ########## @@ -0,0 +1,37 @@ +# Catalog Serialization Tests + +This directory contains test cases for validating the serialization and deserialization of Catalog objects in the Polaris management model. + +## Test Coverage + Review Comment: This readme seems very specific to the catalogSerialization and deserialization test, not a general readme for all admin/model tests. I think the test suite name its-self is pretty self-explaining about the coverage, and you can probably move the details about the test responsibility, coverage, edge case, error handling to the particular test suite as java doc ########## server-templates/typeInfoAnnotation.mustache: ########## @@ -0,0 +1,29 @@ +/* + * 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. + */ +@JsonTypeInfo( + use = JsonTypeInfo.Id.NAME, + include = JsonTypeInfo.As.EXISTING_PROPERTY, + property = "{{propertyName}}", + visible = true +) +@JsonSubTypes({ +{{#mappedModels}} + @JsonSubTypes.Type(value = {{modelName}}.class, name = "{{mappingName}}"){{^-last}},{{/-last}} +{{/mappedModels}} +}) Review Comment: new line ########## .vscode/settings.json: ########## @@ -0,0 +1,13 @@ +{ + "java.configuration.updateBuildConfiguration": "automatic", + "java.compile.nullAnalysis.mode": "automatic", + "java.project.sourcePaths": [ Review Comment: are we suppose to merge this setting for vscode? if not, we probably should remove this from the pr. ########## api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java: ########## @@ -0,0 +1,259 @@ +package org.apache.polaris.core.admin.model; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.DeserializationFeature; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.*; +import java.util.Collections; +import com.fasterxml.jackson.databind.JsonMappingException; +import java.util.Arrays; + +public class CatalogSerializationTest { + + private ObjectMapper mapper; + private static final String TEST_LOCATION = "s3://test/"; + private static final String TEST_CATALOG_NAME = "test-catalog"; + private static final String TEST_ROLE_ARN = "arn:aws:iam::123456789012:role/test-role"; + + @BeforeEach + public void setUp() { + mapper = new ObjectMapper(); + mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + } + + @Test + public void testCatalogSerialization() throws JsonProcessingException { + CatalogProperties properties = new CatalogProperties(TEST_LOCATION); + + StorageConfigInfo storageConfig = new StorageConfigInfo( + StorageConfigInfo.StorageTypeEnum.S3, + Collections.emptyList()); + + Catalog catalog = new Catalog( + Catalog.TypeEnum.INTERNAL, + TEST_CATALOG_NAME, + properties, + storageConfig); + + String json = mapper.writeValueAsString(catalog); + JsonNode jsonNode = mapper.readTree(json); + + assertEquals(1, countTypeFields(jsonNode)); + assertEquals("INTERNAL", jsonNode.get("type").asText()); + } + + @Test + public void testCatalogDeserialization() throws JsonProcessingException { + String json = "{" + + "\"type\": \"INTERNAL\"," + + "\"name\": \"test-catalog\"," + + "\"properties\": {" + + " \"default-base-location\": \"s3://test/\"" + + "}," + + "\"storageConfigInfo\": {" + + " \"storageType\": \"S3\"," + + " \"roleArn\": \"arn:aws:iam::123456789012:role/test-role\"," + + " \"allowedLocations\": []" + + "}" + + "}"; + + Catalog catalog = mapper.readValue(json, Catalog.class); + + assertEquals(Catalog.TypeEnum.INTERNAL, catalog.getType()); + assertEquals(TEST_CATALOG_NAME, catalog.getName()); + assertEquals(TEST_LOCATION, catalog.getProperties().getDefaultBaseLocation()); + } + + @Test + public void testCatalogWithNullFields() throws JsonProcessingException { + // Create catalog with null fields except type (required) + Catalog catalog = new Catalog( + Catalog.TypeEnum.INTERNAL, // type cannot be null for polymorphic deserialization + null, // null name + null, // null properties + null // null storage config + ); + + // Test serialization + String json = mapper.writeValueAsString(catalog); + + // Test deserialization + Catalog deserializedCatalog = mapper.readValue(json, Catalog.class); + + // Verify fields + assertEquals(Catalog.TypeEnum.INTERNAL, deserializedCatalog.getType()); + assertNull(deserializedCatalog.getName()); + assertNull(deserializedCatalog.getProperties()); + assertNull(deserializedCatalog.getStorageConfigInfo()); + } + + @Test + public void testInvalidJsonDeserialization() { + String invalidJson = "{ invalid json }"; + assertThrows(JsonProcessingException.class, () -> mapper.readValue(invalidJson, Catalog.class)); + } + + @Test + public void testCatalogWithEmptyFields() throws JsonProcessingException { + StringBuilder json = new StringBuilder(); + json.append("{") + .append("\"type\": \"INTERNAL\",") + .append("\"name\": \"\",") + .append("\"properties\": {") + .append(" \"default-base-location\": \"\"") + .append("},") + .append("\"storageConfigInfo\": {") + .append(" \"storageType\": \"S3\",") + .append(" \"roleArn\": \"arn:aws:iam::123456789012:role/empty\",") + .append(" \"allowedLocations\": []") + .append("}") + .append("}"); + + Catalog catalog = mapper.readValue(json.toString(), Catalog.class); + assertEquals("", catalog.getName()); + assertEquals("", catalog.getProperties().getDefaultBaseLocation()); + } + + @Test + public void testSpecialCharacters() throws JsonProcessingException { + StorageConfigInfo storageConfig = new AwsStorageConfigInfo( + TEST_ROLE_ARN, + StorageConfigInfo.StorageTypeEnum.S3); + + String specialName = "test\"catalog"; + Catalog catalog = new Catalog( + Catalog.TypeEnum.INTERNAL, + specialName, + new CatalogProperties(TEST_LOCATION), + storageConfig); + + String json = mapper.writeValueAsString(catalog); + JsonNode node = mapper.readTree(json); + assertEquals(specialName, node.get("name").asText()); + } + + @Test + public void testCatalogWithEmptyStrings() throws JsonProcessingException { + String json = "{" + + "\"type\": \"INTERNAL\"," + + "\"name\": \"\"," + + "\"properties\": {" + + " \"default-base-location\": \"\"" + + "}," + + "\"storageConfigInfo\": {" + + " \"storageType\": \"S3\"," + + " \"roleArn\": \"\"," + + " \"allowedLocations\": []" + + "}" + + "}"; + + Catalog catalog = mapper.readValue(json, Catalog.class); + String serialized = mapper.writeValueAsString(catalog); + JsonNode node = mapper.readTree(serialized); + + assertEquals("", node.get("name").asText()); + assertEquals("", node.at("/properties/default-base-location").asText()); + assertEquals("", node.at("/storageConfigInfo/roleArn").asText()); + } + + @Test + public void testInvalidEnumValue() { + String json = "{" + + "\"type\": \"INVALID_TYPE\"," + + "\"name\": \"test-catalog\"" + + "}"; + + assertThrows(JsonMappingException.class, () -> mapper.readValue(json, Catalog.class)); + } + + @Test + public void testMalformedJson() { + String json = "{" + + "\"type\": \"INTERNAL\"," + + "\"name\": \"test-catalog\"," + + "\"properties\": {" + + "}"; + + assertThrows(JsonProcessingException.class, () -> mapper.readValue(json, Catalog.class)); + } + + @Test + public void testLongCatalogName() throws JsonProcessingException { + String longName = "a".repeat(1000); + Catalog catalog = new Catalog( + Catalog.TypeEnum.INTERNAL, + longName, + new CatalogProperties(TEST_LOCATION), + null); + String json = mapper.writeValueAsString(catalog); Review Comment: it seems all tests below are doing round trip testing, i think we can probably extract a testRoundTrip method that takes a Catalog the ensures all necessary fields for the catalog is serialized and deserialized properly, includes name, type, properties etc. -- 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]
