From 99bc30592356a3165ce69884c59359f6bc594d87 Mon Sep 17 00:00:00 2001 From: Jeff Nelson Date: Fri, 5 Jun 2020 18:02:14 -0400 Subject: [PATCH] Fix bug that allows circular links (#417) * add random unit test: * add failing cases * Move referential integrity checks to engine Move the checks for link validity to the engine to cover all corner cases and fix some bugs * update changelog * formatting * doc update * rename some things * rename * add another unit test * update changelog * formatting --- CHANGELOG.md | 3 +- .../concourse/ReferentialIntegrityTest.java | 49 ++++++ .../concourse/server/ConcourseServer.java | 151 +++++++----------- .../server/storage/BufferedStore.java | 75 ++++++--- .../ReferentialIntegrityException.java | 37 +++++ 5 files changed, 202 insertions(+), 113 deletions(-) create mode 100644 concourse-server/src/main/java/com/cinchapi/concourse/server/storage/ReferentialIntegrityException.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 98bd2fafec..1966ede76b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,8 @@ #### Version 0.10.5 (TBD) * Fixed a bug where sorting on a navigation key that isn't fetched (e.g. using a navigation key in a `find` operation or not specifying the navigation key as an operation key in a `get` or `select` operation), causes the results set to be returned in the incorrect order. -* Upgraded CCL version to `2.6.3` in order to fix a parsing bug that occurred when creating a `Criteria` containing a String or String-like value that contain a whitespace or equal sign (e.g. `=`) character. +* Upgraded CCL version to `2.6.3` in order to fix a parsing bug that occurred when creating a `Criteria` containing a String or String-like value with a whitespace or equal sign (e.g. `=`) character. +* Fixed a bug that made it possible to store circular links (e.g. a link from a record to itself) when atomically adding or setting data in multiple records at once. #### Version 0.10.4 (December 15, 2019) * Added support for using the `LIKE`, `NOT_LIKE` and `LINKS_TO` operators in the `TObject#is` methods. diff --git a/concourse-integration-tests/src/test/java/com/cinchapi/concourse/ReferentialIntegrityTest.java b/concourse-integration-tests/src/test/java/com/cinchapi/concourse/ReferentialIntegrityTest.java index bc564bc36b..7fca676a8a 100644 --- a/concourse-integration-tests/src/test/java/com/cinchapi/concourse/ReferentialIntegrityTest.java +++ b/concourse-integration-tests/src/test/java/com/cinchapi/concourse/ReferentialIntegrityTest.java @@ -15,10 +15,13 @@ */ package com.cinchapi.concourse; +import java.util.Map; + import org.junit.Assert; import org.junit.Test; import com.cinchapi.concourse.test.ConcourseIntegrationTest; +import com.google.common.collect.ImmutableSet; /** * Unit tests to verify referential integrity @@ -37,4 +40,50 @@ public void testCanAddNonCircularLinks() { Assert.assertTrue(client.link("foo", 2, 1)); } + @Test + public void testCannotManuallyAddCircularLinks() { + Assert.assertFalse(client.add("foo", Link.to(1), 1)); + } + + @Test + public void testCannotManuallyAddCircularLinksManyRecords() { + Map result = client.add("foo", Link.to(1), + ImmutableSet.of(1L, 2L)); + Assert.assertTrue(result.get(2L)); + Assert.assertFalse(result.get(1L)); + } + + @Test + public void testCannotAddCircularLinksManyRecords() { + Map result = client.link("foo", ImmutableSet.of(1L, 2L), + 1); + Assert.assertTrue(result.get(2L)); + Assert.assertFalse(result.get(1L)); + } + + @Test(expected = InvalidArgumentException.class) + public void testCannotManuallySetCircularLink() { + client.set("foo", Link.to(1), 1); + Assert.assertNull(client.get("foo", 1)); + } + + @Test(expected = InvalidArgumentException.class) + public void testCannotManuallySetCircularLinkMany() { + client.set("foo", Link.to(1), ImmutableSet.of(1L, 2L)); + Assert.assertNotNull(client.get("foo", 2)); + Assert.assertNull(client.get("foo", 1)); + } + + @Test + public void testTryingToSetCircularLinkHasNoEffect() { + client.add("foo", Link.to(2), 1); + try { + client.set("foo", Link.to(2), 2); + Assert.fail(); + } + catch (InvalidArgumentException e) { + Assert.assertEquals(Link.to(2), client.get("foo", 1)); + } + } + } diff --git a/concourse-server/src/main/java/com/cinchapi/concourse/server/ConcourseServer.java b/concourse-server/src/main/java/com/cinchapi/concourse/server/ConcourseServer.java index 33197ff087..1cb5300813 100644 --- a/concourse-server/src/main/java/com/cinchapi/concourse/server/ConcourseServer.java +++ b/concourse-server/src/main/java/com/cinchapi/concourse/server/ConcourseServer.java @@ -55,7 +55,6 @@ import com.cinchapi.common.base.Array; import com.cinchapi.common.reflect.Reflection; import com.cinchapi.concourse.Constants; -import com.cinchapi.concourse.Link; import com.cinchapi.concourse.Timestamp; import com.cinchapi.concourse.data.sort.SortableColumn; import com.cinchapi.concourse.data.sort.SortableSet; @@ -111,7 +110,6 @@ import com.cinchapi.concourse.thrift.TPage; import com.cinchapi.concourse.thrift.TransactionException; import com.cinchapi.concourse.thrift.TransactionToken; -import com.cinchapi.concourse.thrift.Type; import com.cinchapi.concourse.time.Time; import com.cinchapi.concourse.util.Convert; import com.cinchapi.concourse.util.Environments; @@ -150,31 +148,6 @@ public class ConcourseServer extends BaseConcourseServer implements * https://github.com/google/guice/wiki/AOP#limitations for more details. */ - /** - * Contains the credentials used by the {@link #users}. This file is - * typically located in the root of the server installation. - */ - private static final String ACCESS_FILE = ".access"; - - /** - * The minimum heap size required to run Concourse Server. - */ - private static final int MIN_HEAP_SIZE = 268435456; // 256 MB - - /** - * A placeholder to signfiy that no {@link Order} should be imposed on a - * result set. - */ - private static final TOrder NO_ORDER = null; - - /** - * The number of worker threads that Concourse Server uses. - */ - private static final int NUM_WORKER_THREADS = 100; // This may become - // configurable in a - // prefs file in a - // future release. - /** * Create a new {@link ConcourseServer} instance that uses the default port * and storage locations or those defined in the accessible @@ -361,17 +334,29 @@ private static SortableTable> emptySortableResultDatasetWithCapacit } /** - * Return {@code true} if adding {@code link} to {@code record} is valid. - * This method is used to enforce referential integrity (i.e. record cannot - * link to itself) before the data makes it way to the Engine. - * - * @param link - * @param record - * @return {@code true} if the link is valid + * Contains the credentials used by the {@link #users}. This file is + * typically located in the root of the server installation. */ - private static boolean isValidLink(Link link, long record) { - return link.longValue() != record; - } + private static final String ACCESS_FILE = ".access"; + + /** + * The minimum heap size required to run Concourse Server. + */ + private static final int MIN_HEAP_SIZE = 268435456; // 256 MB + + /** + * A placeholder to signfiy that no {@link Order} should be imposed on a + * result set. + */ + private static final TOrder NO_ORDER = null; + + /** + * The number of worker threads that Concourse Server uses. + */ + private static final int NUM_WORKER_THREADS = 100; // This may become + // configurable in a + // prefs file in a + // future release. /** * The base location where the indexed buffer pages are stored. @@ -466,14 +451,8 @@ public long addKeyValue(String key, TObject value, AccessToken creds, public boolean addKeyValueRecord(String key, TObject value, long record, AccessToken creds, TransactionToken transaction, String environment) throws TException { - if(value.getType() != Type.LINK - || isValidLink((Link) Convert.thriftToJava(value), record)) { - return ((BufferedStore) getStore(transaction, environment)).add(key, - value, record); - } - else { - return false; - } + return ((BufferedStore) getStore(transaction, environment)).add(key, + value, record); } @Override @@ -4754,14 +4733,8 @@ public void reconcileKeyRecordValues(String key, long record, public boolean removeKeyValueRecord(String key, TObject value, long record, AccessToken creds, TransactionToken transaction, String environment) throws TException { - if(value.getType() != Type.LINK - || isValidLink((Link) Convert.thriftToJava(value), record)) { - return ((BufferedStore) getStore(transaction, environment)) - .remove(key, value, record); - } - else { - return false; - } + return ((BufferedStore) getStore(transaction, environment)).remove(key, + value, record); } @Override @@ -6980,6 +6953,42 @@ public void verifyOrSet(String key, TObject value, long record, }); } + @Override + protected String getBufferStore() { + return bufferStore; + } + + @Override + protected String getDbStore() { + return dbStore; + } + + /** + * Return the {@link Engine} that is associated with {@code env}. If such an + * Engine does not exist, create a new one and add it to the collection. + * + * @param env + * @return the Engine + */ + protected Engine getEngine(String env) { + Engine engine = engines.get(env); + if(engine == null) { + env = Environments.sanitize(env); + return getEngineUnsafe(env); + } + return engine; + } + + @Override + protected PluginManager plugins() { + return pluginManager; + } + + @Override + protected UserService users() { + return users; + } + /** * Return the {@link Engine} that is associated with the * {@link Default#ENVIRONMENT}. @@ -7129,42 +7138,6 @@ private void validate(ByteBuffer username, ByteBuffer password) } } - @Override - protected String getBufferStore() { - return bufferStore; - } - - @Override - protected String getDbStore() { - return dbStore; - } - - /** - * Return the {@link Engine} that is associated with {@code env}. If such an - * Engine does not exist, create a new one and add it to the collection. - * - * @param env - * @return the Engine - */ - protected Engine getEngine(String env) { - Engine engine = engines.get(env); - if(engine == null) { - env = Environments.sanitize(env); - return getEngineUnsafe(env); - } - return engine; - } - - @Override - protected PluginManager plugins() { - return pluginManager; - } - - @Override - protected UserService users() { - return users; - } - /** * A {@link DeferredWrite} is a wrapper around a key, value, and record. * This is typically used by Concourse Server to gather certain writes diff --git a/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/BufferedStore.java b/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/BufferedStore.java index a03d4b90aa..6a41dc6025 100644 --- a/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/BufferedStore.java +++ b/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/BufferedStore.java @@ -20,13 +20,16 @@ import com.cinchapi.common.base.AnyStrings; import com.cinchapi.common.base.TernaryTruth; +import com.cinchapi.concourse.Link; import com.cinchapi.concourse.server.concurrent.LockService; import com.cinchapi.concourse.server.concurrent.RangeLockService; import com.cinchapi.concourse.server.storage.temp.Limbo; import com.cinchapi.concourse.server.storage.temp.Write; import com.cinchapi.concourse.thrift.Operator; import com.cinchapi.concourse.thrift.TObject; +import com.cinchapi.concourse.thrift.Type; import com.cinchapi.concourse.time.Time; +import com.cinchapi.concourse.util.Convert; import com.cinchapi.concourse.util.TSets; import com.cinchapi.concourse.validate.Keys; import com.google.common.collect.Sets; @@ -63,13 +66,14 @@ public abstract class BufferedStore extends BaseStore { // different code paths for present vs historical reads unlike Limbo. /** - * Perform validation on the {@code key} and {@code value} and throw an - * exception if necessary. + * Perform validation on a write that touches {@code key} as {@code value} + * in {@code record} and throw an exception if necessary. * * @param key * @param value */ - private static void validate(String key, TObject value) { // CON-21 + private static void ensureWriteIntegrity(String key, TObject value, + long record) { // CON-21 if(!Keys.isWritable(key)) { throw new IllegalArgumentException( AnyStrings.joinWithSpace(key, "is not a valid key")); @@ -78,6 +82,11 @@ else if(value.isBlank()) { throw new IllegalArgumentException( "Cannot use a blank value for " + key); } + else if(value.getType() == Type.LINK + && ((Link) Convert.thriftToJava(value)).longValue() == record) { + throw new ReferentialIntegrityException( + "A record cannot link to itself"); + } } /** @@ -240,12 +249,17 @@ public Set select(String key, long record, long timestamp) { * @param record */ public void set(String key, TObject value, long record) { - validate(key, value); - Set values = select(key, record); - for (TObject val : values) { - buffer.insert(Write.remove(key, val, record)); /* Authorized */ + try { + ensureWriteIntegrity(key, value, record); + Set values = select(key, record); + for (TObject val : values) { + buffer.insert(Write.remove(key, val, record)); /* Authorized */ + } + buffer.insert(Write.add(key, value, record)); /* Authorized */ + } + catch (ReferentialIntegrityException e) { + throw new IllegalArgumentException(e.getMessage()); } - buffer.insert(Write.add(key, value, record)); /* Authorized */ } @Override @@ -296,12 +310,17 @@ public boolean verify(String key, TObject value, long record, */ protected boolean add(String key, TObject value, long record, boolean sync, boolean doVerify, boolean lockOnVerify) { - validate(key, value); - Write write = Write.add(key, value, record); - if(!doVerify || !verify(write, lockOnVerify)) { - return buffer.insert(write, sync); /* Authorized */ + try { + ensureWriteIntegrity(key, value, record); + Write write = Write.add(key, value, record); + if(!doVerify || !verify(write, lockOnVerify)) { + return buffer.insert(write, sync); /* Authorized */ + } + return false; + } + catch (ReferentialIntegrityException e) { + return false; } - return false; } /** @@ -515,12 +534,17 @@ protected Map> doExplore(String key, Operator operator, */ protected boolean remove(String key, TObject value, long record, boolean sync, boolean doVerify, boolean lockOnVerify) { - validate(key, value); - Write write = Write.remove(key, value, record); - if(!doVerify || verify(write, lockOnVerify)) { - return buffer.insert(write, sync); /* Authorized */ + try { + ensureWriteIntegrity(key, value, record); + Write write = Write.remove(key, value, record); + if(!doVerify || verify(write, lockOnVerify)) { + return buffer.insert(write, sync); /* Authorized */ + } + return false; + } + catch (ReferentialIntegrityException e) { + return false; } - return false; } /** @@ -563,12 +587,17 @@ protected Set select(String key, long record, boolean lock) { */ protected void set(String key, TObject value, long record, boolean lockOnRead) { - validate(key, value); - Set values = select(key, record, lockOnRead); - for (TObject val : values) { - buffer.insert(Write.remove(key, val, record)); /* Authorized */ + try { + ensureWriteIntegrity(key, value, record); + Set values = select(key, record, lockOnRead); + for (TObject val : values) { + buffer.insert(Write.remove(key, val, record)); /* Authorized */ + } + buffer.insert(Write.add(key, value, record)); /* Authorized */ + } + catch (ReferentialIntegrityException e) { + throw new IllegalArgumentException(e.getMessage()); } - buffer.insert(Write.add(key, value, record)); /* Authorized */ } /** diff --git a/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/ReferentialIntegrityException.java b/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/ReferentialIntegrityException.java new file mode 100644 index 0000000000..c00bd41771 --- /dev/null +++ b/concourse-server/src/main/java/com/cinchapi/concourse/server/storage/ReferentialIntegrityException.java @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2013-2019 Cinchapi Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.cinchapi.concourse.server.storage; + +/** + * An internal exception that indicates that an action violates referential + * integrity. + * + * @author Jeff Nelson + */ +class ReferentialIntegrityException extends RuntimeException { + + private static final long serialVersionUID = 1921936211007486710L; + + /** + * Construct a new instance. + * + * @param message + */ + ReferentialIntegrityException(String message) { + super(message); + } + +}