Skip to content

Commit

Permalink
handle validation methods on pre-parse even for properties not mentio…
Browse files Browse the repository at this point in the history 
…ned in yaml
  • Loading branch information
@Mmuzaf
Mmuzaf committed Jun 2, 2023
1 parent 990e71f commit 7862d91
Show file tree
Hide file tree
Showing 10 changed files with 254 additions and 50 deletions.
8 changes: 0 additions & 8 deletions src/java/org/apache/cassandra/config/DatabaseDescriptor.java
View file
Expand Up @@ -471,13 +471,6 @@ private static void applySimpleConfig()
//InetAddressAndPort and get the right defaults
InetAddressAndPort.initializeDefaultPort(getStoragePort());

// below 2 checks are needed in order to match the pre-CASSANDRA-15234 upper bound for those parameters which were still in megabits per second
setProperty(ConfigFields.STREAM_THROUGHPUT_OUTBOUND, conf.stream_throughput_outbound);
setProperty(ConfigFields.INTER_DC_STREAM_THROUGHPUT_OUTBOUND, conf.inter_dc_stream_throughput_outbound);
setProperty(ConfigFields.ENTIRE_SSTABLE_STREAM_THROUGHPUT_OUTBOUND, conf.entire_sstable_stream_throughput_outbound);
setProperty(ConfigFields.ENTIRE_SSTABLE_INTER_DC_STREAM_THROUGHPUT_OUTBOUND, conf.entire_sstable_inter_dc_stream_throughput_outbound);
setProperty(ConfigFields.COMPACTION_THROUGHPUT, conf.compaction_throughput);

if (conf.auto_snapshot_ttl != null)
{
try
Expand Down Expand Up @@ -606,7 +599,6 @@ else if (conf.disk_access_mode == Config.DiskAccessMode.mmap_index_only)
conf.repair_session_max_tree_depth = 20;
}

setProperty(ConfigFields.REPAIR_SESSION_SPACE, conf.repair_session_space);
checkForLowestAcceptedTimeouts(conf);

long valueInBytes = conf.native_transport_max_frame_size.toBytes();
Expand Down
2 changes: 2 additions & 0 deletions src/java/org/apache/cassandra/config/ValidatedBy.java
View file
Expand Up @@ -19,6 +19,7 @@
package org.apache.cassandra.config;

import java.lang.annotation.ElementType;
import java.lang.annotation.Repeatable;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
Expand All @@ -29,6 +30,7 @@
*/
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.FIELD)
@Repeatable(ValidatedByList.class)
public @interface ValidatedBy
{
String useClass();
Expand Down
145 changes: 117 additions & 28 deletions src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
View file
Expand Up @@ -27,6 +27,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.Nullable;

import com.google.common.annotations.VisibleForTesting;
Expand All @@ -45,14 +46,20 @@
import org.yaml.snakeyaml.TypeDescription;
import org.yaml.snakeyaml.Yaml;
import org.yaml.snakeyaml.composer.Composer;
import org.yaml.snakeyaml.constructor.BaseConstructor;
import org.yaml.snakeyaml.constructor.Constructor;
import org.yaml.snakeyaml.constructor.CustomClassLoaderConstructor;
import org.yaml.snakeyaml.constructor.SafeConstructor;
import org.yaml.snakeyaml.error.YAMLException;
import org.yaml.snakeyaml.introspector.BeanAccess;
import org.yaml.snakeyaml.introspector.MissingProperty;
import org.yaml.snakeyaml.introspector.Property;
import org.yaml.snakeyaml.introspector.PropertyUtils;
import org.yaml.snakeyaml.nodes.MappingNode;
import org.yaml.snakeyaml.nodes.Node;
import org.yaml.snakeyaml.nodes.NodeId;
import org.yaml.snakeyaml.nodes.NodeTuple;
import org.yaml.snakeyaml.nodes.ScalarNode;
import org.yaml.snakeyaml.nodes.Tag;
import org.yaml.snakeyaml.representer.Represent;
import org.yaml.snakeyaml.representer.Representer;
Expand Down Expand Up @@ -137,8 +144,7 @@ public Config loadConfig(URL url) throws ConfigurationException
Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
verifyReplacements(replacements, configBytes);
PropertiesChecker propertiesChecker = new PropertiesChecker();
Yaml yaml = YamlFactory.instance.newYamlInstance(new CustomConstructor(Config.class, Yaml.class.getClassLoader()),
propertiesChecker);
Yaml yaml = YamlFactory.instance.newYamlInstance(new ConfigWithValidationConstructor(Config.class, Yaml.class.getClassLoader()), propertiesChecker);
Config result = loadConfig(yaml, configBytes);
propertiesChecker.check();
maybeAddSystemProperties(result);
Expand Down Expand Up @@ -218,7 +224,7 @@ public static <T> T fromMap(Map<String,Object> map, Class<T> klass)
@SuppressWarnings("unchecked") //getSingleData returns Object, not T
public static <T> T fromMap(Map<String,Object> map, boolean shouldCheck, Class<T> klass)
{
SafeConstructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader());
SafeConstructor constructor = new ConfigWithValidationConstructor(klass, klass.getClassLoader());
Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
verifyReplacements(replacements, map);
YamlConfigurationLoader.PropertiesChecker propertiesChecker = new PropertiesChecker();
Expand All @@ -242,7 +248,7 @@ public Node getSingleNode()
public static <T> T updateFromMap(Map<String, ?> map, boolean shouldCheck, T obj)
{
Class<T> klass = (Class<T>) obj.getClass();
SafeConstructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader())
SafeConstructor constructor = new ConfigWithValidationConstructor(klass, klass.getClassLoader())
{
@Override
protected Object newInstance(Node node)
Expand All @@ -255,7 +261,6 @@ protected Object newInstance(Node node)
Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
verifyReplacements(replacements, map);
YamlConfigurationLoader.PropertiesChecker propertiesChecker = new PropertiesChecker();
constructor.setPropertyUtils(propertiesChecker);
Yaml yaml = YamlFactory.instance.newYamlInstance(constructor, propertiesChecker);
Node node = yaml.represent(map);
constructor.setComposer(new Composer(null, null)
Expand All @@ -272,20 +277,104 @@ public Node getSingleNode()
return value;
}

/**
* For the configuration properties that are not mentioned in the configuration yaml file, we need additionally
* trigger the validation methods for the remaining properties to make sure the default values are valid and set.
* These validation methods are configured with {@link ValidatedBy} for each property. This will create
* a {@link MappingNode} with default values obtained from the {@link Config} class intance to finalize the
* configuration instance creation.
*/
@VisibleForTesting
static class CustomConstructor extends CustomClassLoaderConstructor
static class ConfigWithValidationConstructor extends CustomClassLoaderConstructor
{
CustomConstructor(Class<?> theRoot, ClassLoader classLoader)
private final Class<?> theRoot;

public ConfigWithValidationConstructor(Class<?> theRoot, ClassLoader classLoader)
{
super(theRoot, classLoader);
this.theRoot = theRoot;
yamlClassConstructors.put(NodeId.mapping, new CassandraMappingConstructor());
}

private class CassandraMappingConstructor extends Constructor.ConstructMapping
{
@Override
protected Object constructJavaBean2ndStep(MappingNode loadedYamlNode, Object object)
{
Object result = super.constructJavaBean2ndStep(loadedYamlNode, object);

TypeDescription seedDesc = new TypeDescription(ParameterizedClass.class);
seedDesc.putMapPropertyType("parameters", String.class, String.class);
addTypeDescription(seedDesc);
// If the node is not the root node, we don't need to handle the validation.
if (!loadedYamlNode.getTag().equals(rootTag))
return result;

TypeDescription memtableDesc = new TypeDescription(Config.MemtableOptions.class);
memtableDesc.addPropertyParameters("configurations", String.class, InheritingClass.class);
addTypeDescription(memtableDesc);
assert theRoot.isInstance(result);
Representer representer = YamlFactory.representer(new PropertyUtils()
{
private final Loader loader = Properties.defaultLoader();

@Override
protected Map<String, Property> getPropertiesMap(Class<?> type, BeanAccess bAccess)
{
Map<String, Property> result = loader.getProperties(type);
// Filter out properties that are not validated by any method.
return result.entrySet()
.stream()
.filter(e -> {
Property property = e.getValue();
return property.getAnnotation(ValidatedBy.class) != null &&
(property.getAnnotation(ValidatedByList.class) == null ||
property.getAnnotation(ValidatedByList.class).value().length == 0);
})
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}
});

// Load default values from the Config class instance for the properties that are not mentioned
// in the configuration yaml file.
MappingNode allWithDefauls = (MappingNode) new Yaml(representer).represent(object);
allWithDefauls.setType(theRoot);
removeIfLoadedFromYaml(loadedYamlNode, allWithDefauls);

// This will trigger the validation and default values set for the remaining properties
// that annotated with @ValidatedBy.
return super.constructJavaBean2ndStep(allWithDefauls, result);
}

private void removeIfLoadedFromYaml(MappingNode loadedYamlNode, MappingNode all)
{
all.getValue().removeIf(nodeTuple -> {
Node valueNode = nodeTuple.getValueNode();
String key = mappingKeyHandleReplacements((ScalarNode) nodeTuple.getKeyNode(), all.getType());
Node mappingNodeOrig = mappingNodeByKey(loadedYamlNode, key);

if (valueNode instanceof MappingNode && mappingNodeOrig instanceof MappingNode)
removeIfLoadedFromYaml((MappingNode) mappingNodeOrig, (MappingNode) valueNode);

return mappingNodeOrig != null;
});
}

private String mappingKeyHandleReplacements(ScalarNode keyNode, Class<?> rootType)
{
keyNode.setType(String.class);
String key = (String) constructObject(keyNode);
List<Replacement> replacements = Replacements.getReplacements(rootType);
for (Replacement replacement : replacements)
{
if (replacement.oldName.equals(key))
return replacement.newName;
}
return key;
}

private Node mappingNodeByKey(MappingNode node, String key)
{
return node.getValue().stream()
.filter(t -> mappingKeyHandleReplacements((ScalarNode) t.getKeyNode(), node.getType()).equals(key))
.findFirst()
.map(NodeTuple::getValueNode)
.orElse(null);
}
}

@Override
Expand Down Expand Up @@ -325,8 +414,6 @@ private static class PropertiesChecker extends PropertyUtils
private final Loader loader = Properties.withReplacementsLoader(Properties.validatedPropertyLoader());
private final Set<String> missingProperties = new HashSet<>();

private final Set<String> nullProperties = new HashSet<>();

private final Set<String> deprecationWarnings = new HashSet<>();

PropertiesChecker()
Expand Down Expand Up @@ -360,7 +447,7 @@ public void set(Object object, Object value) throws Exception
{
// TODO: CASSANDRA-17785, add @Nullable to all nullable Config properties and remove value == null
if (value == null && get(object) != null && !allowsNull)
nullProperties.add(getName());
throw new ConfigurationException("Invalid yaml. The property '" + result.getName() + "' can't be null", false);

result.set(object, value);
}
Expand Down Expand Up @@ -405,9 +492,6 @@ private Property getNestedProperty(Class<?> type, String name)

public void check() throws ConfigurationException
{
if (!nullProperties.isEmpty())
throw new ConfigurationException("Invalid yaml. Those properties " + nullProperties + " are not valid", false);

if (!missingProperties.isEmpty())
throw new ConfigurationException("Invalid yaml. Please remove properties " + missingProperties + " from your cassandra.yaml", false);

Expand Down Expand Up @@ -441,15 +525,25 @@ public Yaml newYamlInstance(Class<?> root)
PropertyUtils propertyUtils = new PropertyUtils();
propertyUtils.setBeanAccess(BeanAccess.FIELD);
propertyUtils.setAllowReadOnlyProperties(true);
return newYamlInstance(new CustomConstructor(root, Config.class.getClassLoader()), propertyUtils);
return newYamlInstance(new ConfigWithValidationConstructor(root, Config.class.getClassLoader()), propertyUtils);
}

public Yaml newYamlInstance(SafeConstructor constructor, PropertyUtils propertyUtils)
public Yaml newYamlInstance(BaseConstructor constructor, PropertyUtils propertyUtils)
{
return create(constructor, propertyUtils);
}

private static Yaml create(SafeConstructor constructor, PropertyUtils propertyUtils)
private static Yaml create(BaseConstructor constructor, PropertyUtils propertyUtils)
{
constructor.setPropertyUtils(propertyUtils);
Yaml yaml = new Yaml(constructor, representer(propertyUtils));

scalarCassandraTypes.forEach(yaml::addTypeDescription);
javaBeanCassandraTypes.forEach(yaml::addTypeDescription);
return yaml;
}

private static Representer representer(PropertyUtils propertyUtils)
{
DumperOptions options = new DumperOptions();
options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK);
Expand All @@ -458,12 +552,7 @@ private static Yaml create(SafeConstructor constructor, PropertyUtils propertyUt
options.setPrettyFlow(true); // to remove brackets around
Representer representer = new CassandraRepresenter(scalarCassandraTypes, options);
representer.setPropertyUtils(propertyUtils);
constructor.setPropertyUtils(propertyUtils);
Yaml yaml = new Yaml(constructor, representer);

scalarCassandraTypes.forEach(yaml::addTypeDescription);
javaBeanCassandraTypes.forEach(yaml::addTypeDescription);
return yaml;
return representer;
}

private static List<TypeDescription> loadScalarTypeDescriptions()
Expand Down
15 changes: 15 additions & 0 deletions test/conf/cassandra-validatedby-cases.yaml
View file
@@ -0,0 +1,15 @@
#
# Some configuration options are validated by the methods used in the @ValidatedBy annotation.
#
sstable_preemptive_open_interval:
credentials_update_interval: ~

# Warning! This is for test the backward compatibility of the configuration stream_throughput_outbound.
stream_throughput_outbound_megabits_per_sec: 200000000

# Null value must be handled by the validation method and the corresponding default value must be set correctly.
repair_session_space: null

# non-default values are validated by the methods used in the @ValidatedBy annotation and loaded.
snapshot_links_per_second: 10
compaction_throughput: 48MiB/s
3 changes: 3 additions & 0 deletions test/conf/cassandra-validatedby-null-case.yaml
View file
@@ -0,0 +1,3 @@
# Null value are not allowed for the configuration property as it is not annotated by @Nullable,
# so the validation method must throw an exception.
entire_sstable_inter_dc_stream_throughput_outbound:
15 changes: 13 additions & 2 deletions test/unit/org/apache/cassandra/config/DatabaseDescriptorRefTest.java
View file
Expand Up @@ -102,6 +102,10 @@ public class DatabaseDescriptorRefTest
"org.apache.cassandra.config.Config$PaxosVariant",
"org.apache.cassandra.config.Config$RepairCommandPoolFullStrategy",
"org.apache.cassandra.config.Config$SSTableConfig",
"org.apache.cassandra.config.Config$SSTableConfigBeanInfo",
"org.apache.cassandra.config.Config$SSTableConfigBeanInfo",
"org.apache.cassandra.config.Config$SSTableConfigCustomizer",
"org.apache.cassandra.config.Config$SSTableConfigCustomizer",
"org.apache.cassandra.config.Config$UserFunctionTimeoutPolicy",
"org.apache.cassandra.config.ConfigBeanInfo",
"org.apache.cassandra.config.ConfigCustomizer",
Expand Down Expand Up @@ -179,6 +183,10 @@ public class DatabaseDescriptorRefTest
"org.apache.cassandra.config.DurationSpec$IntSecondsBoundCustomizer",
"org.apache.cassandra.config.DurationSpec$IntSecondsBoundCustomizer",
"org.apache.cassandra.config.DurationSpec$LongMicrosecondsBound",
"org.apache.cassandra.config.DurationSpec$LongMicrosecondsBoundBeanInfo",
"org.apache.cassandra.config.DurationSpec$LongMicrosecondsBoundBeanInfo",
"org.apache.cassandra.config.DurationSpec$LongMicrosecondsBoundCustomizer",
"org.apache.cassandra.config.DurationSpec$LongMicrosecondsBoundCustomizer",
"org.apache.cassandra.config.DurationSpec$LongMillisecondsBound",
"org.apache.cassandra.config.DurationSpec$LongMillisecondsBoundBeanInfo",
"org.apache.cassandra.config.DurationSpec$LongMillisecondsBoundBeanInfo",
Expand Down Expand Up @@ -215,8 +223,8 @@ public class DatabaseDescriptorRefTest
"org.apache.cassandra.config.GuardrailsOptions$TableProperties",
"org.apache.cassandra.config.InheritingClass",
"org.apache.cassandra.config.ListenableProperty",
"org.apache.cassandra.config.ListenableProperty$Listener",
"org.apache.cassandra.config.ListenableProperty$Invoker",
"org.apache.cassandra.config.ListenableProperty$Listener",
"org.apache.cassandra.config.ListenableProperty$PropertyValidateInvoker",
"org.apache.cassandra.config.ListenableProperty$Remover",
"org.apache.cassandra.config.Loader",
Expand Down Expand Up @@ -255,7 +263,10 @@ public class DatabaseDescriptorRefTest
"org.apache.cassandra.config.TransparentDataEncryptionOptionsCustomizer",
"org.apache.cassandra.config.TransparentDataEncryptionOptionsCustomizer",
"org.apache.cassandra.config.ValidatedBy",
"org.apache.cassandra.config.ValidatedByList",
"org.apache.cassandra.config.ValidatedPropertyLoader",
"org.apache.cassandra.config.ValidatedPropertyLoader$MethodInvokeAdapter",
"org.apache.cassandra.config.ValidatedPropertyLoader$MethodInvoker",
"org.apache.cassandra.config.ValidatedPropertyLoader$ValidationPropertyHandler",
"org.apache.cassandra.config.WithReplacementLoader",
"org.apache.cassandra.config.YamlConfigurationLoader",
Expand Down Expand Up @@ -393,7 +404,7 @@ public class DatabaseDescriptorRefTest
"org.apache.cassandra.utils.concurrent.RefCounted",
"org.apache.cassandra.utils.concurrent.SelfRefCounted",
"org.apache.cassandra.utils.concurrent.Transactional",
"org.apache.cassandra.utils.concurrent.UncheckedInterruptedException"
"org.apache.cassandra.utils.concurrent.UncheckedInterruptedException",
};

static final Set<String> checkedClasses = new HashSet<>(Arrays.asList(validClasses));
Expand Down
2 changes: 1 addition & 1 deletion test/unit/org/apache/cassandra/config/LoadOldYAMLBackwardCompatibilityTest.java
View file
Expand Up @@ -58,7 +58,7 @@ public void testConfigurationLoaderBackwardCompatibility()
assertEquals(new DurationSpec.LongMillisecondsBound(500), config.slow_query_log_timeout);
assertNull(config.memtable_heap_space);
assertNull(config.memtable_offheap_space);
assertNull( config.repair_session_space);
assertEquals(new DataStorageSpec.IntMebibytesBound("910MiB"), config.repair_session_space);
assertEquals(new DataStorageSpec.IntBytesBound(4194304), config.internode_application_send_queue_capacity);
assertEquals(new DataStorageSpec.IntBytesBound(134217728), config.internode_application_send_queue_reserve_endpoint_capacity);
assertEquals(new DataStorageSpec.IntBytesBound(536870912), config.internode_application_send_queue_reserve_global_capacity);
Expand Down

0 comments on commit 7862d91

Please sign in to comment.