From e7843e23f85af18c3c0341b283bc503d964327b3 Mon Sep 17 00:00:00 2001 From: Qi Yu Date: Sun, 8 Oct 2023 23:16:52 +0800 Subject: [PATCH] [#415] feat(core, server, catalogs): Add property validation logic for catalog (#452) ### What changes were proposed in this pull request? 1. Add property validation logic for catalog 2. Move property validation code for tables to `PropertiesMetadata` 3. Fix some typo problem. ### Why are the changes needed? Currently, we have not verified the accuracy of the properties when creating/altering catalogs, which is very risky and tends to be buggy. Fix: #415 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? Add `TestPropertyMeta` and `TestPropertyValidator` --- build.gradle.kts | 1 + .../catalog/hive/HiveCatalogOperations.java | 7 + .../hive/HiveTablePropertiesMetadata.java | 6 +- .../iceberg/IcebergCatalogOperations.java | 6 + .../IcebergTablePropertiesMetadata.java | 6 +- .../java/com/datastrato/graviton/Entity.java | 2 +- .../com/datastrato/graviton/EntityStore.java | 4 +- .../graviton/catalog/BaseCatalog.java | 22 +- ...adata.java => BasePropertiesMetadata.java} | 23 +-- .../graviton/catalog/CatalogManager.java | 91 ++++++++- .../catalog/CatalogOperationDispatcher.java | 84 +++----- .../graviton/catalog/HasPropertyMetadata.java | 17 +- .../graviton/catalog/PropertiesMetadata.java | 73 +++++++ .../graviton/storage/kv/KvEntityStore.java | 18 +- ...a.java => TestBasePropertiesMetadata.java} | 12 +- .../com/datastrato/graviton/TestCatalog.java | 2 +- .../graviton/TestCatalogOperations.java | 62 +++++- .../datastrato/graviton/TestEntityStore.java | 4 + .../graviton/catalog/TestCatalogManager.java | 189 +++++++++++++++++- .../TestCatalogOperationDispatcher.java | 4 +- .../graviton/server/web/rest/TestCatalog.java | 5 + 21 files changed, 514 insertions(+), 124 deletions(-) rename core/src/main/java/com/datastrato/graviton/catalog/{TablePropertiesMetadata.java => BasePropertiesMetadata.java} (60%) rename core/src/test/java/com/datastrato/graviton/{TestTablePropertiesMetadata.java => TestBasePropertiesMetadata.java} (71%) diff --git a/build.gradle.kts b/build.gradle.kts index 043dc3b712c..aff3c15af0f 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -127,6 +127,7 @@ tasks.rat { // Ignore files we track but do not distribute "**/.github/**/*", "dev/docker/**/*.xml", + "**/*.log", ) // Add .gitignore excludes to the Apache Rat exclusion list. diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index 9023f2a8558..3c0c3724ca3 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -32,6 +32,7 @@ import com.datastrato.graviton.rel.transforms.Transforms; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.Maps; import java.io.IOException; import java.time.Instant; import java.util.Arrays; @@ -742,4 +743,10 @@ private static String currentUser() { public PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationException { return tablePropertiesMetadata; } + + @Override + public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperationException { + // TODO(yuqi): We will implement this in next PR + return Maps::newHashMap; + } } diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTablePropertiesMetadata.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTablePropertiesMetadata.java index 175e7cc17d1..9bba53bc3fe 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTablePropertiesMetadata.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTablePropertiesMetadata.java @@ -4,14 +4,14 @@ */ package com.datastrato.graviton.catalog.hive; +import com.datastrato.graviton.catalog.BasePropertiesMetadata; import com.datastrato.graviton.catalog.PropertyEntry; -import com.datastrato.graviton.catalog.TablePropertiesMetadata; import com.google.common.collect.Maps; import java.util.Map; -public class HiveTablePropertiesMetadata extends TablePropertiesMetadata { +public class HiveTablePropertiesMetadata extends BasePropertiesMetadata { @Override - protected Map> tablePropertyEntries() { + protected Map> specificPropertyEntries() { // TODO(Minghuang): support Hive table property specs return Maps.newHashMap(); } diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java index 30d39750e73..61a4876ac0a 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java +++ b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java @@ -29,6 +29,7 @@ import com.datastrato.graviton.rel.transforms.Transform; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.Maps; import java.time.Instant; import java.util.ArrayList; import java.util.HashMap; @@ -384,4 +385,9 @@ private static String currentUser() { public PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationException { return tablePropertiesMetadata; } + + @Override + public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperationException { + return Maps::newHashMap; + } } diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergTablePropertiesMetadata.java b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergTablePropertiesMetadata.java index b86eaee6ec4..3c107f75e18 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergTablePropertiesMetadata.java +++ b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergTablePropertiesMetadata.java @@ -4,14 +4,14 @@ */ package com.datastrato.graviton.catalog.lakehouse.iceberg; +import com.datastrato.graviton.catalog.BasePropertiesMetadata; import com.datastrato.graviton.catalog.PropertyEntry; -import com.datastrato.graviton.catalog.TablePropertiesMetadata; import com.google.common.collect.Maps; import java.util.Map; -public class IcebergTablePropertiesMetadata extends TablePropertiesMetadata { +public class IcebergTablePropertiesMetadata extends BasePropertiesMetadata { @Override - protected Map> tablePropertyEntries() { + protected Map> specificPropertyEntries() { // TODO: support Iceberg table property specs return Maps.newHashMap(); } diff --git a/core/src/main/java/com/datastrato/graviton/Entity.java b/core/src/main/java/com/datastrato/graviton/Entity.java index 74b5f560e1c..cad4150d663 100644 --- a/core/src/main/java/com/datastrato/graviton/Entity.java +++ b/core/src/main/java/com/datastrato/graviton/Entity.java @@ -22,7 +22,7 @@ enum EntityType { AUDIT("au", 65534); - // Short name can be used to identify the entity type in the logs, peristent storage, etc. + // Short name can be used to identify the entity type in the logs, persistent storage, etc. private final String shortName; private final int index; diff --git a/core/src/main/java/com/datastrato/graviton/EntityStore.java b/core/src/main/java/com/datastrato/graviton/EntityStore.java index 7b8a99151d9..71c44a72d3c 100644 --- a/core/src/main/java/com/datastrato/graviton/EntityStore.java +++ b/core/src/main/java/com/datastrato/graviton/EntityStore.java @@ -106,7 +106,7 @@ void put(E e, boolean overwritten) * @return E the updated entity * @throws IOException if the store operation fails * @throws NoSuchEntityException if the entity does not exist - * @throws AlreadyExistsException if the updated entity already exitsed. + * @throws AlreadyExistsException if the updated entity already existed. */ E update( NameIdentifier ident, Class type, EntityType entityType, Function updater) @@ -143,7 +143,7 @@ default boolean delete(NameIdentifier ident, EntityType entityType) throws IOExc * * @param ident the name identifier of the entity * @param entityType the type of the entity to be deleted - * @param cascade support cacade detele or not + * @param cascade support cascade delete or not * @return true if the entity exists and is deleted successfully, false otherwise * @throws IOException if the delete operation fails */ diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 024972f1d65..a25a349052f 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -9,6 +9,7 @@ import com.datastrato.graviton.CatalogProvider; import com.datastrato.graviton.meta.CatalogEntity; import com.google.common.base.Preconditions; +import com.google.common.collect.Maps; import java.util.Map; /** @@ -32,6 +33,7 @@ public abstract class BaseCatalog private volatile CatalogOperations ops; + private volatile Map properties; /** * Creates a new instance of CatalogOperations. * @@ -45,6 +47,11 @@ public PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationE return ops().tablePropertiesMetadata(); } + @Override + public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperationException { + return ops().catalogPropertiesMetadata(); + } + /** * Retrieves the CatalogOperations instance associated with this catalog. Lazily initializes the * instance if not already created. @@ -123,8 +130,19 @@ public String comment() { @Override public Map properties() { - Preconditions.checkArgument(entity != null, "entity is not set"); - return entity.getProperties(); + if (properties == null) { + synchronized (this) { + if (properties == null) { + Preconditions.checkArgument(entity != null, "entity is not set"); + properties = Maps.newHashMap(entity.getProperties()); + properties + .entrySet() + .removeIf( + entry -> ops().catalogPropertiesMetadata().isHiddenProperty(entry.getKey())); + } + } + } + return properties; } @Override diff --git a/core/src/main/java/com/datastrato/graviton/catalog/TablePropertiesMetadata.java b/core/src/main/java/com/datastrato/graviton/catalog/BasePropertiesMetadata.java similarity index 60% rename from core/src/main/java/com/datastrato/graviton/catalog/TablePropertiesMetadata.java rename to core/src/main/java/com/datastrato/graviton/catalog/BasePropertiesMetadata.java index a86af23f596..ea1a854a619 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/TablePropertiesMetadata.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BasePropertiesMetadata.java @@ -13,22 +13,22 @@ import java.util.List; import java.util.Map; -public abstract class TablePropertiesMetadata implements PropertiesMetadata { +public abstract class BasePropertiesMetadata implements PropertiesMetadata { - private static final Map> BASIC_TABLE_PROPERTY_ENTRIES; + private static final Map> BASIC_PROPERTY_ENTRIES; - private Map> propertyEntries; + private volatile Map> propertyEntries; static { - List> basicTablePropertyEntries = + // basicPropertyEntries is shared by all entities + List> basicPropertyEntries = ImmutableList.of( PropertyEntry.stringReservedPropertyEntry( ID_KEY, "To differentiate the entities created directly by the underlying sources", true)); - BASIC_TABLE_PROPERTY_ENTRIES = - Maps.uniqueIndex(basicTablePropertyEntries, PropertyEntry::getName); + BASIC_PROPERTY_ENTRIES = Maps.uniqueIndex(basicPropertyEntries, PropertyEntry::getName); } @Override @@ -37,14 +37,13 @@ public Map> propertyEntries() { synchronized (this) { if (propertyEntries == null) { ImmutableMap.Builder> builder = ImmutableMap.builder(); - Map> catalogTableProperty = tablePropertyEntries(); - builder.putAll(catalogTableProperty); + Map> properties = specificPropertyEntries(); + builder.putAll(properties); - BASIC_TABLE_PROPERTY_ENTRIES.forEach( + BASIC_PROPERTY_ENTRIES.forEach( (name, entry) -> { Preconditions.checkArgument( - !catalogTableProperty.containsKey(name), - "Property metadata already exists: " + name); + !properties.containsKey(name), "Property metadata already exists: " + name); builder.put(name, entry); }); @@ -55,5 +54,5 @@ public Map> propertyEntries() { return propertyEntries; } - protected abstract Map> tablePropertyEntries(); + protected abstract Map> specificPropertyEntries(); } diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 672798bed25..d414c8c732f 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -6,6 +6,8 @@ import com.datastrato.graviton.Catalog; import com.datastrato.graviton.CatalogChange; +import com.datastrato.graviton.CatalogChange.RemoveProperty; +import com.datastrato.graviton.CatalogChange.SetProperty; import com.datastrato.graviton.CatalogProvider; import com.datastrato.graviton.Config; import com.datastrato.graviton.Configs; @@ -40,6 +42,7 @@ import java.io.File; import java.io.IOException; import java.time.Instant; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -49,6 +52,7 @@ import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; +import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -210,7 +214,8 @@ public NameIdentifier[] listCatalogs(Namespace namespace) throws NoSuchMetalakeE */ @Override public Catalog loadCatalog(NameIdentifier ident) throws NoSuchCatalogException { - return loadCatalogAndWrap(ident).catalog; + CatalogWrapper wrapper = loadCatalogInternal(ident); + return wrapper.catalog; } /** @@ -266,18 +271,44 @@ public Catalog createCatalog( store.put(e, false /* overwrite */); return e; }); - return catalogCache.get(ident, id -> createCatalogWrapper(entity)).catalog; - - } catch (EntityAlreadyExistsException ee) { - LOG.warn("Catalog {} already exists", ident, ee); + CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(entity)); + wrapper.doWithPropertiesMeta( + f -> { + f.catalogPropertiesMetadata().validatePropertyForCreate(properties); + return null; + }); + return wrapper.catalog; + } catch (EntityAlreadyExistsException e1) { + LOG.warn("Catalog {} already exists", ident, e1); throw new CatalogAlreadyExistsException("Catalog " + ident + " already exists"); - - } catch (IOException ioe) { - LOG.error("Failed to create catalog {}", ident, ioe); - throw new RuntimeException(ioe); + } catch (IllegalArgumentException | NoSuchMetalakeException e2) { + throw e2; + } catch (Exception e3) { + LOG.error("Failed to create catalog {}", ident, e3); + throw new RuntimeException(e3); } } + private Pair, Map> getCatalogAlterProperty( + CatalogChange... catalogChanges) { + Map upserts = Maps.newHashMap(); + Map deletes = Maps.newHashMap(); + + Arrays.stream(catalogChanges) + .forEach( + tableChange -> { + if (tableChange instanceof SetProperty) { + SetProperty setProperty = (SetProperty) tableChange; + upserts.put(setProperty.getProperty(), setProperty.getValue()); + } else if (tableChange instanceof RemoveProperty) { + RemoveProperty removeProperty = (RemoveProperty) tableChange; + deletes.put(removeProperty.getProperty(), removeProperty.getProperty()); + } + }); + + return Pair.of(upserts, deletes); + } + /** * Alters an existing catalog with the specified changes. * @@ -292,8 +323,29 @@ public Catalog alterCatalog(NameIdentifier ident, CatalogChange... changes) throws NoSuchCatalogException, IllegalArgumentException { // There could be a race issue that someone is using the catalog from cache while we are // updating it. - catalogCache.invalidate(ident); + + CatalogWrapper catalogWrapper = loadCatalogAndWrap(ident); + if (catalogWrapper == null) { + throw new NoSuchCatalogException("Catalog " + ident + " does not exist"); + } + try { + catalogWrapper.doWithPropertiesMeta( + f -> { + Pair, Map> alterProperty = + getCatalogAlterProperty(changes); + f.catalogPropertiesMetadata() + .validatePropertyForAlter(alterProperty.getLeft(), alterProperty.getRight()); + return null; + }); + } catch (IllegalArgumentException e1) { + throw e1; + } catch (Exception e) { + LOG.error("Failed to alter catalog {}", ident, e); + throw new RuntimeException(e); + } + + catalogCache.invalidate(ident); try { CatalogEntity updatedCatalog = store.update( @@ -434,7 +486,24 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { // Initialize the catalog catalog = catalog.withCatalogEntity(entity).withCatalogConf(mergedConf); - return new CatalogWrapper(catalog, classLoader); + CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); + + // Call wrapper.catalog.properties() to make BaseCatalog#properties in IsolatedClassLoader + // not null. Why we do this? Because wrapper.catalog.properties() need to be called in the + // IsolatedClassLoader, it needs to load the specific catalog class such as HiveCatalog or so. + // For simply, We will preload the value of properties and thus AppClassLoader can get the + // value of properties. + try { + wrapper.doWithPropertiesMeta( + f -> { + wrapper.catalog.properties(); + return null; + }); + } catch (Exception e) { + LOG.error("Failed to load catalog '{}' properties", entity.name(), e); + throw new RuntimeException(e); + } + return wrapper; } static Map mergeConf(Map properties, Map conf) { diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java index ec21cb7c5a3..dcd80d790f5 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java @@ -34,11 +34,13 @@ import com.datastrato.graviton.rel.Table; import com.datastrato.graviton.rel.TableCatalog; import com.datastrato.graviton.rel.TableChange; +import com.datastrato.graviton.rel.TableChange.RemoveProperty; +import com.datastrato.graviton.rel.TableChange.SetProperty; import com.datastrato.graviton.rel.transforms.Transform; import com.datastrato.graviton.storage.IdGenerator; import com.datastrato.graviton.utils.ThrowableFunction; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; +import com.google.common.collect.Maps; import java.time.Instant; import java.util.Arrays; import java.util.List; @@ -46,6 +48,7 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -348,7 +351,7 @@ public Table createTable( c -> c.doWithPropertiesMeta( p -> { - validateCreateTableProperties(p.tablePropertiesMetadata(), properties); + p.tablePropertiesMetadata().validatePropertyForCreate(properties); return null; }), IllegalArgumentException.class); @@ -402,37 +405,6 @@ public Table createTable( getHiddenPropertyNames(getCatalogIdentifier(ident), table.properties())); } - private void validateCreateTableProperties( - PropertiesMetadata tablePropertiesMetadata, Map properties) { - if (properties == null) { - return; - } - - List reservedProperties = - properties.keySet().stream() - .filter(tablePropertiesMetadata::isReservedProperty) - .collect(Collectors.toList()); - Preconditions.checkArgument( - reservedProperties.isEmpty(), - "Properties are reserved and cannot be set: %s", - reservedProperties); - - List absentProperties = - tablePropertiesMetadata.propertyEntries().keySet().stream() - .filter(tablePropertiesMetadata::isRequiredProperty) - .filter(k -> !properties.containsKey(k)) - .collect(Collectors.toList()); - Preconditions.checkArgument( - absentProperties.isEmpty(), - "Properties are required and must be set: %s", - absentProperties); - - // use decode function to validate the property values - properties.keySet().stream() - .filter(tablePropertiesMetadata::containsProperty) - .forEach(k -> tablePropertiesMetadata.propertyEntries().get(k).decode(properties.get(k))); - } - /** * Alters an existing table. * @@ -502,34 +474,36 @@ public Table alterTable(NameIdentifier ident, TableChange... changes) getHiddenPropertyNames(getCatalogIdentifier(ident), alteredTable.properties())); } + private Pair, Map> getTableAlterProperty( + TableChange... tableChanges) { + Map upserts = Maps.newHashMap(); + Map deletes = Maps.newHashMap(); + + Arrays.stream(tableChanges) + .forEach( + tableChange -> { + if (tableChange instanceof SetProperty) { + SetProperty setProperty = (SetProperty) tableChange; + upserts.put(setProperty.getProperty(), setProperty.getValue()); + } else if (tableChange instanceof RemoveProperty) { + RemoveProperty removeProperty = (RemoveProperty) tableChange; + deletes.put(removeProperty.getProperty(), removeProperty.getProperty()); + } + }); + + return Pair.of(upserts, deletes); + } + private void validateAlterTableProperties(NameIdentifier ident, TableChange... changes) { doWithCatalog( getCatalogIdentifier(ident), c -> c.doWithPropertiesMeta( p -> { - PropertiesMetadata tablePropertiesMetadata = p.tablePropertiesMetadata(); - for (TableChange change : changes) { - if (change instanceof TableChange.SetProperty) { - String propertyName = ((TableChange.SetProperty) change).getProperty(); - if (tablePropertiesMetadata.isReservedProperty(propertyName) - || tablePropertiesMetadata.isImmutableProperty(propertyName)) { - throw new IllegalArgumentException( - String.format( - "Property %s is reserved or immutable and cannot be set", - propertyName)); - } - } - - if (change instanceof TableChange.RemoveProperty) { - String propertyName = ((TableChange.RemoveProperty) change).getProperty(); - if (tablePropertiesMetadata.isReservedProperty(propertyName) - || tablePropertiesMetadata.isImmutableProperty(propertyName)) { - throw new IllegalArgumentException( - String.format("Property %s cannot be removed by user", propertyName)); - } - } - } + Pair, Map> alterProperty = + getTableAlterProperty(changes); + p.tablePropertiesMetadata() + .validatePropertyForAlter(alterProperty.getLeft(), alterProperty.getRight()); return null; }), IllegalArgumentException.class); diff --git a/core/src/main/java/com/datastrato/graviton/catalog/HasPropertyMetadata.java b/core/src/main/java/com/datastrato/graviton/catalog/HasPropertyMetadata.java index ce9dbd41fd9..ec691f400ed 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/HasPropertyMetadata.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/HasPropertyMetadata.java @@ -4,7 +4,22 @@ */ package com.datastrato.graviton.catalog; -/** This interface represents entities that have properties metadata. */ +/** This interface represents entities that have property metadata. */ public interface HasPropertyMetadata { + + /** + * Returns the table property metadata. + * + * @return The table property metadata. + * @throws UnsupportedOperationException if the entity does not support table properties. + */ PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationException; + + /** + * Returns the catalog property metadata. + * + * @return The catalog property metadata. + * @throws UnsupportedOperationException if the entity does not support catalog properties. + */ + PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperationException; } diff --git a/core/src/main/java/com/datastrato/graviton/catalog/PropertiesMetadata.java b/core/src/main/java/com/datastrato/graviton/catalog/PropertiesMetadata.java index e8ccfdddba1..be9ae9261d5 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/PropertiesMetadata.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/PropertiesMetadata.java @@ -4,7 +4,12 @@ */ package com.datastrato.graviton.catalog; +import com.google.common.base.Preconditions; +import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.function.Function; +import java.util.stream.Collectors; /** The PropertiesMetadata class is responsible for managing property metadata. */ public interface PropertiesMetadata { @@ -33,4 +38,72 @@ default boolean isHiddenProperty(String propertyName) { default boolean containsProperty(String propertyName) { return propertyEntries().containsKey(propertyName); } + + static void checkValueFormat(String key, String value, Function decoder) { + try { + decoder.apply(value); + } catch (Exception e) { + throw new IllegalArgumentException( + String.format("Invalid value: '%s' for property: '%s'", value, key), e); + } + } + + default void validatePropertyForAlter(Map upserts, Map deletes) { + for (Map.Entry entry : upserts.entrySet()) { + PropertyEntry propertyEntry = propertyEntries().get(entry.getKey()); + if (Objects.nonNull(propertyEntry)) { + Preconditions.checkArgument( + !propertyEntry.isImmutable() && !propertyEntry.isReserved(), + "Property " + propertyEntry.getName() + " is immutable or reserved, cannot be set"); + checkValueFormat(entry.getKey(), entry.getValue(), propertyEntry::decode); + } + } + + for (Map.Entry entry : deletes.entrySet()) { + PropertyEntry propertyEntry = propertyEntries().get(entry.getKey()); + if (Objects.nonNull(propertyEntry)) { + Preconditions.checkArgument( + !propertyEntry.isImmutable() && !propertyEntry.isReserved(), + "Property " + propertyEntry.getName() + " is immutable or reserved, cannot be deleted"); + } + } + } + + default void validatePropertyForCreate(Map properties) + throws IllegalArgumentException { + if (properties == null) { + return; + } + + List reservedProperties = + properties.keySet().stream().filter(this::isReservedProperty).collect(Collectors.toList()); + Preconditions.checkArgument( + reservedProperties.isEmpty(), + "Properties are reserved and cannot be set: %s", + reservedProperties); + + List absentProperties = + propertyEntries().keySet().stream() + .filter(this::isRequiredProperty) + .filter(k -> !properties.containsKey(k)) + .collect(Collectors.toList()); + Preconditions.checkArgument( + absentProperties.isEmpty(), + "Properties are required and must be set: %s", + absentProperties); + + // use decode function to validate the property values + for (Map.Entry entry : properties.entrySet()) { + String key = entry.getKey(); + String value = entry.getValue(); + if (containsProperty(key)) { + try { + propertyEntries().get(key).decode(value); + } catch (Exception e) { + throw new IllegalArgumentException( + String.format("Invalid value: '%s' for property: '%s'", value, key)); + } + } + } + } } diff --git a/core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java b/core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java index 18fddaadda3..63925171c6c 100644 --- a/core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java +++ b/core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java @@ -254,28 +254,28 @@ public E get( */ private List getSubEntitiesPrefix(NameIdentifier ident, EntityType type) throws IOException { - List prefixs = Lists.newArrayList(); + List prefixes = Lists.newArrayList(); byte[] encode = entityKeyEncoder.encode(ident, type, true); switch (type) { case METALAKE: - prefixs.add(replacePrefixTypeInfo(encode, CATALOG.getShortName())); - prefixs.add(replacePrefixTypeInfo(encode, SCHEMA.getShortName())); - prefixs.add(replacePrefixTypeInfo(encode, TABLE.getShortName())); + prefixes.add(replacePrefixTypeInfo(encode, CATALOG.getShortName())); + prefixes.add(replacePrefixTypeInfo(encode, SCHEMA.getShortName())); + prefixes.add(replacePrefixTypeInfo(encode, TABLE.getShortName())); break; case CATALOG: - prefixs.add(replacePrefixTypeInfo(encode, SCHEMA.getShortName())); - prefixs.add(replacePrefixTypeInfo(encode, TABLE.getShortName())); + prefixes.add(replacePrefixTypeInfo(encode, SCHEMA.getShortName())); + prefixes.add(replacePrefixTypeInfo(encode, TABLE.getShortName())); break; case SCHEMA: - prefixs.add(replacePrefixTypeInfo(encode, TABLE.getShortName())); + prefixes.add(replacePrefixTypeInfo(encode, TABLE.getShortName())); break; case TABLE: break; default: LOG.warn("Currently unknown type: {}, please check it", type); } - Collections.reverse(prefixs); - return prefixs; + Collections.reverse(prefixes); + return prefixes; } private byte[] replacePrefixTypeInfo(byte[] encode, String subTypePrefix) { diff --git a/core/src/test/java/com/datastrato/graviton/TestTablePropertiesMetadata.java b/core/src/test/java/com/datastrato/graviton/TestBasePropertiesMetadata.java similarity index 71% rename from core/src/test/java/com/datastrato/graviton/TestTablePropertiesMetadata.java rename to core/src/test/java/com/datastrato/graviton/TestBasePropertiesMetadata.java index 4be0f42f1a9..6f2236f291b 100644 --- a/core/src/test/java/com/datastrato/graviton/TestTablePropertiesMetadata.java +++ b/core/src/test/java/com/datastrato/graviton/TestBasePropertiesMetadata.java @@ -4,14 +4,14 @@ */ package com.datastrato.graviton; +import com.datastrato.graviton.catalog.BasePropertiesMetadata; import com.datastrato.graviton.catalog.PropertyEntry; -import com.datastrato.graviton.catalog.TablePropertiesMetadata; import com.google.common.collect.ImmutableList; import com.google.common.collect.Maps; import java.util.List; import java.util.Map; -public class TestTablePropertiesMetadata extends TablePropertiesMetadata { +public class TestBasePropertiesMetadata extends BasePropertiesMetadata { public static final String COMMENT_KEY = "comment"; @@ -19,7 +19,7 @@ public class TestTablePropertiesMetadata extends TablePropertiesMetadata { public static final String TEST_IMMUTABLE_KEY = "immutableKey"; - private static final Map> TEST_TABLE_PROPERTY; + private static final Map> TEST_BASE_PROPERTY; static { List> tablePropertyMetadata = @@ -30,11 +30,11 @@ public class TestTablePropertiesMetadata extends TablePropertiesMetadata { PropertyEntry.stringImmutablePropertyEntry( TEST_IMMUTABLE_KEY, "test immutable property", false, null, false, false)); - TEST_TABLE_PROPERTY = Maps.uniqueIndex(tablePropertyMetadata, PropertyEntry::getName); + TEST_BASE_PROPERTY = Maps.uniqueIndex(tablePropertyMetadata, PropertyEntry::getName); } @Override - protected Map> tablePropertyEntries() { - return TEST_TABLE_PROPERTY; + protected Map> specificPropertyEntries() { + return TEST_BASE_PROPERTY; } } diff --git a/core/src/test/java/com/datastrato/graviton/TestCatalog.java b/core/src/test/java/com/datastrato/graviton/TestCatalog.java index d84172d04b5..f25ee6c6068 100644 --- a/core/src/test/java/com/datastrato/graviton/TestCatalog.java +++ b/core/src/test/java/com/datastrato/graviton/TestCatalog.java @@ -20,7 +20,7 @@ public String shortName() { @Override protected CatalogOperations newOps(Map config) { - return new TestCatalogOperations(); + return new TestCatalogOperations(config); } @Override diff --git a/core/src/test/java/com/datastrato/graviton/TestCatalogOperations.java b/core/src/test/java/com/datastrato/graviton/TestCatalogOperations.java index 674b508efc8..a04fa59efa3 100644 --- a/core/src/test/java/com/datastrato/graviton/TestCatalogOperations.java +++ b/core/src/test/java/com/datastrato/graviton/TestCatalogOperations.java @@ -4,9 +4,10 @@ */ package com.datastrato.graviton; +import com.datastrato.graviton.catalog.BasePropertiesMetadata; import com.datastrato.graviton.catalog.CatalogOperations; import com.datastrato.graviton.catalog.PropertiesMetadata; -import com.datastrato.graviton.catalog.TablePropertiesMetadata; +import com.datastrato.graviton.catalog.PropertyEntry; import com.datastrato.graviton.exceptions.NoSuchCatalogException; import com.datastrato.graviton.exceptions.NoSuchSchemaException; import com.datastrato.graviton.exceptions.NoSuchTableException; @@ -24,6 +25,7 @@ import com.datastrato.graviton.rel.TableCatalog; import com.datastrato.graviton.rel.TableChange; import com.datastrato.graviton.rel.transforms.Transform; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import java.io.IOException; import java.time.Instant; @@ -36,12 +38,14 @@ public class TestCatalogOperations implements CatalogOperations, TableCatalog, S private final Map schemas; - private final TablePropertiesMetadata tablePropertiesMetadata; + private final BasePropertiesMetadata tablePropertiesMetadata; + private Map config; - public TestCatalogOperations() { + public TestCatalogOperations(Map config) { tables = Maps.newHashMap(); schemas = Maps.newHashMap(); - tablePropertiesMetadata = new TestTablePropertiesMetadata(); + tablePropertiesMetadata = new TestBasePropertiesMetadata(); + this.config = config; } @Override @@ -263,7 +267,7 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty if (cascade) { tables.keySet().stream() .filter(table -> table.namespace().toString().equals(ident.toString())) - .forEach(table -> tables.remove(table)); + .forEach(tables::remove); } return true; @@ -273,4 +277,52 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty public PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationException { return tablePropertiesMetadata; } + + @Override + public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperationException { + if (config.containsKey("mock")) { + return new BasePropertiesMetadata() { + @Override + protected Map> specificPropertyEntries() { + return ImmutableMap.>builder() + .put( + "key1", + PropertyEntry.stringPropertyEntry( + "key1", "value1", true, true, null, false, false)) + .put( + "key2", + PropertyEntry.stringPropertyEntry( + "key2", "value2", true, false, null, false, false)) + .put( + "key3", + new PropertyEntry.Builder() + .withDecoder(Integer::parseInt) + .withEncoder(Object::toString) + .withDefaultValue(1) + .withDescription("key3") + .withHidden(false) + .withReserved(false) + .withImmutable(true) + .withJavaType(Integer.class) + .withRequired(false) + .withName("key3") + .build()) + .put( + "key4", + PropertyEntry.stringPropertyEntry( + "key4", "value4", false, false, "value4", false, false)) + .put( + "reserved_key", + PropertyEntry.stringPropertyEntry( + "reserved_key", "reserved_key", false, true, "reserved_value", false, true)) + .put( + "hidden_key", + PropertyEntry.stringPropertyEntry( + "hidden_key", "hidden_key", false, false, "hidden_value", true, false)) + .build(); + } + }; + } + return Maps::newHashMap; + } } diff --git a/core/src/test/java/com/datastrato/graviton/TestEntityStore.java b/core/src/test/java/com/datastrato/graviton/TestEntityStore.java index 867b1346ed2..d364932acfa 100644 --- a/core/src/test/java/com/datastrato/graviton/TestEntityStore.java +++ b/core/src/test/java/com/datastrato/graviton/TestEntityStore.java @@ -40,6 +40,10 @@ public InMemoryEntityStore() { this.lock = new ReentrantLock(); } + public void clear() { + entityMap.clear(); + } + @Override public void initialize(Config config) throws RuntimeException {} diff --git a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java index 46b3e23756e..470b1b2bd9f 100644 --- a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java +++ b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java @@ -4,6 +4,8 @@ */ package com.datastrato.graviton.catalog; +import static com.datastrato.graviton.StringIdentifier.ID_KEY; + import com.datastrato.graviton.Catalog; import com.datastrato.graviton.CatalogChange; import com.datastrato.graviton.Config; @@ -12,6 +14,7 @@ import com.datastrato.graviton.NameIdentifier; import com.datastrato.graviton.StringIdentifier; import com.datastrato.graviton.TestEntityStore; +import com.datastrato.graviton.TestEntityStore.InMemoryEntityStore; import com.datastrato.graviton.exceptions.CatalogAlreadyExistsException; import com.datastrato.graviton.exceptions.NoSuchCatalogException; import com.datastrato.graviton.exceptions.NoSuchMetalakeException; @@ -26,10 +29,17 @@ import java.util.Map; import java.util.Set; import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.MethodOrderer; +import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; +import org.mockito.Mockito; +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class TestCatalogManager { private static CatalogManager catalogManager; @@ -42,6 +52,15 @@ public class TestCatalogManager { private static String provider = "test"; + private static BaseMetalake metalakeEntity = + new BaseMetalake.Builder() + .withId(1L) + .withName(metalake) + .withAuditInfo( + new AuditInfo.Builder().withCreator("test").withCreateTime(Instant.now()).build()) + .withVersion(SchemaVersion.V_0_1) + .build(); + @BeforeAll public static void setUp() throws IOException { config = new Config(false) {}; @@ -51,17 +70,17 @@ public static void setUp() throws IOException { entityStore.initialize(config); entityStore.setSerDe(null); - BaseMetalake metalakeEntity = - new BaseMetalake.Builder() - .withId(1L) - .withName(metalake) - .withAuditInfo( - new AuditInfo.Builder().withCreator("test").withCreateTime(Instant.now()).build()) - .withVersion(SchemaVersion.V_0_1) - .build(); entityStore.put(metalakeEntity, true); catalogManager = new CatalogManager(config, entityStore, new RandomIdGenerator()); + catalogManager = Mockito.spy(catalogManager); + } + + @BeforeEach + @AfterEach + void reset() throws IOException { + ((InMemoryEntityStore) entityStore).clear(); + entityStore.put(metalakeEntity, true); } @AfterAll @@ -77,6 +96,154 @@ public static void tearDown() throws Exception { } } + @Test + @Order(1) + void testLoadTable() throws IOException { + NameIdentifier ident = NameIdentifier.of("metalake", "test444"); + // key1 is required; + Map props1 = + ImmutableMap.builder() + .put("key2", "value2") + .put("key1", "value1") + .put("hidden_key", "hidden_value") + .put("mock", "mock") + .build(); + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props1)); + + Map properties = catalogManager.loadCatalog(ident).properties(); + Assertions.assertTrue(properties.containsKey("key2")); + Assertions.assertTrue(properties.containsKey("key1")); + Assertions.assertFalse(properties.containsKey("hidden_key")); + Assertions.assertFalse(properties.containsKey(ID_KEY)); + reset(); + } + + @Test + @Order(2) + void testPropertyValidationInAlter() throws IOException { + // key1 is required and immutable and do not have default value, is not hidden and not reserved + // key2 is required and mutable and do not have default value, is not hidden and not reserved + // key3 is optional and immutable and have default value, is not hidden and not reserved + // key4 is optional and mutable and have default value, is not hidden and not reserved + // reserved_key is optional and immutable and have default value, is not hidden and reserved + // hidden_key is optional and mutable and have default value, is hidden and not reserved + + NameIdentifier ident = NameIdentifier.of("metalake", "test111"); + // key1 is required; + Map props1 = + ImmutableMap.builder() + .put("key2", "value2") + .put("key1", "value1") + .put("mock", "mock") + .build(); + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props1)); + + NameIdentifier ident2 = NameIdentifier.of("metalake", "test222"); + // key1 is required; + Map props2 = + ImmutableMap.builder() + .put("key2", "value2") + .put("key1", "value1") + .put("key3", "3") + .put("key4", "value4") + .put("mock", "mock") + .build(); + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + ident2, Catalog.Type.RELATIONAL, provider, "comment", props2)); + + Exception e1 = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> catalogManager.alterCatalog(ident, CatalogChange.setProperty("key1", "value1"))); + Assertions.assertTrue(e1.getMessage().contains("Property key1 is immutable")); + + Exception e2 = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> catalogManager.alterCatalog(ident2, CatalogChange.setProperty("key3", "value2"))); + Assertions.assertTrue(e2.getMessage().contains("Property key3 is immutable")); + + Assertions.assertDoesNotThrow( + () -> catalogManager.alterCatalog(ident2, CatalogChange.setProperty("key4", "value4"))); + Assertions.assertDoesNotThrow( + () -> catalogManager.alterCatalog(ident2, CatalogChange.setProperty("key2", "value2"))); + + Exception e3 = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.alterCatalog( + ident2, + CatalogChange.setProperty("key4", "value4"), + CatalogChange.removeProperty("key1"))); + Assertions.assertTrue(e3.getMessage().contains("Property key1 is immutable")); + reset(); + } + + @Test + @Order(3) + void testPropertyValidationInCreate() throws IOException { + // key1 is required and immutable and do not have default value, is not hidden and not reserved + // key2 is required and mutable and do not have default value, is not hidden and not reserved + // key3 is optional and immutable and have default value, is not hidden and not reserved + // key4 is optional and mutable and have default value, is not hidden and not reserved + // reserved_key is optional and immutable and have default value, is not hidden and reserved + // hidden_key is optional and mutable and have default value, is hidden and not reserved + NameIdentifier ident = NameIdentifier.of("metalake", "test111111"); + + // key1 is required; + Map props1 = + ImmutableMap.builder().put("key2", "value2").put("mock", "mock").build(); + IllegalArgumentException e1 = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props1)); + Assertions.assertTrue( + e1.getMessage().contains("Properties are required and must be set: [key1]")); + // BUG here, in memory does not support rollback + reset(); + + // key2 is required; + Map props2 = + ImmutableMap.builder().put("key1", "value1").put("mock", "mock").build(); + e1 = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props2)); + Assertions.assertTrue( + e1.getMessage().contains("Properties are required and must be set: [key2]")); + reset(); + + // key3 is optional, but we assign a wrong value format + Map props3 = + ImmutableMap.builder() + .put("key1", "value1") + .put("key2", "value2") + .put("key3", "a12a1a") + .put("mock", "mock") + .build(); + e1 = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props3)); + Assertions.assertTrue(e1.getMessage().contains("Invalid value: 'a12a1a' for property: 'key3'")); + reset(); + } + @Test public void testCreateCatalog() { NameIdentifier ident = NameIdentifier.of("metalake", "test1"); @@ -232,8 +399,8 @@ private void testProperties(Map expectedProps, Map dispatcher.alterTable(tableIdent, illegalChange1)); Assertions.assertEquals( - "Property comment is reserved or immutable and cannot be set", exception.getMessage()); + "Property comment is immutable or reserved, cannot be set", exception.getMessage()); TableChange[] changes = new TableChange[] {TableChange.setProperty("k3", "v3"), TableChange.removeProperty("k1")}; diff --git a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java index 4381270a506..b6825f67252 100644 --- a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java +++ b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java @@ -7,6 +7,7 @@ import com.datastrato.graviton.catalog.BaseCatalog; import com.datastrato.graviton.catalog.CatalogOperations; import com.datastrato.graviton.catalog.PropertiesMetadata; +import com.google.common.collect.Maps; import java.io.IOException; import java.util.Map; @@ -29,6 +30,10 @@ public void initialize(Map config) throws RuntimeException {} @Override public void close() throws IOException {} + + public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperationException { + return Maps::newHashMap; + } }; } }