Harden parsing of boolean values in CQL in PropertyDefinitions

This patch also cleans up related classes and fixes methods to return primitive values instead of objects.

patch by Stefan Miklosovic; reviewed by Aleksei Yeschenko for CASSANDRA-17878
This commit is contained in:
Stefan Miklosovic 2022-11-10 11:08:11 +01:00
parent d0e0e20f80
commit 6e3770bc15
5 changed files with 185 additions and 205 deletions

View File

@ -1,4 +1,5 @@
4.2
* Harden parsing of boolean values in CQL in PropertyDefinitions (CASSANDRA-17878)
* Add nodetool forcecompact to remove tombstoned or ttl'd data ignoring GC grace for given table and partition keys (CASSANDRA-17711)
* Offer IF (NOT) EXISTS in cqlsh completion for CREATE TYPE, DROP TYPE, CREATE ROLE and DROP ROLE (CASSANDRA-16640)
* Nodetool bootstrap resume will now return an error if the operation fails (CASSANDRA-16491)

View File

@ -17,7 +17,9 @@
*/
package org.apache.cassandra.cql3.statements;
import java.util.*;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
import org.slf4j.Logger;
@ -25,24 +27,27 @@ import org.slf4j.LoggerFactory;
import org.apache.cassandra.exceptions.SyntaxException;
import static java.lang.String.format;
public class PropertyDefinitions
{
private static final Pattern PATTERN_POSITIVE = Pattern.compile("(1|true|yes)");
private static final Pattern POSITIVE_PATTERN = Pattern.compile("(1|true|yes)");
private static final Pattern NEGATIVE_PATTERN = Pattern.compile("(0|false|no)");
protected static final Logger logger = LoggerFactory.getLogger(PropertyDefinitions.class);
protected final Map<String, Object> properties = new HashMap<String, Object>();
protected final Map<String, Object> properties = new HashMap<>();
public void addProperty(String name, String value) throws SyntaxException
{
if (properties.put(name, value) != null)
throw new SyntaxException(String.format("Multiple definition for property '%s'", name));
throw new SyntaxException(format("Multiple definitions for property '%s'", name));
}
public void addProperty(String name, Map<String, String> value) throws SyntaxException
{
if (properties.put(name, value) != null)
throw new SyntaxException(String.format("Multiple definition for property '%s'", name));
throw new SyntaxException(format("Multiple definitions for property '%s'", name));
}
public void validate(Set<String> keywords, Set<String> obsolete) throws SyntaxException
@ -55,7 +60,7 @@ public class PropertyDefinitions
if (obsolete.contains(name))
logger.warn("Ignoring obsolete property {}", name);
else
throw new SyntaxException(String.format("Unknown property '%s'", name));
throw new SyntaxException(format("Unknown property '%s'", name));
}
}
@ -72,13 +77,18 @@ public class PropertyDefinitions
properties.remove(name);
}
protected String getSimple(String name) throws SyntaxException
public boolean hasProperty(String name)
{
return properties.containsKey(name);
}
protected String getString(String name) throws SyntaxException
{
Object val = properties.get(name);
if (val == null)
return null;
if (!(val instanceof String))
throw new SyntaxException(String.format("Invalid value for property '%s'. It should be a string", name));
throw new SyntaxException(format("Invalid value for property '%s'. It should be a string", name));
return (String)val;
}
@ -88,72 +98,73 @@ public class PropertyDefinitions
if (val == null)
return null;
if (!(val instanceof Map))
throw new SyntaxException(String.format("Invalid value for property '%s'. It should be a map.", name));
throw new SyntaxException(format("Invalid value for property '%s'. It should be a map.", name));
return (Map<String, String>)val;
}
public Boolean hasProperty(String name)
public boolean getBoolean(String key, boolean defaultValue) throws SyntaxException
{
return properties.containsKey(name);
String value = getString(key);
return value != null ? parseBoolean(key, value) : defaultValue;
}
public String getString(String key, String defaultValue) throws SyntaxException
public static boolean parseBoolean(String key, String value) throws SyntaxException
{
String value = getSimple(key);
return value != null ? value : defaultValue;
if (null == value)
throw new IllegalArgumentException("value argument can't be null");
String lowerCasedValue = value.toLowerCase();
if (POSITIVE_PATTERN.matcher(lowerCasedValue).matches())
return true;
else if (NEGATIVE_PATTERN.matcher(lowerCasedValue).matches())
return false;
throw new SyntaxException(format("Invalid boolean value %s for '%s'. " +
"Positive values can be '1', 'true' or 'yes'. " +
"Negative values can be '0', 'false' or 'no'.",
value, key));
}
// Return a property value, typed as a Boolean
public Boolean getBoolean(String key, Boolean defaultValue) throws SyntaxException
public int getInt(String key, int defaultValue) throws SyntaxException
{
String value = getSimple(key);
return (value == null) ? defaultValue : PATTERN_POSITIVE.matcher(value.toLowerCase()).matches();
String value = getString(key);
return value != null ? parseInt(key, value) : defaultValue;
}
public static int parseInt(String key, String value) throws SyntaxException
{
if (null == value)
throw new IllegalArgumentException("value argument can't be null");
try
{
return Integer.parseInt(value);
}
catch (NumberFormatException e)
{
throw new SyntaxException(format("Invalid integer value %s for '%s'", value, key));
}
}
// Return a property value, typed as a double
public double getDouble(String key, double defaultValue) throws SyntaxException
{
String value = getSimple(key);
if (value == null)
{
return defaultValue;
}
else
{
try
{
return Double.parseDouble(value);
}
catch (NumberFormatException e)
{
throw new SyntaxException(String.format("Invalid double value %s for '%s'", value, key));
}
}
String value = getString(key);
return value != null ? parseDouble(key, value) : defaultValue;
}
// Return a property value, typed as an Integer
public Integer getInt(String key, Integer defaultValue) throws SyntaxException
public static double parseDouble(String key, String value) throws SyntaxException
{
String value = getSimple(key);
return toInt(key, value, defaultValue);
}
if (null == value)
throw new IllegalArgumentException("value argument can't be null");
public static Integer toInt(String key, String value, Integer defaultValue) throws SyntaxException
{
if (value == null)
try
{
return defaultValue;
return Double.parseDouble(value);
}
else
catch (NumberFormatException e)
{
try
{
return Integer.valueOf(value);
}
catch (NumberFormatException e)
{
throw new SyntaxException(String.format("Invalid integer value %s for '%s'", value, key));
}
throw new SyntaxException(format("Invalid double value %s for '%s'", value, key));
}
}
}

View File

@ -25,7 +25,6 @@ import com.google.common.collect.Sets;
import org.apache.cassandra.cql3.statements.PropertyDefinitions;
import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.exceptions.SyntaxException;
import org.apache.cassandra.schema.CachingParams;
import org.apache.cassandra.schema.CompactionParams;
import org.apache.cassandra.schema.CompressionParams;
@ -37,6 +36,7 @@ import org.apache.cassandra.service.reads.SpeculativeRetryPolicy;
import org.apache.cassandra.service.reads.repair.ReadRepairStrategy;
import static java.lang.String.format;
import static org.apache.cassandra.schema.TableParams.Option.*;
public final class TableAttributes extends PropertyDefinitions
{
@ -74,7 +74,7 @@ public final class TableAttributes extends PropertyDefinitions
public TableId getId() throws ConfigurationException
{
String id = getSimple(ID);
String id = getString(ID);
try
{
return id != null ? TableId.fromString(id) : null;
@ -100,111 +100,75 @@ public final class TableAttributes extends PropertyDefinitions
if (hasOption(Option.ALLOW_AUTO_SNAPSHOT))
builder.allowAutoSnapshot(getBoolean(Option.ALLOW_AUTO_SNAPSHOT.toString(), true));
if (hasOption(Option.BLOOM_FILTER_FP_CHANCE))
builder.bloomFilterFpChance(getDouble(Option.BLOOM_FILTER_FP_CHANCE));
if (hasOption(BLOOM_FILTER_FP_CHANCE))
builder.bloomFilterFpChance(getDouble(BLOOM_FILTER_FP_CHANCE));
if (hasOption(Option.CACHING))
builder.caching(CachingParams.fromMap(getMap(Option.CACHING)));
if (hasOption(CACHING))
builder.caching(CachingParams.fromMap(getMap(CACHING)));
if (hasOption(Option.COMMENT))
builder.comment(getString(Option.COMMENT));
if (hasOption(COMMENT))
builder.comment(getString(COMMENT));
if (hasOption(Option.COMPACTION))
builder.compaction(CompactionParams.fromMap(getMap(Option.COMPACTION)));
if (hasOption(COMPACTION))
builder.compaction(CompactionParams.fromMap(getMap(COMPACTION)));
if (hasOption(Option.COMPRESSION))
if (hasOption(COMPRESSION))
{
//crc_check_chance was "promoted" from a compression property to a top-level-property after #9839
//crc_check_chance was "promoted" from a compression property to a top-level-property after #9839,
//so we temporarily accept it to be defined as a compression option, to maintain backwards compatibility
Map<String, String> compressionOpts = getMap(Option.COMPRESSION);
if (compressionOpts.containsKey(Option.CRC_CHECK_CHANCE.toString().toLowerCase()))
Map<String, String> compressionOpts = getMap(COMPRESSION);
if (compressionOpts.containsKey(CRC_CHECK_CHANCE.toString().toLowerCase()))
{
Double crcCheckChance = getDeprecatedCrcCheckChance(compressionOpts);
double crcCheckChance = getDeprecatedCrcCheckChance(compressionOpts);
builder.crcCheckChance(crcCheckChance);
}
builder.compression(CompressionParams.fromMap(getMap(Option.COMPRESSION)));
builder.compression(CompressionParams.fromMap(getMap(COMPRESSION)));
}
if (hasOption(Option.MEMTABLE))
builder.memtable(MemtableParams.get(getString(Option.MEMTABLE)));
if (hasOption(Option.DEFAULT_TIME_TO_LIVE))
builder.defaultTimeToLive(getInt(Option.DEFAULT_TIME_TO_LIVE));
if (hasOption(DEFAULT_TIME_TO_LIVE))
builder.defaultTimeToLive(getInt(DEFAULT_TIME_TO_LIVE));
if (hasOption(Option.GC_GRACE_SECONDS))
builder.gcGraceSeconds(getInt(Option.GC_GRACE_SECONDS));
if (hasOption(GC_GRACE_SECONDS))
builder.gcGraceSeconds(getInt(GC_GRACE_SECONDS));
if (hasOption(Option.MAX_INDEX_INTERVAL))
builder.maxIndexInterval(getInt(Option.MAX_INDEX_INTERVAL));
if (hasOption(MAX_INDEX_INTERVAL))
builder.maxIndexInterval(getInt(MAX_INDEX_INTERVAL));
if (hasOption(Option.MEMTABLE_FLUSH_PERIOD_IN_MS))
builder.memtableFlushPeriodInMs(getInt(Option.MEMTABLE_FLUSH_PERIOD_IN_MS));
if (hasOption(MEMTABLE_FLUSH_PERIOD_IN_MS))
builder.memtableFlushPeriodInMs(getInt(MEMTABLE_FLUSH_PERIOD_IN_MS));
if (hasOption(Option.MIN_INDEX_INTERVAL))
builder.minIndexInterval(getInt(Option.MIN_INDEX_INTERVAL));
if (hasOption(MIN_INDEX_INTERVAL))
builder.minIndexInterval(getInt(MIN_INDEX_INTERVAL));
if (hasOption(Option.SPECULATIVE_RETRY))
builder.speculativeRetry(SpeculativeRetryPolicy.fromString(getString(Option.SPECULATIVE_RETRY)));
if (hasOption(SPECULATIVE_RETRY))
builder.speculativeRetry(SpeculativeRetryPolicy.fromString(getString(SPECULATIVE_RETRY)));
if (hasOption(Option.ADDITIONAL_WRITE_POLICY))
builder.additionalWritePolicy(SpeculativeRetryPolicy.fromString(getString(Option.ADDITIONAL_WRITE_POLICY)));
if (hasOption(ADDITIONAL_WRITE_POLICY))
builder.additionalWritePolicy(SpeculativeRetryPolicy.fromString(getString(ADDITIONAL_WRITE_POLICY)));
if (hasOption(Option.CRC_CHECK_CHANCE))
builder.crcCheckChance(getDouble(Option.CRC_CHECK_CHANCE));
if (hasOption(CRC_CHECK_CHANCE))
builder.crcCheckChance(getDouble(CRC_CHECK_CHANCE));
if (hasOption(Option.CDC))
builder.cdc(getBoolean(Option.CDC.toString(), false));
if (hasOption(CDC))
builder.cdc(getBoolean(CDC));
if (hasOption(Option.READ_REPAIR))
builder.readRepair(ReadRepairStrategy.fromString(getString(Option.READ_REPAIR)));
if (hasOption(READ_REPAIR))
builder.readRepair(ReadRepairStrategy.fromString(getString(READ_REPAIR)));
return builder.build();
}
private Double getDeprecatedCrcCheckChance(Map<String, String> compressionOpts)
public boolean hasOption(Option option)
{
String value = compressionOpts.get(Option.CRC_CHECK_CHANCE.toString().toLowerCase());
try
{
return Double.valueOf(value);
}
catch (NumberFormatException e)
{
throw new SyntaxException(String.format("Invalid double value %s for crc_check_chance.'", value));
}
}
private double getDouble(Option option)
{
String value = getString(option);
try
{
return Double.parseDouble(value);
}
catch (NumberFormatException e)
{
throw new SyntaxException(format("Invalid double value %s for '%s'", value, option));
}
}
private int getInt(Option option)
{
String value = getString(option);
try
{
return Integer.parseInt(value);
}
catch (NumberFormatException e)
{
throw new SyntaxException(String.format("Invalid integer value %s for '%s'", value, option));
}
return hasProperty(option.toString());
}
private String getString(Option option)
{
String value = getSimple(option.toString());
String value = getString(option.toString());
if (value == null)
throw new IllegalStateException(format("Option '%s' is absent", option));
return value;
@ -218,8 +182,24 @@ public final class TableAttributes extends PropertyDefinitions
return value;
}
public boolean hasOption(Option option)
private boolean getBoolean(Option option)
{
return hasProperty(option.toString());
return parseBoolean(option.toString(), getString(option));
}
private int getInt(Option option)
{
return parseInt(option.toString(), getString(option));
}
private double getDouble(Option option)
{
return parseDouble(option.toString(), getString(option));
}
private double getDeprecatedCrcCheckChance(Map<String, String> compressionOpts)
{
String value = compressionOpts.get(CRC_CHECK_CHANCE.toString().toLowerCase());
return parseDouble(CRC_CHECK_CHANCE.toString(), value);
}
}

View File

@ -36,6 +36,7 @@ import org.apache.cassandra.utils.ByteBufferUtil;
import static java.lang.String.format;
import static java.util.stream.Collectors.toMap;
import static org.apache.cassandra.schema.TableParams.Option.*;
public final class TableParams
{
@ -90,7 +91,7 @@ public final class TableParams
{
comment = builder.comment;
allowAutoSnapshot = builder.allowAutoSnapshot;
bloomFilterFpChance = builder.bloomFilterFpChance == null
bloomFilterFpChance = builder.bloomFilterFpChance == -1
? builder.compaction.defaultBloomFilterFbChance()
: builder.bloomFilterFpChance;
crcCheckChance = builder.crcCheckChance;
@ -151,7 +152,7 @@ public final class TableParams
if (bloomFilterFpChance <= minBloomFilterFpChanceValue || bloomFilterFpChance > 1)
{
fail("%s must be larger than %s and less than or equal to 1.0 (got %s)",
Option.BLOOM_FILTER_FP_CHANCE,
BLOOM_FILTER_FP_CHANCE,
minBloomFilterFpChanceValue,
bloomFilterFpChance);
}
@ -159,33 +160,33 @@ public final class TableParams
if (crcCheckChance < 0 || crcCheckChance > 1.0)
{
fail("%s must be larger than or equal to 0 and smaller than or equal to 1.0 (got %s)",
Option.CRC_CHECK_CHANCE,
CRC_CHECK_CHANCE,
crcCheckChance);
}
if (defaultTimeToLive < 0)
fail("%s must be greater than or equal to 0 (got %s)", Option.DEFAULT_TIME_TO_LIVE, defaultTimeToLive);
fail("%s must be greater than or equal to 0 (got %s)", DEFAULT_TIME_TO_LIVE, defaultTimeToLive);
if (defaultTimeToLive > Attributes.MAX_TTL)
fail("%s must be less than or equal to %d (got %s)", Option.DEFAULT_TIME_TO_LIVE, Attributes.MAX_TTL, defaultTimeToLive);
fail("%s must be less than or equal to %d (got %s)", DEFAULT_TIME_TO_LIVE, Attributes.MAX_TTL, defaultTimeToLive);
if (gcGraceSeconds < 0)
fail("%s must be greater than or equal to 0 (got %s)", Option.GC_GRACE_SECONDS, gcGraceSeconds);
fail("%s must be greater than or equal to 0 (got %s)", GC_GRACE_SECONDS, gcGraceSeconds);
if (minIndexInterval < 1)
fail("%s must be greater than or equal to 1 (got %s)", Option.MIN_INDEX_INTERVAL, minIndexInterval);
fail("%s must be greater than or equal to 1 (got %s)", MIN_INDEX_INTERVAL, minIndexInterval);
if (maxIndexInterval < minIndexInterval)
{
fail("%s must be greater than or equal to %s (%s) (got %s)",
Option.MAX_INDEX_INTERVAL,
Option.MIN_INDEX_INTERVAL,
MAX_INDEX_INTERVAL,
MIN_INDEX_INTERVAL,
minIndexInterval,
maxIndexInterval);
}
if (memtableFlushPeriodInMs < 0)
fail("%s must be greater than or equal to 0 (got %s)", Option.MEMTABLE_FLUSH_PERIOD_IN_MS, memtableFlushPeriodInMs);
fail("%s must be greater than or equal to 0 (got %s)", MEMTABLE_FLUSH_PERIOD_IN_MS, memtableFlushPeriodInMs);
if (cdc && memtable.factory().writesShouldSkipCommitLog())
fail("CDC cannot work if writes skip the commit log. Check your memtable configuration.");
@ -208,6 +209,7 @@ public final class TableParams
TableParams p = (TableParams) o;
return comment.equals(p.comment)
&& additionalWritePolicy.equals(p.additionalWritePolicy)
&& allowAutoSnapshot == p.allowAutoSnapshot
&& bloomFilterFpChance == p.bloomFilterFpChance
&& crcCheckChance == p.crcCheckChance
@ -230,6 +232,7 @@ public final class TableParams
public int hashCode()
{
return Objects.hashCode(comment,
additionalWritePolicy,
allowAutoSnapshot,
bloomFilterFpChance,
crcCheckChance,
@ -252,23 +255,24 @@ public final class TableParams
public String toString()
{
return MoreObjects.toStringHelper(this)
.add(Option.COMMENT.toString(), comment)
.add(Option.ALLOW_AUTO_SNAPSHOT.toString(), allowAutoSnapshot)
.add(Option.BLOOM_FILTER_FP_CHANCE.toString(), bloomFilterFpChance)
.add(Option.CRC_CHECK_CHANCE.toString(), crcCheckChance)
.add(Option.GC_GRACE_SECONDS.toString(), gcGraceSeconds)
.add(Option.DEFAULT_TIME_TO_LIVE.toString(), defaultTimeToLive)
.add(Option.MEMTABLE_FLUSH_PERIOD_IN_MS.toString(), memtableFlushPeriodInMs)
.add(Option.MIN_INDEX_INTERVAL.toString(), minIndexInterval)
.add(Option.MAX_INDEX_INTERVAL.toString(), maxIndexInterval)
.add(Option.SPECULATIVE_RETRY.toString(), speculativeRetry)
.add(Option.CACHING.toString(), caching)
.add(Option.COMPACTION.toString(), compaction)
.add(Option.COMPRESSION.toString(), compression)
.add(Option.MEMTABLE.toString(), memtable)
.add(Option.EXTENSIONS.toString(), extensions)
.add(Option.CDC.toString(), cdc)
.add(Option.READ_REPAIR.toString(), readRepair)
.add(COMMENT.toString(), comment)
.add(ADDITIONAL_WRITE_POLICY.toString(), additionalWritePolicy)
.add(ALLOW_AUTO_SNAPSHOT.toString(), allowAutoSnapshot)
.add(BLOOM_FILTER_FP_CHANCE.toString(), bloomFilterFpChance)
.add(CRC_CHECK_CHANCE.toString(), crcCheckChance)
.add(GC_GRACE_SECONDS.toString(), gcGraceSeconds)
.add(DEFAULT_TIME_TO_LIVE.toString(), defaultTimeToLive)
.add(MEMTABLE_FLUSH_PERIOD_IN_MS.toString(), memtableFlushPeriodInMs)
.add(MIN_INDEX_INTERVAL.toString(), minIndexInterval)
.add(MAX_INDEX_INTERVAL.toString(), maxIndexInterval)
.add(SPECULATIVE_RETRY.toString(), speculativeRetry)
.add(CACHING.toString(), caching)
.add(COMPACTION.toString(), compaction)
.add(COMPRESSION.toString(), compression)
.add(MEMTABLE.toString(), memtable)
.add(EXTENSIONS.toString(), extensions)
.add(CDC.toString(), cdc)
.add(READ_REPAIR.toString(), readRepair)
.toString();
}
@ -325,7 +329,7 @@ public final class TableParams
{
private String comment = "";
private boolean allowAutoSnapshot = true;
private Double bloomFilterFpChance;
private double bloomFilterFpChance = -1;
private double crcCheckChance = 1.0;
private int gcGraceSeconds = 864000; // 10 days
private int defaultTimeToLive = 0;

View File

@ -20,62 +20,46 @@
*/
package org.apache.cassandra.cql3.statements;
import org.junit.After;
import org.junit.Test;
import org.junit.Before;
import static org.junit.Assert.assertEquals;
import org.apache.cassandra.exceptions.SyntaxException;
public class PropertyDefinitionsTest {
PropertyDefinitions pd;
@Before
public void setUp()
import static org.apache.cassandra.cql3.statements.PropertyDefinitions.parseBoolean;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
public class PropertyDefinitionsTest
{
@Test
public void testPostiveBooleanParsing()
{
pd = new PropertyDefinitions();
assertTrue(parseBoolean("prop1", "1"));
assertTrue(parseBoolean("prop2", "true"));
assertTrue(parseBoolean("prop3", "True"));
assertTrue(parseBoolean("prop4", "TrUe"));
assertTrue(parseBoolean("prop5", "yes"));
assertTrue(parseBoolean("prop6", "Yes"));
}
@After
public void clear()
{
pd = null;
}
@Test
public void testGetBooleanExistant()
public void testNegativeBooleanParsing()
{
String key = "one";
pd.addProperty(key, "1");
assertEquals(Boolean.TRUE, pd.getBoolean(key, null));
key = "TRUE";
pd.addProperty(key, "TrUe");
assertEquals(Boolean.TRUE, pd.getBoolean(key, null));
key = "YES";
pd.addProperty(key, "YeS");
assertEquals(Boolean.TRUE, pd.getBoolean(key, null));
key = "BAD_ONE";
pd.addProperty(key, " 1");
assertEquals(Boolean.FALSE, pd.getBoolean(key, null));
key = "BAD_TRUE";
pd.addProperty(key, "true ");
assertEquals(Boolean.FALSE, pd.getBoolean(key, null));
key = "BAD_YES";
pd.addProperty(key, "ye s");
assertEquals(Boolean.FALSE, pd.getBoolean(key, null));
assertFalse(parseBoolean("prop1", "0"));
assertFalse(parseBoolean("prop2", "false"));
assertFalse(parseBoolean("prop3", "False"));
assertFalse(parseBoolean("prop4", "FaLse"));
assertFalse(parseBoolean("prop6", "No"));
}
@Test
public void testGetBooleanNonexistant()
@Test(expected = SyntaxException.class)
public void testInvalidPositiveBooleanParsing()
{
assertEquals(Boolean.FALSE, pd.getBoolean("nonexistant", Boolean.FALSE));
assertEquals(Boolean.TRUE, pd.getBoolean("nonexistant", Boolean.TRUE));
parseBoolean("cdc", "tru");
}
@Test(expected = SyntaxException.class)
public void testInvalidNegativeBooleanParsing()
{
parseBoolean("cdc", "fals");
}
}