From d9dfd48bf30324af7b7a10570a0111bb46a9c0e5 Mon Sep 17 00:00:00 2001 From: Jerry Shao Date: Wed, 6 Sep 2023 22:53:01 +0800 Subject: [PATCH] [#317] feat(core, catalog): make fields in AuditInfo optional and overwriteable (#330) ### What changes were proposed in this pull request? This PR proposes to change the requirements of the `AuditInfo` fields to make them optional and overwriteable. ### Why are the changes needed? This is the first change of #250, the change is going to address two problems: 1. If the `AuditInfo` is not existed in both Graviton store and underlying source, we should support the empty `AuditInfo`, or only several fields are set in `AuditInfo`. 2. If the `AuditInfo` are both set in the Graviton store and underlying source, we should support `AuditInfo` mergeable. Fix: #317 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Modify and add the UTs to test. --- .../catalog/hive/HiveCatalogOperations.java | 46 +++++++++++-------- .../graviton/catalog/hive/HiveSchema.java | 7 +++ .../graviton/catalog/hive/HiveTable.java | 16 +++++++ .../graviton/catalog/hive/HiveSchemaTest.java | 25 ++++++++-- .../graviton/catalog/hive/HiveTableTest.java | 25 ++++++++-- .../dto/responses/CatalogResponse.java | 5 -- .../dto/responses/MetalakeListResponse.java | 6 --- .../dto/responses/MetalakeResponse.java | 5 -- .../dto/responses/SchemaResponse.java | 5 -- .../graviton/dto/responses/TableResponse.java | 5 -- .../datastrato/graviton/meta/AuditInfo.java | 39 ++++++++++++---- .../graviton/proto/AuditInfoSerDe.java | 37 +++++++-------- .../graviton/meta/TestAuditInfo.java | 31 +++++-------- .../graviton/proto/TestEntityProtoSerDe.java | 9 ++++ meta/src/main/proto/graviton_meta.proto | 4 +- 15 files changed, 167 insertions(+), 98 deletions(-) diff --git a/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index ab5929425d..83164b179b 100644 --- a/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -241,10 +241,12 @@ public HiveSchema loadSchema(NameIdentifier ident) throws NoSuchSchemaException .withId(baseSchema.getId()) .withCatalogId(baseSchema.getCatalogId()) .withNamespace(ident.namespace()) - .withAuditInfo(baseSchema.auditInfo()) .withConf(hiveConf); HiveSchema hiveSchema = HiveSchema.fromInnerDB(database, builder); + // Merge audit info from Graviton store + hiveSchema.auditInfo().merge(baseSchema.auditInfo(), true /*overwrite*/); + LOG.info("Loaded Hive schema (database) {} from Hive Metastore ", ident.name()); return hiveSchema; @@ -331,16 +333,18 @@ public HiveSchema alterSchema(NameIdentifier ident, SchemaChange... changes) .withId(oldSchema.getId()) .withCatalogId(oldSchema.getCatalogId()) .withNamespace(ident.namespace()) - .withAuditInfo( - new AuditInfo.Builder() - .withCreator(oldSchema.auditInfo().creator()) - .withCreateTime(oldSchema.auditInfo().createTime()) - .withLastModifier(currentUser()) - .withLastModifiedTime(Instant.now()) - .build()) .withConf(hiveConf); HiveSchema hiveSchema = HiveSchema.fromInnerDB(alteredDatabase, builder); + AuditInfo newAudit = + new AuditInfo.Builder() + .withCreator(oldSchema.auditInfo().creator()) + .withCreateTime(oldSchema.auditInfo().createTime()) + .withLastModifier(currentUser()) + .withLastModifiedTime(Instant.now()) + .build(); + hiveSchema.auditInfo().merge(newAudit, true /*overwrite*/); + // To be on the safe side, here uses delete before put (although hive schema does // not support rename yet) store.delete(ident, SCHEMA); @@ -552,10 +556,12 @@ public Table loadTable(NameIdentifier tableIdent) throws NoSuchTableException { .withId(baseTable.getId()) .withSchemaId(baseTable.getSchemaId()) .withName(tableIdent.name()) - .withNameSpace(tableIdent.namespace()) - .withAuditInfo(baseTable.auditInfo()); + .withNameSpace(tableIdent.namespace()); HiveTable table = HiveTable.fromInnerTable(hiveTable, builder); + // Merge the audit info from Graviton store. + table.auditInfo().merge(baseTable.auditInfo(), true /* overwrite */); + LOG.info("Loaded Hive table {} from Hive Metastore ", tableIdent.name()); return table; @@ -728,15 +734,19 @@ public Table alterTable(NameIdentifier tableIdent, TableChange... changes) .withId(table.getId()) .withSchemaId(table.getSchemaId()) .withName(alteredHiveTable.getTableName()) - .withNameSpace(tableIdent.namespace()) - .withAuditInfo( - new AuditInfo.Builder() - .withCreator(table.auditInfo().creator()) - .withCreateTime(table.auditInfo().createTime()) - .withLastModifier(currentUser()) - .withLastModifiedTime(Instant.now()) - .build()); + .withNameSpace(tableIdent.namespace()); + HiveTable alteredTable = HiveTable.fromInnerTable(alteredHiveTable, builder); + + AuditInfo newAudit = + new AuditInfo.Builder() + .withCreator(table.auditInfo().creator()) + .withCreateTime(table.auditInfo().createTime()) + .withLastModifier(currentUser()) + .withLastModifiedTime(Instant.now()) + .build(); + alteredTable.auditInfo().merge(newAudit, true /* overwrite */); + store.delete(tableIdent, TABLE); store.put(alteredTable, false); clientPool.run( diff --git a/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveSchema.java b/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveSchema.java index ec2beec8be..c875ae4077 100644 --- a/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveSchema.java +++ b/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveSchema.java @@ -4,6 +4,7 @@ */ package com.datastrato.graviton.catalog.hive; +import com.datastrato.graviton.meta.AuditInfo; import com.datastrato.graviton.meta.rel.BaseSchema; import com.google.common.base.Preconditions; import com.google.common.collect.Maps; @@ -38,10 +39,16 @@ private HiveSchema() {} public static HiveSchema fromInnerDB(Database db, Builder builder) { Map properties = convertToMetadata(db); + // Get audit info from Hive's Database object. Because Hive's database doesn't store create + // time, last modifier and last modified time, we only get creator from Hive's database. + AuditInfo.Builder auditInfoBuilder = new AuditInfo.Builder(); + Optional.ofNullable(db.getOwnerName()).ifPresent(auditInfoBuilder::withCreator); + return builder .withName(db.getName()) .withComment(db.getDescription()) .withProperties(properties) + .withAuditInfo(auditInfoBuilder.build()) .build(); } diff --git a/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTable.java b/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTable.java index 22e5797f2e..ca65601b70 100644 --- a/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTable.java +++ b/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTable.java @@ -10,10 +10,13 @@ import com.datastrato.graviton.NameIdentifier; import com.datastrato.graviton.catalog.hive.converter.FromHiveType; import com.datastrato.graviton.catalog.hive.converter.ToHiveType; +import com.datastrato.graviton.meta.AuditInfo; import com.datastrato.graviton.meta.rel.BaseTable; import com.google.common.collect.Lists; import com.google.common.collect.Sets; +import java.time.Instant; import java.util.Arrays; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import lombok.Getter; @@ -47,6 +50,13 @@ private HiveTable() {} * @return A new HiveTable instance. */ public static HiveTable fromInnerTable(Table table, Builder builder) { + // Get audit info from Hive's Table object. Because Hive's table doesn't store last modifier + // and last modified time, we only get creator and create time from Hive's table. + AuditInfo.Builder auditInfoBuilder = new AuditInfo.Builder(); + Optional.ofNullable(table.getOwner()).ifPresent(auditInfoBuilder::withCreator); + if (table.isSetCreateTime()) { + auditInfoBuilder.withCreateTime(Instant.ofEpochSecond(table.getCreateTime())); + } return builder .withComment(table.getParameters().get(HMS_TABLE_COMMENT)) @@ -62,6 +72,7 @@ public static HiveTable fromInnerTable(Table table, Builder builder) { .build()) .toArray(HiveColumn[]::new)) .withLocation(table.getSd().getLocation()) + .withAuditInfo(auditInfoBuilder.build()) .build(); } @@ -79,6 +90,11 @@ public Table toInnerTable() { hiveTable.setParameters(properties); hiveTable.setPartitionKeys(Lists.newArrayList() /* TODO(Minghuang): Add partition support */); + // Set AuditInfo to Hive's Table object. Hive's Table doesn't support setting last modifier + // and last modified time, so we only set creator and create time. + hiveTable.setOwner(auditInfo.creator()); + hiveTable.setCreateTime(Math.toIntExact(auditInfo.createTime().getEpochSecond())); + return hiveTable; } diff --git a/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveSchemaTest.java b/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveSchemaTest.java index a2c0aae6b8..f21be06079 100644 --- a/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveSchemaTest.java +++ b/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveSchemaTest.java @@ -105,6 +105,9 @@ public void testCreateHiveSchema() throws IOException { Assertions.assertTrue(Arrays.asList(idents).contains(ident)); Assertions.assertTrue(store.exists(ident, SCHEMA)); + Schema loadedSchema = hiveCatalog.asSchemas().loadSchema(ident); + Assertions.assertEquals(schema.auditInfo(), loadedSchema.auditInfo()); + // Test illegal identifier NameIdentifier ident1 = NameIdentifier.of("metalake", hiveCatalog.name()); Throwable exception = @@ -136,7 +139,7 @@ public void testAlterSchema() { properties.put("key2", "val2"); String comment = "comment"; - hiveCatalog.asSchemas().createSchema(ident, comment, properties); + Schema createdSchema = hiveCatalog.asSchemas().createSchema(ident, comment, properties); Assertions.assertTrue(hiveCatalog.asSchemas().schemaExists(ident)); Map properties1 = hiveCatalog.asSchemas().loadSchema(ident).properties(); @@ -149,19 +152,35 @@ public void testAlterSchema() { ident, SchemaChange.removeProperty("key1"), SchemaChange.setProperty("key2", "val2-alter")); - Map properties2 = hiveCatalog.asSchemas().loadSchema(ident).properties(); + Schema alteredSchema = hiveCatalog.asSchemas().loadSchema(ident); + Map properties2 = alteredSchema.properties(); Assertions.assertFalse(properties2.containsKey("key1")); Assertions.assertEquals("val2-alter", properties2.get("key2")); + Assertions.assertEquals( + createdSchema.auditInfo().creator(), alteredSchema.auditInfo().creator()); + Assertions.assertEquals( + createdSchema.auditInfo().createTime(), alteredSchema.auditInfo().createTime()); + Assertions.assertNotNull(alteredSchema.auditInfo().lastModifier()); + Assertions.assertNotNull(alteredSchema.auditInfo().lastModifiedTime()); + hiveCatalog .asSchemas() .alterSchema( ident, SchemaChange.setProperty("key3", "val3"), SchemaChange.setProperty("key4", "val4")); - Map properties3 = hiveCatalog.asSchemas().loadSchema(ident).properties(); + Schema alteredSchema1 = hiveCatalog.asSchemas().loadSchema(ident); + Map properties3 = alteredSchema1.properties(); Assertions.assertEquals("val3", properties3.get("key3")); Assertions.assertEquals("val4", properties3.get("key4")); + + Assertions.assertEquals( + createdSchema.auditInfo().creator(), alteredSchema1.auditInfo().creator()); + Assertions.assertEquals( + createdSchema.auditInfo().createTime(), alteredSchema1.auditInfo().createTime()); + Assertions.assertNotNull(alteredSchema1.auditInfo().lastModifier()); + Assertions.assertNotNull(alteredSchema1.auditInfo().lastModifiedTime()); } @Test diff --git a/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveTableTest.java b/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveTableTest.java index 4383d929eb..d96b3cabba 100644 --- a/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveTableTest.java +++ b/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/HiveTableTest.java @@ -146,6 +146,9 @@ public void testCreateHiveTable() throws IOException { Assertions.assertEquals(HIVE_COMMENT, table.comment()); Assertions.assertArrayEquals(columns, table.columns()); + Table loadedTable = hiveCatalog.asTableCatalog().loadTable(tableIdentifier); + Assertions.assertEquals(table.auditInfo(), loadedTable.auditInfo()); + Assertions.assertTrue(hiveCatalog.asTableCatalog().tableExists(tableIdentifier)); NameIdentifier[] tableIdents = hiveCatalog.asTableCatalog().listTables(tableIdentifier.namespace()); @@ -244,7 +247,10 @@ public void testAlterHiveTable() throws IOException { .build(); Column[] columns = new Column[] {col1, col2}; - hiveCatalog.asTableCatalog().createTable(tableIdentifier, columns, HIVE_COMMENT, properties); + Table createdTable = + hiveCatalog + .asTableCatalog() + .createTable(tableIdentifier, columns, HIVE_COMMENT, properties); Assertions.assertTrue(hiveCatalog.asTableCatalog().tableExists(tableIdentifier)); // test alter @@ -283,6 +289,12 @@ public void testAlterHiveTable() throws IOException { Assertions.assertFalse(store.exists(tableIdentifier, TABLE)); Assertions.assertTrue(store.exists(((HiveTable) alteredTable).nameIdentifier(), TABLE)); + Assertions.assertEquals(createdTable.auditInfo().creator(), alteredTable.auditInfo().creator()); + Assertions.assertEquals( + createdTable.auditInfo().createTime(), alteredTable.auditInfo().createTime()); + Assertions.assertNotNull(alteredTable.auditInfo().lastModifier()); + Assertions.assertNotNull(alteredTable.auditInfo().lastModifiedTime()); + Column[] expected = new Column[] { new HiveColumn.Builder() @@ -309,12 +321,19 @@ public void testAlterHiveTable() throws IOException { .alterTable( NameIdentifier.of(tableIdentifier.namespace(), "test_hive_table_new"), TableChange.deleteColumn(new String[] {"col_1"}, false)); - alteredTable = + Table alteredTable1 = hiveCatalog .asTableCatalog() .loadTable(NameIdentifier.of(tableIdentifier.namespace(), "test_hive_table_new")); expected = Arrays.stream(expected).filter(c -> !"col_1".equals(c.name())).toArray(Column[]::new); - Assertions.assertArrayEquals(expected, alteredTable.columns()); + Assertions.assertArrayEquals(expected, alteredTable1.columns()); + + Assertions.assertEquals( + createdTable.auditInfo().creator(), alteredTable1.auditInfo().creator()); + Assertions.assertEquals( + createdTable.auditInfo().createTime(), alteredTable1.auditInfo().createTime()); + Assertions.assertNotNull(alteredTable1.auditInfo().lastModifier()); + Assertions.assertNotNull(alteredTable1.auditInfo().lastModifiedTime()); } } diff --git a/common/src/main/java/com/datastrato/graviton/dto/responses/CatalogResponse.java b/common/src/main/java/com/datastrato/graviton/dto/responses/CatalogResponse.java index 02ad04036f..7258846a6e 100644 --- a/common/src/main/java/com/datastrato/graviton/dto/responses/CatalogResponse.java +++ b/common/src/main/java/com/datastrato/graviton/dto/responses/CatalogResponse.java @@ -51,10 +51,5 @@ public void validate() throws IllegalArgumentException { StringUtils.isNotBlank(catalog.name()), "catalog 'name' must not be null and empty"); Preconditions.checkArgument(catalog.type() != null, "catalog 'type' must not be null"); Preconditions.checkArgument(catalog.auditInfo() != null, "catalog 'audit' must not be null"); - Preconditions.checkArgument( - StringUtils.isNotBlank(catalog.auditInfo().creator()), - "catalog 'audit.creator' must not be null and empty"); - Preconditions.checkArgument( - catalog.auditInfo().createTime() != null, "catalog 'audit.createTime' must not be null"); } } diff --git a/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeListResponse.java b/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeListResponse.java index d8ef74be4c..5bf7a135a0 100644 --- a/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeListResponse.java +++ b/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeListResponse.java @@ -56,12 +56,6 @@ public void validate() throws IllegalArgumentException { "metalake 'name' must not be null and empty"); Preconditions.checkArgument( metalake.auditInfo() != null, "metalake 'audit' must not be null"); - Preconditions.checkArgument( - StringUtils.isNotBlank(metalake.auditInfo().creator()), - "metalake 'audit.creator' must not be null and empty"); - Preconditions.checkArgument( - metalake.auditInfo().createTime() != null, - "metalake 'audit.createTime' must not be null"); }); } } diff --git a/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeResponse.java b/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeResponse.java index e6a3b82f4f..06239a6805 100644 --- a/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeResponse.java +++ b/common/src/main/java/com/datastrato/graviton/dto/responses/MetalakeResponse.java @@ -50,10 +50,5 @@ public void validate() throws IllegalArgumentException { Preconditions.checkArgument( StringUtils.isNotBlank(metalake.name()), "metalake 'name' must not be null and empty"); Preconditions.checkArgument(metalake.auditInfo() != null, "metalake 'audit' must not be null"); - Preconditions.checkArgument( - StringUtils.isNotBlank(metalake.auditInfo().creator()), - "metalake 'audit.creator' must not be null and empty"); - Preconditions.checkArgument( - metalake.auditInfo().createTime() != null, "metalake 'audit.createTime' must not be null"); } } diff --git a/common/src/main/java/com/datastrato/graviton/dto/responses/SchemaResponse.java b/common/src/main/java/com/datastrato/graviton/dto/responses/SchemaResponse.java index 3480c202f0..54a1468a22 100644 --- a/common/src/main/java/com/datastrato/graviton/dto/responses/SchemaResponse.java +++ b/common/src/main/java/com/datastrato/graviton/dto/responses/SchemaResponse.java @@ -39,10 +39,5 @@ public void validate() throws IllegalArgumentException { Preconditions.checkArgument( StringUtils.isNotBlank(schema.name()), "schema 'name' must not be null and empty"); Preconditions.checkArgument(schema.auditInfo() != null, "schema 'audit' must not be null"); - Preconditions.checkArgument( - StringUtils.isNotBlank(schema.auditInfo().creator()), - "schema 'audit.creator' must not be null and empty"); - Preconditions.checkArgument( - schema.auditInfo().createTime() != null, "schema 'audit.createTime' must not be null"); } } diff --git a/common/src/main/java/com/datastrato/graviton/dto/responses/TableResponse.java b/common/src/main/java/com/datastrato/graviton/dto/responses/TableResponse.java index 0a4e5b0157..2b29e5aee6 100644 --- a/common/src/main/java/com/datastrato/graviton/dto/responses/TableResponse.java +++ b/common/src/main/java/com/datastrato/graviton/dto/responses/TableResponse.java @@ -42,10 +42,5 @@ public void validate() throws IllegalArgumentException { table.columns() != null && table.columns().length > 0, "table 'columns' must not be null and empty"); Preconditions.checkArgument(table.auditInfo() != null, "table 'audit' must not be null"); - Preconditions.checkArgument( - StringUtils.isNotBlank(table.auditInfo().creator()), - "table 'audit.creator' must not be null and empty"); - Preconditions.checkArgument( - table.auditInfo().createTime() != null, "table 'audit.createTime' must not be null"); } } diff --git a/core/src/main/java/com/datastrato/graviton/meta/AuditInfo.java b/core/src/main/java/com/datastrato/graviton/meta/AuditInfo.java index 69c8267060..caae45eb65 100644 --- a/core/src/main/java/com/datastrato/graviton/meta/AuditInfo.java +++ b/core/src/main/java/com/datastrato/graviton/meta/AuditInfo.java @@ -8,7 +8,6 @@ import com.datastrato.graviton.Audit; import com.datastrato.graviton.Entity; import com.datastrato.graviton.Field; -import com.google.common.base.Preconditions; import java.time.Instant; import java.util.Collections; import java.util.HashMap; @@ -23,9 +22,9 @@ public final class AuditInfo implements Audit, Entity { public static final Field CREATOR = - Field.required("creator", String.class, "The name of the user who created the entity"); + Field.optional("creator", String.class, "The name of the user who created the entity"); public static final Field CREATE_TIME = - Field.required("create_time", Instant.class, "The time when the entity was created"); + Field.optional("create_time", Instant.class, "The time when the entity was created"); public static final Field LAST_MODIFIER = Field.optional( "last_modifier", String.class, "The name of the user who last modified the entity"); @@ -33,9 +32,9 @@ public final class AuditInfo implements Audit, Entity { Field.optional( "last_modified_time", Instant.class, "The time when the entity was last modified"); - private String creator; + @Nullable private String creator; - private Instant createTime; + @Nullable private Instant createTime; @Nullable private String lastModifier; @@ -53,10 +52,8 @@ public void validate() throws IllegalArgumentException { CREATOR.validate(creator); CREATE_TIME.validate(createTime); - Preconditions.checkArgument( - lastModifier == null && lastModifiedTime == null - || lastModifier != null && lastModifiedTime != null, - "last_modifier and last_modified_time must be both set or both left unset"); + LAST_MODIFIER.validate(lastModifier); + LAST_MODIFIED_TIME.validate(lastModifiedTime); } /** @@ -125,6 +122,30 @@ public EntityType type() { return EntityType.AUDIT; } + /** + * Merges the audit information with another audit information. If the {@code overwrite} flag is + * set to {@code true} or the field is null, the values from the other audit information will + * overwrite the values of this audit information, otherwise the values of this audit information + * will be preserved. + * + * @param other the other audit information. + * @param overwrite the overwrite flag. + * @return the merged audit information. + */ + public AuditInfo merge(AuditInfo other, boolean overwrite) { + if (other == null) { + return this; + } + + this.creator = overwrite || this.creator == null ? other.creator : creator; + this.createTime = overwrite || this.createTime == null ? other.createTime : createTime; + this.lastModifier = overwrite || this.lastModifier == null ? other.lastModifier : lastModifier; + this.lastModifiedTime = + overwrite || this.lastModifiedTime == null ? other.lastModifiedTime : lastModifiedTime; + + return this; + } + /** Builder class for creating instances of {@link AuditInfo}. */ public static class Builder { private AuditInfo auditInfo; diff --git a/core/src/main/java/com/datastrato/graviton/proto/AuditInfoSerDe.java b/core/src/main/java/com/datastrato/graviton/proto/AuditInfoSerDe.java index 2dd2584369..a88df4d1e8 100644 --- a/core/src/main/java/com/datastrato/graviton/proto/AuditInfoSerDe.java +++ b/core/src/main/java/com/datastrato/graviton/proto/AuditInfoSerDe.java @@ -5,6 +5,8 @@ package com.datastrato.graviton.proto; +import java.util.Optional; + /** A class for serializing and deserializing AuditInfo objects. */ class AuditInfoSerDe implements ProtoSerDe { @@ -17,16 +19,16 @@ class AuditInfoSerDe implements ProtoSerDe new AuditInfo.Builder().withCreator(creator).build()); - Assertions.assertEquals("Field create_time is required", exception.getMessage()); - - Throwable exception1 = - Assertions.assertThrows( - IllegalArgumentException.class, - () -> { - new AuditInfo.Builder() - .withCreator(creator) - .withCreateTime(now) - .withLastModifier(lastModifier) - .build(); - }); - Assertions.assertEquals( - "last_modifier and last_modified_time must be both set or both left unset", - exception1.getMessage()); + AuditInfo auditInfo1 = + new AuditInfo.Builder().withLastModifier(lastModifier).withLastModifiedTime(now).build(); + auditInfo1.validate(); + Assertions.assertNull(auditInfo1.creator()); + Assertions.assertNull(auditInfo1.createTime()); + + AuditInfo auditInfo2 = new AuditInfo.Builder().build(); + auditInfo2.validate(); + Assertions.assertNull(auditInfo2.creator()); + Assertions.assertNull(auditInfo2.createTime()); + Assertions.assertNull(auditInfo2.lastModifier()); + Assertions.assertNull(auditInfo2.lastModifiedTime()); } } diff --git a/core/src/test/java/com/datastrato/graviton/proto/TestEntityProtoSerDe.java b/core/src/test/java/com/datastrato/graviton/proto/TestEntityProtoSerDe.java index 437e1c9d3e..70bf89cee2 100644 --- a/core/src/test/java/com/datastrato/graviton/proto/TestEntityProtoSerDe.java +++ b/core/src/test/java/com/datastrato/graviton/proto/TestEntityProtoSerDe.java @@ -51,6 +51,15 @@ public void testAuditInfoSerDe() throws IOException { auditInfoFromBytes = protoEntitySerDe.deserialize(bytes, com.datastrato.graviton.meta.AuditInfo.class); Assertions.assertEquals(auditInfo1, auditInfoFromBytes); + + // Test with empty field + com.datastrato.graviton.meta.AuditInfo auditInfo2 = + new com.datastrato.graviton.meta.AuditInfo.Builder().build(); + + byte[] bytes1 = protoEntitySerDe.serialize(auditInfo2); + com.datastrato.graviton.meta.AuditInfo auditInfoFromBytes1 = + protoEntitySerDe.deserialize(bytes1, com.datastrato.graviton.meta.AuditInfo.class); + Assertions.assertEquals(auditInfo2, auditInfoFromBytes1); } @Test diff --git a/meta/src/main/proto/graviton_meta.proto b/meta/src/main/proto/graviton_meta.proto index f46282b360..0335fce099 100644 --- a/meta/src/main/proto/graviton_meta.proto +++ b/meta/src/main/proto/graviton_meta.proto @@ -22,8 +22,8 @@ message SchemaVersion { * The AuditInfo message is used to record the audit information of a resource. */ message AuditInfo { - string creator = 1; - google.protobuf.Timestamp create_time = 2; + optional string creator = 1; + optional google.protobuf.Timestamp create_time = 2; optional string last_modifier = 3; optional google.protobuf.Timestamp last_modified_time = 4; }