Thanks Tim for bringing this into notice. I have updated the test name and added more tests as well. Sorry about that.
Regards, Gagan On Tue, Jun 4, 2013 at 6:58 PM, Tim Williams <[email protected]> wrote: > On Tue, Jun 4, 2013 at 12:56 AM, <[email protected]> wrote: >> Updated Branches: >> refs/heads/0.1.5 757288e8f -> 4c9c22cfd >> >> >> BLUR:110 fixed. >> >> >> Project: http://git-wip-us.apache.org/repos/asf/incubator-blur/repo >> Commit: http://git-wip-us.apache.org/repos/asf/incubator-blur/commit/4c9c22cf >> Tree: http://git-wip-us.apache.org/repos/asf/incubator-blur/tree/4c9c22cf >> Diff: http://git-wip-us.apache.org/repos/asf/incubator-blur/diff/4c9c22cf >> >> Branch: refs/heads/0.1.5 >> Commit: 4c9c22cfd66d54ad786eaaeba3c974c11da0f3f4 >> Parents: 757288e >> Author: Gagan <[email protected]> >> Authored: Tue Jun 4 10:26:21 2013 +0530 >> Committer: Gagan <[email protected]> >> Committed: Tue Jun 4 10:26:21 2013 +0530 >> >> ---------------------------------------------------------------------- >> .../blur/manager/writer/TransactionRecorder.java | 3 +- >> .../java/org/apache/blur/server/ShardContext.java | 2 + >> .../java/org/apache/blur/thrift/TableAdmin.java | 2 + >> .../main/java/org/apache/blur/utils/BlurUtil.java | 26 ++++++++++++ >> .../manager/writer/TransactionRecorderTest.java | 32 +++++++++++++++ >> 5 files changed, 64 insertions(+), 1 deletions(-) >> ---------------------------------------------------------------------- >> >> >> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/4c9c22cf/src/blur-core/src/main/java/org/apache/blur/manager/writer/TransactionRecorder.java >> ---------------------------------------------------------------------- >> diff --git >> a/src/blur-core/src/main/java/org/apache/blur/manager/writer/TransactionRecorder.java >> >> b/src/blur-core/src/main/java/org/apache/blur/manager/writer/TransactionRecorder.java >> index fdc0aaf..cd05ccf 100644 >> --- >> a/src/blur-core/src/main/java/org/apache/blur/manager/writer/TransactionRecorder.java >> +++ >> b/src/blur-core/src/main/java/org/apache/blur/manager/writer/TransactionRecorder.java >> @@ -42,6 +42,7 @@ import org.apache.blur.thrift.generated.Column; >> import org.apache.blur.thrift.generated.Record; >> import org.apache.blur.thrift.generated.Row; >> import org.apache.blur.utils.BlurConstants; >> +import org.apache.blur.utils.BlurUtil; >> import org.apache.blur.utils.RowIndexWriter; >> import org.apache.hadoop.conf.Configuration; >> import org.apache.hadoop.fs.FSDataInputStream; >> @@ -86,7 +87,6 @@ public class TransactionRecorder extends TimerTask >> implements Closeable { >> >> private static final Log LOG = >> LogFactory.getLog(TransactionRecorder.class); >> public static FieldType ID_TYPE; >> - >> static { >> ID_TYPE = new FieldType(); >> ID_TYPE.setIndexed(true); >> @@ -367,6 +367,7 @@ public class TransactionRecorder extends TimerTask >> implements Closeable { >> } >> >> public static Document convert(String rowId, Record record, StringBuilder >> builder, BlurAnalyzer analyzer) { >> + BlurUtil.validateRowIdAndRecord(rowId, record); >> Document document = new Document(); >> document.add(new Field(BlurConstants.ROW_ID, rowId, ID_TYPE)); >> document.add(new Field(BlurConstants.RECORD_ID, record.recordId, >> ID_TYPE)); >> >> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/4c9c22cf/src/blur-core/src/main/java/org/apache/blur/server/ShardContext.java >> ---------------------------------------------------------------------- >> diff --git >> a/src/blur-core/src/main/java/org/apache/blur/server/ShardContext.java >> b/src/blur-core/src/main/java/org/apache/blur/server/ShardContext.java >> index 0b0f808..69e3786 100644 >> --- a/src/blur-core/src/main/java/org/apache/blur/server/ShardContext.java >> +++ b/src/blur-core/src/main/java/org/apache/blur/server/ShardContext.java >> @@ -19,6 +19,7 @@ package org.apache.blur.server; >> import java.io.IOException; >> >> import org.apache.blur.store.hdfs.HdfsDirectory; >> +import org.apache.blur.utils.BlurUtil; >> import org.apache.hadoop.fs.Path; >> import org.apache.lucene.store.Directory; >> >> @@ -75,6 +76,7 @@ public class ShardContext { >> } >> >> public static ShardContext create(TableContext tableContext, String >> shard) throws IOException { >> + BlurUtil.validateShardName(shard); >> ShardContext shardContext = new ShardContext(); >> shardContext.tableContext = tableContext; >> shardContext.walShardPath = new Path(tableContext.getWalTablePath(), >> shard); >> >> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/4c9c22cf/src/blur-core/src/main/java/org/apache/blur/thrift/TableAdmin.java >> ---------------------------------------------------------------------- >> diff --git >> a/src/blur-core/src/main/java/org/apache/blur/thrift/TableAdmin.java >> b/src/blur-core/src/main/java/org/apache/blur/thrift/TableAdmin.java >> index c946276..8069764 100644 >> --- a/src/blur-core/src/main/java/org/apache/blur/thrift/TableAdmin.java >> +++ b/src/blur-core/src/main/java/org/apache/blur/thrift/TableAdmin.java >> @@ -31,6 +31,7 @@ import org.apache.blur.thrift.generated.BlurException; >> import org.apache.blur.thrift.generated.ShardState; >> import org.apache.blur.thrift.generated.TableDescriptor; >> import org.apache.blur.thrift.generated.TableStats; >> +import org.apache.blur.utils.BlurUtil; >> import org.apache.zookeeper.ZooKeeper; >> >> public abstract class TableAdmin implements Iface { >> @@ -59,6 +60,7 @@ public abstract class TableAdmin implements Iface { >> public final void createTable(TableDescriptor tableDescriptor) throws >> BlurException, TException { >> try { >> TableContext.clear(); >> + BlurUtil.validateTableName(tableDescriptor.name); >> // @todo Remove this once issue #27 is resolved >> if (tableDescriptor.compressionBlockSize > 32768) { >> tableDescriptor.compressionBlockSize = 32768; >> >> http://git-wip-us.apache.org/repos/asf/incubator-blur/blob/4c9c22cf/src/blur-core/src/main/java/org/apache/blur/utils/BlurUtil.java >> ---------------------------------------------------------------------- >> diff --git a/src/blur-core/src/main/java/org/apache/blur/utils/BlurUtil.java >> b/src/blur-core/src/main/java/org/apache/blur/utils/BlurUtil.java >> index a421a2c..029ee6e 100644 >> --- a/src/blur-core/src/main/java/org/apache/blur/utils/BlurUtil.java >> +++ b/src/blur-core/src/main/java/org/apache/blur/utils/BlurUtil.java >> @@ -45,6 +45,7 @@ import java.util.concurrent.ExecutorService; >> import java.util.concurrent.Future; >> import java.util.concurrent.TimeUnit; >> import java.util.concurrent.atomic.AtomicLongArray; >> +import java.util.regex.Pattern; >> >> import org.apache.blur.log.Log; >> import org.apache.blur.log.LogFactory; >> @@ -58,6 +59,7 @@ import org.apache.blur.thrift.generated.Blur.Iface; >> import org.apache.blur.thrift.generated.BlurQuery; >> import org.apache.blur.thrift.generated.BlurResult; >> import org.apache.blur.thrift.generated.BlurResults; >> +import org.apache.blur.thrift.generated.Column; >> import org.apache.blur.thrift.generated.FetchResult; >> import org.apache.blur.thrift.generated.Record; >> import org.apache.blur.thrift.generated.RecordMutation; >> @@ -101,6 +103,7 @@ public class BlurUtil { >> private static final Class<?>[] EMPTY_PARAMETER_TYPES = new Class[] {}; >> private static final Log LOG = LogFactory.getLog(BlurUtil.class); >> private static final String UNKNOWN = "UNKNOWN"; >> + private static Pattern validator = >> Pattern.compile("^[a-zA-Z0-9\\_\\-]+$"); >> >> @SuppressWarnings("unchecked") >> public static <T extends Iface> T recordMethodCallsAndAverageTimes(final >> T t, Class<T> clazz) { >> @@ -635,5 +638,28 @@ public class BlurUtil { >> int index = shard.indexOf('-'); >> return Integer.parseInt(shard.substring(index + 1)); >> } >> + >> + public static void validateRowIdAndRecord(String rowId, Record >> record) { > > this name says it's validating both the rowid and the record. it > seems only to validate the column family and column names. if true, > wouldn't validateRecordFamilyAndColumnNames be more descriptive? > >> + if (!validator.matcher(record.family).matches()) { >> + throw new IllegalArgumentException("Invalid column >> family name [ " + record.family + " ]. It should contain only this pattern >> [A-Za-z0-9_-]"); >> + } >> + >> + for (Column column : record.getColumns()) { >> + if (!validator.matcher(column.name).matches()) { >> + throw new IllegalArgumentException("Invalid >> column name [ " + column.name + " ]. It should contain only this pattern >> [A-Za-z0-9_-]"); >> + } >> + } >> + } > ... >> + @Test(expected=IllegalArgumentException.class) >> + public void testConvertShouldFail(){ >> + String rowId = "RowId_123.1"; >> + Record record = new Record(); >> + record.setRecordId("RecordId_123-1"); >> + record.setFamily("Family_123-1"); >> + >> + Column column = new Column(); >> + column.setName("columnName_123-1"); >> + record.setColumns(Arrays.asList(column)); >> + >> + TransactionRecorder.convert(rowId, record, new StringBuilder(), new >> BlurAnalyzer()); >> + assert(true); >> + } > > It seems there are multiple tests hiding in here. It's not clear why > it's expected to fail. Because the rowid, column name, family name, > etc.? Can we name the test method so the intent is more clear (e.g. > testConvertWithBadRowIdFails()) ? > > Thanks, > --tim
