From a7b4cb9b465f0cd3ca208a8ce4a3b79b48cfdaf9 Mon Sep 17 00:00:00 2001 From: Nick Watts <1156625+nawatts@users.noreply.github.com> Date: Wed, 31 Jan 2024 11:16:14 -0500 Subject: [PATCH] [AJ-1279] Add tests for SQL expressions for data type conversions (#476) * Add tests for SQL expressions for data type conversions * Attempt to get as many of the args statements onto one line --------- Co-authored-by: Josh Ladieu --- .../workspacedataservice/dao/RecordDao.java | 3 +- .../dao/RecordDaoTest.java | 287 ++++++++++++++---- 2 files changed, 236 insertions(+), 54 deletions(-) diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/dao/RecordDao.java b/service/src/main/java/org/databiosphere/workspacedataservice/dao/RecordDao.java index 2735868a2..17df346d7 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/dao/RecordDao.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/dao/RecordDao.java @@ -1104,7 +1104,8 @@ public void updateAttributeDataType( } } - private String getPostgresTypeConversionExpression( + @VisibleForTesting + String getPostgresTypeConversionExpression( String attribute, DataTypeMapping dataType, DataTypeMapping newDataType) { // Some data types are not yet supported. // Some conversions don't make sense / are invalid. diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dao/RecordDaoTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/dao/RecordDaoTest.java index 74fac62ed..5ff736dcb 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dao/RecordDaoTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dao/RecordDaoTest.java @@ -1,5 +1,6 @@ package org.databiosphere.workspacedataservice.dao; +import static org.databiosphere.workspacedataservice.service.model.DataTypeMapping.*; import static org.databiosphere.workspacedataservice.service.model.DataTypeMapping.ARRAY_OF_NUMBER; import static org.databiosphere.workspacedataservice.service.model.DataTypeMapping.ARRAY_OF_STRING; import static org.databiosphere.workspacedataservice.service.model.ReservedNames.RECORD_ID; @@ -17,6 +18,7 @@ import java.util.UUID; import java.util.stream.Collectors; import java.util.stream.IntStream; +import java.util.stream.Stream; import org.databiosphere.workspacedataservice.service.DataTypeInferer; import org.databiosphere.workspacedataservice.service.RelationUtils; import org.databiosphere.workspacedataservice.service.model.DataTypeMapping; @@ -32,7 +34,9 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.test.context.SpringBootTest; @@ -176,7 +180,7 @@ void deleteAndQueryFunkyPrimaryKeyValues() { Record testRecord = new Record(recordId, funkyPk, RecordAttributes.empty()); recordDao.createRecordType( instanceId, - Map.of("attr1", DataTypeMapping.STRING), + Map.of("attr1", STRING), funkyPk, new RelationCollection(Collections.emptySet(), Collections.emptySet()), sample_id); @@ -199,7 +203,7 @@ void batchDeleteAndTestRelationsFunkyPrimaryKeyValues() { String sample_id = "Sample ID"; String recordId = "1199"; Record testRecord = new Record(recordId, referencedRt, RecordAttributes.empty()); - Map schema = Map.of("attr1", DataTypeMapping.RELATION); + Map schema = Map.of("attr1", RELATION); recordDao.createRecordType( instanceId, schema, @@ -251,7 +255,7 @@ void batchDeleteWithRelationArrays() { recordDao.batchUpsert(instanceId, referencedRt, referencedRecords, Collections.emptyMap()); // Create records to do the referencing - Map schema = Map.of("attr1", DataTypeMapping.ARRAY_OF_RELATION); + Map schema = Map.of("attr1", ARRAY_OF_RELATION); RecordType referencer = RecordType.valueOf("referencer"); Relation arrayRel = new Relation("attr1", referencedRt); recordDao.createRecordType( @@ -300,8 +304,8 @@ void batchDeleteWithRelationArrays() { void testGetRecordsWithRelations() { // Create two records of the same type, one with a value for a relation // attribute, the other without - recordDao.addColumn(instanceId, recordType, "foo", DataTypeMapping.STRING); - recordDao.addColumn(instanceId, recordType, "relationAttr", DataTypeMapping.RELATION); + recordDao.addColumn(instanceId, recordType, "foo", STRING); + recordDao.addColumn(instanceId, recordType, "relationAttr", RELATION); recordDao.addForeignKeyForReference(recordType, recordType, instanceId, "relationAttr"); String refRecordId = "referencedRecord"; @@ -321,8 +325,7 @@ void testGetRecordsWithRelations() { instanceId, recordType, List.of(referencedRecord, testRecord), - new HashMap<>( - Map.of("foo", DataTypeMapping.STRING, "relationAttr", DataTypeMapping.RELATION))); + new HashMap<>(Map.of("foo", STRING, "relationAttr", RELATION))); List relations = recordDao.getRelationCols(instanceId, recordType); @@ -341,7 +344,7 @@ void testGetRecordsWithRelations() { @Test @Transactional void testCreateSingleRecord() throws InvalidRelationException { - recordDao.addColumn(instanceId, recordType, "foo", DataTypeMapping.STRING); + recordDao.addColumn(instanceId, recordType, "foo", STRING); // create record with no attributes String recordId = "testRecord"; @@ -360,7 +363,7 @@ void testCreateSingleRecord() throws InvalidRelationException { instanceId, recordType, Collections.singletonList(recordWithAttr), - new HashMap<>(Map.of("foo", DataTypeMapping.STRING))); + new HashMap<>(Map.of("foo", STRING))); search = recordDao.getSingleRecord(instanceId, recordType, attrId).get(); assertEquals( @@ -372,9 +375,9 @@ void testCreateSingleRecord() throws InvalidRelationException { void testCreateRecordWithRelations() { // make sure columns are in recordType, as this will be taken care of before we // get to the dao - recordDao.addColumn(instanceId, recordType, "foo", DataTypeMapping.STRING); + recordDao.addColumn(instanceId, recordType, "foo", STRING); - recordDao.addColumn(instanceId, recordType, "testRecordType", DataTypeMapping.RELATION); + recordDao.addColumn(instanceId, recordType, "testRecordType", RELATION); recordDao.addForeignKeyForReference(recordType, recordType, instanceId, "testRecordType"); String refRecordId = "referencedRecord"; @@ -393,8 +396,7 @@ void testCreateRecordWithRelations() { instanceId, recordType, Collections.singletonList(testRecord), - new HashMap<>( - Map.of("foo", DataTypeMapping.STRING, "testRecordType", DataTypeMapping.RELATION))); + new HashMap<>(Map.of("foo", STRING, "testRecordType", RELATION))); Record search = recordDao.getSingleRecord(instanceId, recordType, recordId).get(); assertEquals(testRecord, search, "Created record with references should match entered record"); @@ -403,7 +405,7 @@ void testCreateRecordWithRelations() { @Test @Transactional void testGetReferenceCols() { - recordDao.addColumn(instanceId, recordType, "referenceCol", DataTypeMapping.STRING); + recordDao.addColumn(instanceId, recordType, "referenceCol", STRING); recordDao.addForeignKeyForReference(recordType, recordType, instanceId, "referenceCol"); List refCols = recordDao.getRelationCols(instanceId, recordType); @@ -434,14 +436,10 @@ void testDeleteSingleRecord() { void testDeleteRelatedRecord() { // make sure columns are in recordType, as this will be taken care of before we // get to the dao - recordDao.addColumn(instanceId, recordType, "foo", DataTypeMapping.STRING); + recordDao.addColumn(instanceId, recordType, "foo", STRING); recordDao.addColumn( - instanceId, - recordType, - "testRecordType", - DataTypeMapping.RELATION, - RecordType.valueOf("testRecordType")); + instanceId, recordType, "testRecordType", RELATION, RecordType.valueOf("testRecordType")); String refRecordId = "referencedRecord"; Record referencedRecord = @@ -459,8 +457,7 @@ void testDeleteRelatedRecord() { instanceId, recordType, Collections.singletonList(testRecord), - new HashMap<>( - Map.of("foo", DataTypeMapping.STRING, "testRecordType", DataTypeMapping.RELATION))); + new HashMap<>(Map.of("foo", STRING, "testRecordType", RELATION))); // Should throw an error assertThrows( @@ -486,7 +483,7 @@ void testDeleteRecordReferencedInArray() { // Create record type RecordType relationArrayType = RecordType.valueOf("relationArrayType"); Relation arrayRelation = new Relation("relArrAttr", recordType); - Map schema = Map.of("relArrAttr", DataTypeMapping.ARRAY_OF_RELATION); + Map schema = Map.of("relArrAttr", ARRAY_OF_RELATION); recordDao.createRecordType( instanceId, schema, @@ -539,7 +536,7 @@ void testDeleteRecordWithRelationArray() { // Create record type RecordType relationArrayType = RecordType.valueOf("relationArrayType"); Relation arrayRelation = new Relation("relArrAttr", recordType); - Map schema = Map.of("relArrAttr", DataTypeMapping.ARRAY_OF_RELATION); + Map schema = Map.of("relArrAttr", ARRAY_OF_RELATION); recordDao.createRecordType( instanceId, schema, @@ -611,7 +608,7 @@ void testGetAllRecordTypesNoJoins() { Relation arrayRelation = new Relation("relArrAttr", recordType); recordDao.createRecordType( instanceId, - Map.of("relArrAttr", DataTypeMapping.ARRAY_OF_RELATION), + Map.of("relArrAttr", ARRAY_OF_RELATION), relationArrayType, new RelationCollection(Collections.emptySet(), Set.of(arrayRelation)), RECORD_ID); @@ -672,8 +669,7 @@ void testDeleteRecordTypeWithRelation() { new RelationCollection(Collections.emptySet(), Collections.emptySet()), RECORD_ID); - recordDao.addColumn( - instanceId, recordTypeName, "relation", DataTypeMapping.RELATION, referencedType); + recordDao.addColumn(instanceId, recordTypeName, "relation", RELATION, referencedType); String refRecordId = "referencedRecord"; Record referencedRecord = new Record(refRecordId, referencedType, RecordAttributes.empty()); @@ -688,7 +684,7 @@ void testDeleteRecordTypeWithRelation() { instanceId, recordTypeName, Collections.singletonList(testRecord), - new HashMap<>(Map.of("relation", DataTypeMapping.RELATION))); + new HashMap<>(Map.of("relation", RELATION))); // Should throw an error assertThrows( @@ -712,7 +708,7 @@ void testDeleteRecordTypeWithRelationArray() { // Create referencing record type RecordType relationArrayType = RecordType.valueOf("relationArrayType"); Relation arrayRelation = new Relation("relArrAttr", recordType); - Map schema = Map.of("relArrAttr", DataTypeMapping.ARRAY_OF_RELATION); + Map schema = Map.of("relArrAttr", ARRAY_OF_RELATION); recordDao.createRecordType( instanceId, schema, @@ -758,7 +754,7 @@ void testRenameAttribute() { RecordType recordTypeWithAttributes = RecordType.valueOf("withAttributes"); recordDao.createRecordType( instanceId, - Map.of("foo", DataTypeMapping.STRING, "bar", DataTypeMapping.STRING), + Map.of("foo", STRING, "bar", STRING), recordTypeWithAttributes, new RelationCollection(Collections.emptySet(), Collections.emptySet()), PRIMARY_KEY); @@ -772,6 +768,207 @@ void testRenameAttribute() { assertEquals(Set.of(PRIMARY_KEY, "foo", "baz"), attributeNames); } + @ParameterizedTest(name = "returns expression for converting {0} to {1}") + @MethodSource({ + "stringConversionExpressions", + "stringArrayConversionExpressions", + "numberConversionExpressions", + "numberArrayConversionExpressions", + "booleanConversionExpressions", + "booleanArrayConversionExpressions", + "dateConversionExpressions", + "dateArrayConversionExpressions", + "datetimeConversionExpressions", + "datetimeArrayConversionExpressions" + }) + void testGetPostgresTypeConversionExpression( + DataTypeMapping dataType, DataTypeMapping newDataType, String expectedExpression) { + // Act + String actualExpression = + recordDao.getPostgresTypeConversionExpression("attr", dataType, newDataType); + + // Assert + assertEquals(expectedExpression, actualExpression); + } + + static Stream stringConversionExpressions() { + return Stream.of( + // Number + args(STRING, NUMBER, "\"attr\"::numeric"), + // Boolean + args(STRING, BOOLEAN, "\"attr\"::boolean"), + // Date + args(STRING, DATE, "\"attr\"::date"), + // Datetime + args(STRING, DATE_TIME, "\"attr\"::timestamp with time zone"), + // String array + args(STRING, ARRAY_OF_STRING, "array_append('{}', \"attr\")::text[]"), + // Number array + args(STRING, ARRAY_OF_NUMBER, "array_append('{}', \"attr\")::numeric[]"), + // Boolean array + args(STRING, ARRAY_OF_BOOLEAN, "array_append('{}', \"attr\")::boolean[]"), + // Date array + args(STRING, ARRAY_OF_DATE, "array_append('{}', \"attr\")::date[]"), + // Datetime array + args( + STRING, + ARRAY_OF_DATE_TIME, + "array_append('{}', \"attr\")::timestamp with time zone[]")); + } + + static Stream stringArrayConversionExpressions() { + return Stream.of( + // Number array + args(ARRAY_OF_STRING, ARRAY_OF_NUMBER, "\"attr\"::numeric[]"), + // Boolean array + args(ARRAY_OF_STRING, ARRAY_OF_BOOLEAN, "\"attr\"::boolean[]"), + // Date array + args(ARRAY_OF_STRING, ARRAY_OF_DATE, "\"attr\"::date[]"), + // Datetime array + args(ARRAY_OF_STRING, ARRAY_OF_DATE_TIME, "\"attr\"::timestamp with time zone[]")); + } + + static Stream numberConversionExpressions() { + return Stream.of( + // String + args(NUMBER, STRING, "\"attr\"::text"), + // Boolean + args(NUMBER, BOOLEAN, "\"attr\"::int::boolean"), + // Date + args(NUMBER, DATE, "to_timestamp(\"attr\")::date"), + // Datetime + args(NUMBER, DATE_TIME, "to_timestamp(\"attr\")::timestamp with time zone"), + // String array + args(NUMBER, ARRAY_OF_STRING, "array_append('{}', \"attr\")::text[]"), + // Number array + args(NUMBER, ARRAY_OF_NUMBER, "array_append('{}', \"attr\")::numeric[]"), + // Boolean array + args(NUMBER, ARRAY_OF_BOOLEAN, "array_append('{}', \"attr\")::int[]::boolean[]"), + // Date array + args(NUMBER, ARRAY_OF_DATE, "array_append('{}', to_timestamp(\"attr\"))::date[]"), + // Datetime array + args( + NUMBER, + ARRAY_OF_DATE_TIME, + "array_append('{}', to_timestamp(\"attr\"))::timestamp with time zone[]")); + } + + static Stream numberArrayConversionExpressions() { + return Stream.of( + // String array + args(ARRAY_OF_NUMBER, ARRAY_OF_STRING, "\"attr\"::text[]"), + // Boolean array + args(ARRAY_OF_NUMBER, ARRAY_OF_BOOLEAN, "\"attr\"::int[]::boolean[]"), + // Date array + args( + ARRAY_OF_NUMBER, + ARRAY_OF_DATE, + "(sys_wds.convert_array_of_numbers_to_timestamps(\"attr\"))::date[]"), + // Datetime array + args( + ARRAY_OF_NUMBER, + ARRAY_OF_DATE_TIME, + "(sys_wds.convert_array_of_numbers_to_timestamps(\"attr\"))::timestamp with time zone[]")); + } + + static Stream booleanConversionExpressions() { + return Stream.of( + // String + args(BOOLEAN, STRING, "\"attr\"::text"), + // Number + args(BOOLEAN, NUMBER, "\"attr\"::int::numeric"), + // String array + args(BOOLEAN, ARRAY_OF_STRING, "array_append('{}', \"attr\")::text[]"), + // Number array + args(BOOLEAN, ARRAY_OF_NUMBER, "array_append('{}', \"attr\")::int[]::numeric[]"), + // Boolean array + args(BOOLEAN, ARRAY_OF_BOOLEAN, "array_append('{}', \"attr\")::boolean[]")); + } + + static Stream booleanArrayConversionExpressions() { + return Stream.of( + // String array + args(ARRAY_OF_BOOLEAN, ARRAY_OF_STRING, "\"attr\"::text[]"), + // Number array + args(ARRAY_OF_BOOLEAN, ARRAY_OF_NUMBER, "\"attr\"::int[]::numeric[]")); + } + + static Stream dateConversionExpressions() { + return Stream.of( + // String + args(DATE, STRING, "\"attr\"::text"), + // Number + args(DATE, NUMBER, "extract(epoch from \"attr\")::bigint::numeric"), + // Datetime + args(DATE, DATE_TIME, "\"attr\"::timestamp with time zone"), + // String array + args(DATE, ARRAY_OF_STRING, "array_append('{}', \"attr\")::text[]"), + // Number array + args( + DATE, + ARRAY_OF_NUMBER, + "array_append('{}', extract(epoch from \"attr\")::bigint)::numeric[]"), + // Date array + args(DATE, ARRAY_OF_DATE, "array_append('{}', \"attr\")::date[]"), + // Datetime array + args(DATE, ARRAY_OF_DATE_TIME, "array_append('{}', \"attr\")::timestamp with time zone[]")); + } + + static Stream dateArrayConversionExpressions() { + return Stream.of( + // String array + args(ARRAY_OF_DATE, ARRAY_OF_STRING, "\"attr\"::text[]"), + // Number array + args( + ARRAY_OF_DATE, + ARRAY_OF_NUMBER, + "(sys_wds.convert_array_of_timestamps_to_numbers(\"attr\")::bigint[])::numeric[]"), + // Datetime array + args(ARRAY_OF_DATE, ARRAY_OF_DATE_TIME, "\"attr\"::timestamp with time zone[]")); + } + + static Stream datetimeConversionExpressions() { + return Stream.of( + // String + args(DATE_TIME, STRING, "\"attr\"::text"), + // Number + args(DATE_TIME, NUMBER, "extract(epoch from \"attr\")::numeric"), + // Date + args(DATE_TIME, DATE, "\"attr\"::date"), + // String array + args(DATE_TIME, ARRAY_OF_STRING, "array_append('{}', \"attr\")::text[]"), + // Number array + args( + DATE_TIME, + ARRAY_OF_NUMBER, + "array_append('{}', extract(epoch from \"attr\"))::numeric[]"), + // Date array + args(DATE_TIME, ARRAY_OF_DATE, "array_append('{}', \"attr\")::date[]"), + // Datetime array + args( + DATE_TIME, + ARRAY_OF_DATE_TIME, + "array_append('{}', \"attr\")::timestamp with time zone[]")); + } + + static Stream datetimeArrayConversionExpressions() { + return Stream.of( + // String array + args(ARRAY_OF_DATE_TIME, ARRAY_OF_STRING, "\"attr\"::text[]"), + // Number array + args( + ARRAY_OF_DATE_TIME, + ARRAY_OF_NUMBER, + "(sys_wds.convert_array_of_timestamps_to_numbers(\"attr\"))::numeric[]"), + // Date array + args(ARRAY_OF_DATE_TIME, ARRAY_OF_DATE, "\"attr\"::date[]")); + } + + /** Simple helper to provide an even terser shorthand */ + private static Arguments args(Object... arguments) { + return Arguments.of(arguments); + } + @Test @Transactional void testDeleteAttribute() { @@ -779,7 +976,7 @@ void testDeleteAttribute() { RecordType recordTypeWithAttributes = RecordType.valueOf("withAttributes"); recordDao.createRecordType( instanceId, - Map.of("attr1", DataTypeMapping.STRING, "attr2", DataTypeMapping.STRING), + Map.of("attr1", STRING, "attr2", STRING), recordTypeWithAttributes, new RelationCollection(Collections.emptySet(), Collections.emptySet()), PRIMARY_KEY); @@ -820,13 +1017,7 @@ void testCreateRecordTypeWithRelationArray() { Relation arrayRelation = new Relation("relArrAttr", recordType); recordDao.createRecordType( instanceId, - Map.of( - "stringAttr", - DataTypeMapping.STRING, - "refAttr", - DataTypeMapping.RELATION, - "relArrAttr", - DataTypeMapping.ARRAY_OF_RELATION), + Map.of("stringAttr", STRING, "refAttr", RELATION, "relArrAttr", ARRAY_OF_RELATION), relationarrayType, new RelationCollection(Set.of(singleRelation), Set.of(arrayRelation)), RECORD_ID); @@ -834,9 +1025,9 @@ void testCreateRecordTypeWithRelationArray() { Map schema = recordDao.getExistingTableSchemaLessPrimaryKey(instanceId, relationarrayType); assertEquals(3, schema.size()); - assertEquals(DataTypeMapping.STRING, schema.get("stringAttr")); - assertEquals(DataTypeMapping.RELATION, schema.get("refAttr")); - assertEquals(DataTypeMapping.ARRAY_OF_RELATION, schema.get("relArrAttr")); + assertEquals(STRING, schema.get("stringAttr")); + assertEquals(RELATION, schema.get("refAttr")); + assertEquals(ARRAY_OF_RELATION, schema.get("relArrAttr")); List relationCols = recordDao.getRelationCols(instanceId, relationarrayType); assertEquals(List.of(singleRelation), relationCols); List relationArrayCols = @@ -862,13 +1053,7 @@ void testCreateAndGetRecordWithRelationArray() { RecordType relationArrayType = RecordType.valueOf("relationArrayType"); Relation arrayRelation = new Relation("relArrAttr", recordType); Map schema = - Map.of( - "stringAttr", - DataTypeMapping.STRING, - "refAttr", - DataTypeMapping.RELATION, - "relArrAttr", - DataTypeMapping.ARRAY_OF_RELATION); + Map.of("stringAttr", STRING, "refAttr", RELATION, "relArrAttr", ARRAY_OF_RELATION); recordDao.createRecordType( instanceId, schema, @@ -924,11 +1109,7 @@ void testGetRelationArrayColumns() { Relation arrayRelation2 = new Relation("relArr2", recordType); recordDao.createRecordType( instanceId, - Map.of( - "relArr1", - DataTypeMapping.ARRAY_OF_RELATION, - "relArr2", - DataTypeMapping.ARRAY_OF_RELATION), + Map.of("relArr1", ARRAY_OF_RELATION, "relArr2", ARRAY_OF_RELATION), relationarrayType, new RelationCollection(Collections.emptySet(), Set.of(arrayRelation1, arrayRelation2)), RECORD_ID);