incubator-blur-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Williams <william...@gmail.com>
Subject Re: git commit: BLUR:110 fixed.
Date Tue, 04 Jun 2013 13:28:06 GMT
On Tue, Jun 4, 2013 at 12:56 AM,  <gagz@apache.org> 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 <gagandeepjuneja@gmail.com>
> Authored: Tue Jun 4 10:26:21 2013 +0530
> Committer: Gagan <gagandeepjuneja@gmail.com>
> 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

Mime
View raw message