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
