incubator-blur-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gagan Juneja <gagandeepjun...@gmail.com>
Subject Re: git commit: BLUR:110 fixed.
Date Tue, 04 Jun 2013 13:54:33 GMT
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 <williamstw@gmail.com> wrote:
> 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