Skip to content

Commit

Permalink
Fix bug that allows circular links (#417)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@jtnelson
jtnelson committed Jun 5, 2020
1 parent 12cc8ae commit 99bc305
Show file tree
Hide file tree
Showing 5 changed files with 202 additions and 113 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
49 changes: 49 additions & 0 deletions ...urse-integration-tests/src/test/java/com/cinchapi/concourse/ReferentialIntegrityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<Long, Boolean> 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<Long, Boolean> 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));
}
}

}
151 changes: 62 additions & 89 deletions concourse-server/src/main/java/com/cinchapi/concourse/server/ConcourseServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -361,17 +334,29 @@ private static SortableTable<Set<TObject>> 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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}.
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 99bc305

Please sign in to comment.