Return-Path: 
 <commits-return-7169-apmail-accumulo-commits-archive=accumulo.apache.org@accumulo.apache.org>
X-Original-To: apmail-accumulo-commits-archive@www.apache.org
Delivered-To: apmail-accumulo-commits-archive@www.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 6D1D010F72
	for <apmail-accumulo-commits-archive@www.apache.org>;
 Wed, 18 Dec 2013 14:53:45 +0000 (UTC)
Received: (qmail 58235 invoked by uid 500); 18 Dec 2013 14:53:35 -0000
Delivered-To: apmail-accumulo-commits-archive@accumulo.apache.org
Received: (qmail 58164 invoked by uid 500); 18 Dec 2013 14:53:33 -0000
Mailing-List: contact commits-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:commits-help@accumulo.apache.org>
List-Unsubscribe: <mailto:commits-unsubscribe@accumulo.apache.org>
List-Post: <mailto:commits@accumulo.apache.org>
List-Id: <commits.accumulo.apache.org>
Reply-To: dev@accumulo.apache.org
Delivered-To: mailing list commits@accumulo.apache.org
Received: (qmail 58092 invoked by uid 99); 18 Dec 2013 14:53:32 -0000
Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org)
 (140.211.11.114)
    by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 18 Dec 2013 14:53:32 +0000
Received: by tyr.zones.apache.org (Postfix, from userid 65534)
	id C13D98C28; Wed, 18 Dec 2013 14:53:31 +0000 (UTC)
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: ecn@apache.org
To: commits@accumulo.apache.org
Date: Wed, 18 Dec 2013 14:53:33 -0000
Message-Id: <6a7fc14ab9ce48c4a7048423f4d3fef9@git.apache.org>
In-Reply-To: <687d1301506841f8a6e0e7b7a23e4f37@git.apache.org>
References: <687d1301506841f8a6e0e7b7a23e4f37@git.apache.org>
X-Mailer: ASF-Git Admin Mailer
Subject: [03/13] git commit: ACCUMULO-1986 Add data integrity checks to Key
 and Mutation

ACCUMULO-1986 Add data integrity checks to Key and Mutation

This change adds checks to the constructors for Key and Mutations which
take in Thrift data structures to ensure that required fields are not
null. These checks prevent creation of invalid objects from modified
Thrift structures.

Signed-off-by: Eric Newton <eric.newton@gmail.com>


Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo
Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/adee0f12
Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/adee0f12
Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/adee0f12

Branch: refs/heads/1.5.1-SNAPSHOT
Commit: adee0f129f66c346e026b1803793caa233d29930
Parents: bec36bc
Author: Bill Havanki <bhavanki@cloudera.com>
Authored: Thu Dec 12 16:15:08 2013 -0500
Committer: Eric Newton <eric.newton@gmail.com>
Committed: Wed Dec 18 09:23:47 2013 -0500

----------------------------------------------------------------------
 .../java/org/apache/accumulo/core/data/Key.java | 13 +++
 .../org/apache/accumulo/core/data/Mutation.java |  7 ++
 .../org/apache/accumulo/core/data/KeyTest.java  | 29 +++++-
 .../apache/accumulo/core/data/MutationTest.java | 97 +++++++++++++-------
 4 files changed, 111 insertions(+), 35 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/accumulo/blob/adee0f12/src/core/src/main/java/org/apache/accumulo/core/data/Key.java
----------------------------------------------------------------------
diff --git a/src/core/src/main/java/org/apache/accumulo/core/data/Key.java b/src/core/src/main/java/org/apache/accumulo/core/data/Key.java
index cfb0b5c..b6cfad7 100644
--- a/src/core/src/main/java/org/apache/accumulo/core/data/Key.java
+++ b/src/core/src/main/java/org/apache/accumulo/core/data/Key.java
@@ -291,6 +291,19 @@ public class Key implements WritableComparable<Key>, Cloneable {
     this.colVisibility = toBytes(tkey.colVisibility);
     this.timestamp = tkey.timestamp;
     this.deleted = false;
+
+    if (row == null) {
+      throw new IllegalArgumentException("null row");
+    }
+    if (colFamily == null) {
+      throw new IllegalArgumentException("null column family");
+    }
+    if (colQualifier == null) {
+      throw new IllegalArgumentException("null column qualifier");
+    }
+    if (colVisibility == null) {
+      throw new IllegalArgumentException("null column visibility");
+    }
   }
   
   /**

http://git-wip-us.apache.org/repos/asf/accumulo/blob/adee0f12/src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java
----------------------------------------------------------------------
diff --git a/src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java b/src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java
index 3979da9..6b2c09f 100644
--- a/src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java
+++ b/src/core/src/main/java/org/apache/accumulo/core/data/Mutation.java
@@ -187,6 +187,13 @@ public class Mutation implements Writable {
     this.data = ByteBufferUtil.toBytes(tmutation.data);
     this.entries = tmutation.entries;
     this.values = ByteBufferUtil.toBytesList(tmutation.values);
+
+    if (this.row == null) {
+      throw new IllegalArgumentException("null row");
+    }
+    if (this.data == null) {
+      throw new IllegalArgumentException("null serialized data");
+    }
   }
   
   public Mutation(Mutation m) {

http://git-wip-us.apache.org/repos/asf/accumulo/blob/adee0f12/src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java
----------------------------------------------------------------------
diff --git a/src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java b/src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java
index 9a7f0d7..3ea77e3 100644
--- a/src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java
+++ b/src/core/src/test/java/org/apache/accumulo/core/data/KeyTest.java
@@ -16,11 +16,18 @@
  */
 package org.apache.accumulo.core.data;
 
-import junit.framework.TestCase;
+import org.junit.Test;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 
+import org.apache.accumulo.core.data.thrift.TKey;
 import org.apache.hadoop.io.Text;
 
-public class KeyTest extends TestCase {
+public class KeyTest {
+
+  @Test
   public void testDeletedCompare() {
     Key k1 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
     Key k2 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
@@ -33,6 +40,7 @@ public class KeyTest extends TestCase {
     assertTrue(k3.compareTo(k1) < 0);
   }
   
+  @Test
   public void testCopyData() {
     byte row[] = "r".getBytes();
     byte cf[] = "cf".getBytes();
@@ -66,6 +74,7 @@ public class KeyTest extends TestCase {
     
   }
   
+  @Test
   public void testString() {
     Key k1 = new Key("r1");
     Key k2 = new Key(new Text("r1"));
@@ -93,8 +102,24 @@ public class KeyTest extends TestCase {
     
   }
   
+  @Test
   public void testVisibilityFollowingKey() {
     Key k = new Key("r", "f", "q", "v");
     assertEquals(k.followingKey(PartialKey.ROW_COLFAM_COLQUAL_COLVIS).toString(), "r f:q [v%00;] " + Long.MAX_VALUE + " false");
   }
+
+  @Test
+  public void testThrift() {
+    Key k = new Key("r1", "cf2", "cq2", "cv");
+    TKey tk = k.toThrift();
+    Key k2 = new Key(tk);
+    assertEquals(k, k2);
+  }
+  @Test(expected=IllegalArgumentException.class)
+  public void testThrift_Invalid() {
+    Key k = new Key("r1", "cf2", "cq2", "cv");
+    TKey tk = k.toThrift();
+    tk.setRow((byte[]) null);
+    new Key(tk);
+  }
 }

http://git-wip-us.apache.org/repos/asf/accumulo/blob/adee0f12/src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java
----------------------------------------------------------------------
diff --git a/src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java b/src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java
index 38ddcad..2d04430 100644
--- a/src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java
+++ b/src/core/src/test/java/org/apache/accumulo/core/data/MutationTest.java
@@ -24,12 +24,17 @@ import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
 
-import junit.framework.TestCase;
+import org.junit.Test;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
+import org.apache.accumulo.core.data.thrift.TMutation;
 import org.apache.accumulo.core.security.ColumnVisibility;
 import org.apache.hadoop.io.Text;
 
-public class MutationTest extends TestCase {
+public class MutationTest {
+  @Test
   public void test1() {
     Mutation m = new Mutation(new Text("r1"));
     m.put(new Text("cf1"), new Text("cq1"), new Value("v1".getBytes()));
@@ -47,6 +52,7 @@ public class MutationTest extends TestCase {
     
   }
   
+  @Test
   public void test2() throws IOException {
     Mutation m = new Mutation(new Text("r1"));
     m.put(new Text("cf1"), new Text("cq1"), new Value("v1".getBytes()));
@@ -113,6 +119,7 @@ public class MutationTest extends TestCase {
     return m;
   }
   
+  @Test
   public void test3() throws IOException {
     Mutation m = new Mutation(new Text("r1"));
     for (int i = 0; i < 5; i++) {
@@ -155,6 +162,7 @@ public class MutationTest extends TestCase {
     return new Value(s.getBytes());
   }
   
+  @Test
   public void testPuts() {
     Mutation m = new Mutation(new Text("r1"));
     
@@ -175,18 +183,19 @@ public class MutationTest extends TestCase {
     assertEquals(8, m.size());
     assertEquals(8, updates.size());
     
-    assertEquals(updates.get(0), "cf1", "cq1", "", 0l, false, false, "v1");
-    assertEquals(updates.get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
-    assertEquals(updates.get(2), "cf3", "cq3", "", 3l, true, false, "v3");
-    assertEquals(updates.get(3), "cf4", "cq4", "cv4", 4l, true, false, "v4");
+    verifyColumnUpdate(updates.get(0), "cf1", "cq1", "", 0l, false, false, "v1");
+    verifyColumnUpdate(updates.get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
+    verifyColumnUpdate(updates.get(2), "cf3", "cq3", "", 3l, true, false, "v3");
+    verifyColumnUpdate(updates.get(3), "cf4", "cq4", "cv4", 4l, true, false, "v4");
     
-    assertEquals(updates.get(4), "cf5", "cq5", "", 0l, false, true, "");
-    assertEquals(updates.get(5), "cf6", "cq6", "cv6", 0l, false, true, "");
-    assertEquals(updates.get(6), "cf7", "cq7", "", 7l, true, true, "");
-    assertEquals(updates.get(7), "cf8", "cq8", "cv8", 8l, true, true, "");
+    verifyColumnUpdate(updates.get(4), "cf5", "cq5", "", 0l, false, true, "");
+    verifyColumnUpdate(updates.get(5), "cf6", "cq6", "cv6", 0l, false, true, "");
+    verifyColumnUpdate(updates.get(6), "cf7", "cq7", "", 7l, true, true, "");
+    verifyColumnUpdate(updates.get(7), "cf8", "cq8", "cv8", 8l, true, true, "");
     
   }
   
+  @Test
   public void testPutsString() {
     Mutation m = new Mutation(new Text("r1"));
     
@@ -207,17 +216,18 @@ public class MutationTest extends TestCase {
     assertEquals(8, m.size());
     assertEquals(8, updates.size());
     
-    assertEquals(updates.get(0), "cf1", "cq1", "", 0l, false, false, "v1");
-    assertEquals(updates.get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
-    assertEquals(updates.get(2), "cf3", "cq3", "", 3l, true, false, "v3");
-    assertEquals(updates.get(3), "cf4", "cq4", "cv4", 4l, true, false, "v4");
+    verifyColumnUpdate(updates.get(0), "cf1", "cq1", "", 0l, false, false, "v1");
+    verifyColumnUpdate(updates.get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
+    verifyColumnUpdate(updates.get(2), "cf3", "cq3", "", 3l, true, false, "v3");
+    verifyColumnUpdate(updates.get(3), "cf4", "cq4", "cv4", 4l, true, false, "v4");
     
-    assertEquals(updates.get(4), "cf5", "cq5", "", 0l, false, true, "");
-    assertEquals(updates.get(5), "cf6", "cq6", "cv6", 0l, false, true, "");
-    assertEquals(updates.get(6), "cf7", "cq7", "", 7l, true, true, "");
-    assertEquals(updates.get(7), "cf8", "cq8", "cv8", 8l, true, true, "");
+    verifyColumnUpdate(updates.get(4), "cf5", "cq5", "", 0l, false, true, "");
+    verifyColumnUpdate(updates.get(5), "cf6", "cq6", "cv6", 0l, false, true, "");
+    verifyColumnUpdate(updates.get(6), "cf7", "cq7", "", 7l, true, true, "");
+    verifyColumnUpdate(updates.get(7), "cf8", "cq8", "cv8", 8l, true, true, "");
   }
   
+  @Test
   public void testPutsStringString() {
     Mutation m = new Mutation(new Text("r1"));
     
@@ -238,15 +248,15 @@ public class MutationTest extends TestCase {
     assertEquals(8, m.size());
     assertEquals(8, updates.size());
     
-    assertEquals(updates.get(0), "cf1", "cq1", "", 0l, false, false, "v1");
-    assertEquals(updates.get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
-    assertEquals(updates.get(2), "cf3", "cq3", "", 3l, true, false, "v3");
-    assertEquals(updates.get(3), "cf4", "cq4", "cv4", 4l, true, false, "v4");
+    verifyColumnUpdate(updates.get(0), "cf1", "cq1", "", 0l, false, false, "v1");
+    verifyColumnUpdate(updates.get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
+    verifyColumnUpdate(updates.get(2), "cf3", "cq3", "", 3l, true, false, "v3");
+    verifyColumnUpdate(updates.get(3), "cf4", "cq4", "cv4", 4l, true, false, "v4");
     
-    assertEquals(updates.get(4), "cf5", "cq5", "", 0l, false, true, "");
-    assertEquals(updates.get(5), "cf6", "cq6", "cv6", 0l, false, true, "");
-    assertEquals(updates.get(6), "cf7", "cq7", "", 7l, true, true, "");
-    assertEquals(updates.get(7), "cf8", "cq8", "cv8", 8l, true, true, "");
+    verifyColumnUpdate(updates.get(4), "cf5", "cq5", "", 0l, false, true, "");
+    verifyColumnUpdate(updates.get(5), "cf6", "cq6", "cv6", 0l, false, true, "");
+    verifyColumnUpdate(updates.get(6), "cf7", "cq7", "", 7l, true, true, "");
+    verifyColumnUpdate(updates.get(7), "cf8", "cq8", "cv8", 8l, true, true, "");
   }
   
   /**
@@ -254,6 +264,7 @@ public class MutationTest extends TestCase {
    * the first set of column updates (and value lengths). Hadoop input formats reuse objects when reading, so if Mutations are used with an input format (or as
    * the input to a combiner or reducer) then they will be used in this fashion.
    */
+  @Test
   public void testMultipleReadFieldsCalls() throws IOException {
     // Create test mutations and write them to a byte output stream
     Mutation m1 = new Mutation("row1");
@@ -290,9 +301,9 @@ public class MutationTest extends TestCase {
     assertEquals(size1, m.size());
     assertEquals(nb1, m.numBytes());
     assertEquals(3, m.getUpdates().size());
-    assertEquals(m.getUpdates().get(0), "cf1.1", "cq1.1", "A|B", 0L, false, false, "val1.1");
-    assertEquals(m.getUpdates().get(1), "cf1.2", "cq1.2", "C|D", 0L, false, false, "val1.2");
-    assertEquals(m.getUpdates().get(2), "cf1.3", "cq1.3", "E|F", 0L, false, false, new String(val1_3));
+    verifyColumnUpdate(m.getUpdates().get(0), "cf1.1", "cq1.1", "A|B", 0L, false, false, "val1.1");
+    verifyColumnUpdate(m.getUpdates().get(1), "cf1.2", "cq1.2", "C|D", 0L, false, false, "val1.2");
+    verifyColumnUpdate(m.getUpdates().get(2), "cf1.3", "cq1.3", "E|F", 0L, false, false, new String(val1_3));
     
     // Reuse the same mutation object (which is what happens in the hadoop framework
     // when objects are read by an input format)
@@ -302,10 +313,10 @@ public class MutationTest extends TestCase {
     assertEquals(size2, m.size());
     assertEquals(nb2, m.numBytes());
     assertEquals(1, m.getUpdates().size());
-    assertEquals(m.getUpdates().get(0), "cf2", "cq2", "G|H", 1234L, true, false, new String(val2));
+    verifyColumnUpdate(m.getUpdates().get(0), "cf2", "cq2", "G|H", 1234L, true, false, new String(val2));
   }
   
-  private void assertEquals(ColumnUpdate cu, String cf, String cq, String cv, long ts, boolean timeSet, boolean deleted, String val) {
+  private void verifyColumnUpdate(ColumnUpdate cu, String cf, String cq, String cv, long ts, boolean timeSet, boolean deleted, String val) {
     
     assertEquals(cf, new String(cu.getColumnFamily()));
     assertEquals(cq, new String(cu.getColumnQualifier()));
@@ -317,6 +328,7 @@ public class MutationTest extends TestCase {
     assertEquals(val, new String(cu.getValue()));
   }
   
+  @Test
   public void test4() throws Exception {
     Mutation m1 = new Mutation(new Text("r1"));
     
@@ -343,11 +355,12 @@ public class MutationTest extends TestCase {
     assertEquals("r1", new String(m2.getRow()));
     assertEquals(2, m2.getUpdates().size());
     assertEquals(2, m2.size());
-    assertEquals(m2.getUpdates().get(0), "cf1", "cq1", "", 0l, false, false, "v1");
-    assertEquals(m2.getUpdates().get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
+    verifyColumnUpdate(m2.getUpdates().get(0), "cf1", "cq1", "", 0l, false, false, "v1");
+    verifyColumnUpdate(m2.getUpdates().get(1), "cf2", "cq2", "cv2", 0l, false, false, "v2");
     
   }
   
+  @Test
   public void testEquals() {
     Mutation m1 = new Mutation("r1");
     
@@ -416,4 +429,22 @@ public class MutationTest extends TestCase {
     assertFalse(m3.equals(m5));
     assertFalse(m4.equals(m5));
   }
+
+  @Test
+  public void testThrift() {
+    Mutation m1 = new Mutation("r1");
+    m1.put("cf1", "cq1", "v1");
+    TMutation tm1 = m1.toThrift();
+    Mutation m2 = new Mutation(tm1);
+    assertEquals(m1, m2);
+  }
+  @Test(expected=IllegalArgumentException.class)
+  public void testThrift_Invalid() {
+    Mutation m1 = new Mutation("r1");
+    m1.put("cf1", "cq1", "v1");
+    TMutation tm1 = m1.toThrift();
+    tm1.setRow((byte[]) null);
+    new Mutation(tm1);
+  }
+
 }