From e129315a6e498fa8fa0a81ed96e50ed4ae98dff8 Mon Sep 17 00:00:00 2001 From: Robert Stupp Date: Tue, 21 Jan 2025 10:24:30 +0100 Subject: [PATCH] Fix backwards compatibility issues with Obj.referenced (#10218) While this is a real bug fix, the underlying issue does not affect data correctness. `Obj.referenced()` was introduced to track when a particular `Obj` was (last) written. This information is crucial when purging unreferenced data in Nessie's persistence backend database. The current code uses the sentinel value `0` for `Obj.reference()` for accidentally _two_ scenarios: to indicate that the value is not set and needs to be written with the actual timestamp and it is also `0` when old rows that do not have the `referenced` column set (aka `NULL`). This change updates the code to introduce `-1` for `referenced` as a sentinel for "absent" (NULL). There is also a bug in the implementations that prevents the "purge unreferenced data" to actually work for rows that do not have a value in the `referenced` column. This change fixes this. Unfortunately not all databases properly (DynamoDB and BigTable) support checking for the absence of a value. --- CHANGELOG.md | 3 ++ .../storage/bigtable/BigTablePersist.java | 34 +++++++++------ .../storage/cassandra/CassandraPersist.java | 39 +++++++++++++----- .../storage/cassandra2/Cassandra2Persist.java | 39 +++++++++++++----- .../commontests/AbstractBasePersistTests.java | 41 +++++++++++++++++++ .../versioned/storage/common/persist/Obj.java | 12 ++++++ .../storage/dynamodb/DynamoDBPersist.java | 29 ++++++++----- .../storage/dynamodb2/DynamoDB2Persist.java | 29 ++++++++----- .../storage/inmemory/InmemoryPersist.java | 6 ++- .../storage/jdbc/AbstractJdbcPersist.java | 21 ++++++++-- .../versioned/storage/jdbc/SqlConstants.java | 12 ++++++ .../storage/jdbc2/AbstractJdbc2Persist.java | 21 ++++++++-- .../versioned/storage/jdbc2/SqlConstants.java | 12 ++++++ .../storage/mongodb/MongoDBPersist.java | 18 ++++++-- .../storage/mongodb2/MongoDB2Persist.java | 18 ++++++-- .../storage/rocksdb/RocksDBPersist.java | 8 +++- 16 files changed, 272 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44e4071b40c..efdbc25be03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,9 @@ as necessary. Empty sections will not end in the release notes. ### Fixes +- Fix an issue that prevents the Nessie Server Admin tool to purge unreferenced data in the backend + database, for data being written before Nessie version 0.101.0. + ### Commits ## [0.101.3] Release (2024-12-18) diff --git a/versioned/storage/bigtable/src/main/java/org/projectnessie/versioned/storage/bigtable/BigTablePersist.java b/versioned/storage/bigtable/src/main/java/org/projectnessie/versioned/storage/bigtable/BigTablePersist.java index 2432cb22a2f..7848693a586 100644 --- a/versioned/storage/bigtable/src/main/java/org/projectnessie/versioned/storage/bigtable/BigTablePersist.java +++ b/versioned/storage/bigtable/src/main/java/org/projectnessie/versioned/storage/bigtable/BigTablePersist.java @@ -568,11 +568,18 @@ public void upsertObjs(@Nonnull Obj[] objs) throws ObjTooLargeException { @Override public boolean deleteWithReferenced(@Nonnull Obj obj) { - Filter condition = - FILTERS - .chain() - .filter(FILTERS.qualifier().exactMatch(QUALIFIER_OBJ_REFERENCED)) - .filter(FILTERS.value().exactMatch(copyFromUtf8(Long.toString(obj.referenced())))); + Filter condition; + if (obj.referenced() != -1L) { + condition = + FILTERS + .chain() + .filter(FILTERS.qualifier().exactMatch(QUALIFIER_OBJ_REFERENCED)) + .filter(FILTERS.value().exactMatch(copyFromUtf8(Long.toString(obj.referenced())))); + } else { + // We take a risk here in case the given object does _not_ have a referenced() value (old + // object). It's sadly not possible to check for the _absence_ of a cell. + condition = FILTERS.pass(); + } ConditionalRowMutation conditionalRowMutation = conditionalRowMutation(obj, condition, Mutation.create().deleteRow()); @@ -653,12 +660,15 @@ static > M objToMutation( } mutation .setCell(FAMILY_OBJS, QUALIFIER_OBJS, CELL_TIMESTAMP, unsafeWrap(serialized)) - .setCell(FAMILY_OBJS, QUALIFIER_OBJ_TYPE, CELL_TIMESTAMP, objTypeValue) - .setCell( - FAMILY_OBJS, - QUALIFIER_OBJ_REFERENCED, - CELL_TIMESTAMP, - copyFromUtf8(Long.toString(referenced))); + .setCell(FAMILY_OBJS, QUALIFIER_OBJ_TYPE, CELL_TIMESTAMP, objTypeValue); + if (obj.referenced() != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + mutation.setCell( + FAMILY_OBJS, + QUALIFIER_OBJ_REFERENCED, + CELL_TIMESTAMP, + copyFromUtf8(Long.toString(referenced))); + } UpdateableObj.extractVersionToken(obj) .map(ByteString::copyFromUtf8) .ifPresent(bs -> mutation.setCell(FAMILY_OBJS, QUALIFIER_OBJ_VERS, CELL_TIMESTAMP, bs)); @@ -756,7 +766,7 @@ private Obj objFromRow(Row row) { List objReferenced = row.getCells(FAMILY_OBJS, QUALIFIER_OBJ_REFERENCED); long referenced = objReferenced.isEmpty() - ? 0L + ? -1L : Long.parseLong(objReferenced.get(0).getValue().toStringUtf8()); List objCells = row.getCells(FAMILY_OBJS, QUALIFIER_OBJS); ByteBuffer obj = objCells.get(0).getValue().asReadOnlyByteBuffer(); diff --git a/versioned/storage/cassandra/src/main/java/org/projectnessie/versioned/storage/cassandra/CassandraPersist.java b/versioned/storage/cassandra/src/main/java/org/projectnessie/versioned/storage/cassandra/CassandraPersist.java index 7eeaa31b2ea..6bd8e63c3bb 100644 --- a/versioned/storage/cassandra/src/main/java/org/projectnessie/versioned/storage/cassandra/CassandraPersist.java +++ b/versioned/storage/cassandra/src/main/java/org/projectnessie/versioned/storage/cassandra/CassandraPersist.java @@ -285,7 +285,8 @@ public T[] fetchTypedObjsIfExist( } ObjId id = deserializeObjId(row.getString(COL_OBJ_ID.name())); String versionToken = row.getString(COL_OBJ_VERS.name()); - long referenced = row.getLong(COL_OBJ_REFERENCED.name()); + String colReferenced = COL_OBJ_REFERENCED.name(); + long referenced = row.isNull(colReferenced) ? -1 : row.getLong(colReferenced); @SuppressWarnings("unchecked") T typed = (T) @@ -345,13 +346,21 @@ public void upsertObjs(@Nonnull Obj[] objs) throws ObjTooLargeException { @Override public boolean deleteWithReferenced(@Nonnull Obj obj) { + long referenced = obj.referenced(); BoundStatement stmt = - backend.buildStatement( - DELETE_OBJ_REFERENCED, - false, - config.repositoryId(), - serializeObjId(obj.id()), - obj.referenced()); + referenced != -1L + ? backend.buildStatement( + DELETE_OBJ_REFERENCED, + false, + config.repositoryId(), + serializeObjId(obj.id()), + referenced) + // We take a risk here in case the given object does _not_ have a referenced() value + // (old object). + // Cassandra's conditional DELETE ... IF doesn't allow us to use "IF col IS NULL" or "IF + // (col = 0 OR col IS NULL)". + : backend.buildStatement( + DELETE_OBJ, false, config.repositoryId(), serializeObjId(obj.id())); return backend.executeCas(stmt); } @@ -391,8 +400,13 @@ public boolean updateConditional(@Nonnull UpdateableObj expected, @Nonnull Updat .setString(COL_OBJ_ID.name(), serializeObjId(id)) .setString(COL_OBJ_TYPE.name() + EXPECTED_SUFFIX, type.name()) .setString(COL_OBJ_VERS.name() + EXPECTED_SUFFIX, expectedVersion) - .setString(COL_OBJ_VERS.name(), newVersion) - .setLong(COL_OBJ_REFERENCED.name(), referenced); + .setString(COL_OBJ_VERS.name(), newVersion); + if (newValue.referenced() != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + stmt = stmt.setLong(COL_OBJ_REFERENCED.name(), referenced); + } else { + stmt = stmt.setToNull(COL_OBJ_REFERENCED.name()); + } serializer.serialize( newValue.withReferenced(referenced), @@ -531,9 +545,14 @@ private R writeSingleObj( .newBoundStatementBuilder(serializer.insertCql(upsert), upsert) .setString(COL_REPO_ID.name(), config.repositoryId()) .setString(COL_OBJ_ID.name(), serializeObjId(id)) - .setLong(COL_OBJ_REFERENCED.name(), referenced) .setString(COL_OBJ_TYPE.name(), type.name()) .setString(COL_OBJ_VERS.name(), versionToken); + if (obj.referenced() != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + stmt = stmt.setLong(COL_OBJ_REFERENCED.name(), referenced); + } else { + stmt = stmt.setToNull(COL_OBJ_REFERENCED.name()); + } serializer.serialize( obj, diff --git a/versioned/storage/cassandra2/src/main/java/org/projectnessie/versioned/storage/cassandra2/Cassandra2Persist.java b/versioned/storage/cassandra2/src/main/java/org/projectnessie/versioned/storage/cassandra2/Cassandra2Persist.java index f457a5baa0e..3b6a5f8373d 100644 --- a/versioned/storage/cassandra2/src/main/java/org/projectnessie/versioned/storage/cassandra2/Cassandra2Persist.java +++ b/versioned/storage/cassandra2/src/main/java/org/projectnessie/versioned/storage/cassandra2/Cassandra2Persist.java @@ -290,7 +290,8 @@ public T[] fetchTypedObjsIfExist( ObjId id = deserializeObjId(row.getByteBuffer(COL_OBJ_ID.name())); String versionToken = row.getString(COL_OBJ_VERS.name()); ByteBuffer serialized = row.getByteBuffer(COL_OBJ_VALUE.name()); - long referenced = row.getLong(COL_OBJ_REFERENCED.name()); + String colReferenced = COL_OBJ_REFERENCED.name(); + long referenced = row.isNull(colReferenced) ? -1 : row.getLong(colReferenced); return typeClass.cast(deserializeObj(id, referenced, serialized, versionToken)); }; @@ -345,13 +346,21 @@ public void upsertObjs(@Nonnull Obj[] objs) throws ObjTooLargeException { @Override public boolean deleteWithReferenced(@Nonnull Obj obj) { + var referenced = obj.referenced(); BoundStatement stmt = - backend.buildStatement( - DELETE_OBJ_REFERENCED, - false, - config.repositoryId(), - serializeObjId(obj.id()), - obj.referenced()); + referenced != -1L + ? backend.buildStatement( + DELETE_OBJ_REFERENCED, + false, + config.repositoryId(), + serializeObjId(obj.id()), + referenced) + // We take a risk here in case the given object does _not_ have a referenced() value + // (old object). + // Cassandra's conditional DELETE ... IF doesn't allow us to use "IF col IS NULL" or "IF + // (col = 0 OR col IS NULL)". + : backend.buildStatement( + DELETE_OBJ, false, config.repositoryId(), serializeObjId(obj.id())); return backend.executeCas(stmt); } @@ -397,8 +406,13 @@ public boolean updateConditional(@Nonnull UpdateableObj expected, @Nonnull Updat .setString(COL_OBJ_TYPE.name() + EXPECTED_SUFFIX, type.shortName()) .setString(COL_OBJ_VERS.name() + EXPECTED_SUFFIX, expectedVersion) .setString(COL_OBJ_VERS.name(), newVersion) - .setByteBuffer(COL_OBJ_VALUE.name(), ByteBuffer.wrap(serialized)) - .setLong(COL_OBJ_REFERENCED.name(), referenced); + .setByteBuffer(COL_OBJ_VALUE.name(), ByteBuffer.wrap(serialized)); + if (newValue.referenced() != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + stmt = stmt.setLong(COL_OBJ_REFERENCED.name(), referenced); + } else { + stmt = stmt.setToNull(COL_OBJ_REFERENCED.name()); + } return backend.executeCas(stmt.build()); } @@ -538,10 +552,15 @@ private R writeSingleObj( .newBoundStatementBuilder(upsert ? UPSERT_OBJ : STORE_OBJ, upsert) .setString(COL_REPO_ID.name(), config.repositoryId()) .setByteBuffer(COL_OBJ_ID.name(), serializeObjId(id)) - .setLong(COL_OBJ_REFERENCED.name(), referenced) .setString(COL_OBJ_TYPE.name(), type.shortName()) .setString(COL_OBJ_VERS.name(), versionToken) .setByteBuffer(COL_OBJ_VALUE.name(), ByteBuffer.wrap(serialized)); + if (obj.referenced() != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + stmt = stmt.setLong(COL_OBJ_REFERENCED.name(), referenced); + } else { + stmt = stmt.setToNull(COL_OBJ_REFERENCED.name()); + } return consumer.apply(stmt.build()); } diff --git a/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractBasePersistTests.java b/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractBasePersistTests.java index 314854ac794..955752663ce 100644 --- a/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractBasePersistTests.java +++ b/versioned/storage/common-tests/src/main/java/org/projectnessie/versioned/storage/commontests/AbstractBasePersistTests.java @@ -25,6 +25,7 @@ import static java.util.Objects.requireNonNull; import static java.util.UUID.randomUUID; import static org.assertj.core.api.Assumptions.assumeThat; +import static org.assertj.core.api.InstanceOfAssertFactories.LONG; import static org.assertj.core.api.InstanceOfAssertFactories.list; import static org.assertj.core.api.InstanceOfAssertFactories.type; import static org.junit.jupiter.params.provider.Arguments.arguments; @@ -140,6 +141,46 @@ public class AbstractBasePersistTests { @NessiePersist protected Persist persist; + @SuppressWarnings("DataFlowIssue") + @Test + public void deleteWithReferenced() throws Exception { + assumeThat(persist.isCaching()).isFalse(); + + // Do NOT use any batch store operation here - implementations are only adopted to "respect" the + // test-sentinel value -1 for exactly this .storeObj() signature! + + var objWithReferenced = SimpleTestObj.builder().id(randomObjId()).text("foo").build(); + persist.storeObj(objWithReferenced, true); + var readWithReferenced = persist.fetchObj(objWithReferenced.id()); + soft.assertThat(readWithReferenced).extracting(Obj::referenced, LONG).isGreaterThan(0); + soft.assertThat(persist.deleteWithReferenced(objWithReferenced)).isFalse(); + soft.assertThatCode(() -> persist.fetchObj(objWithReferenced.id())).doesNotThrowAnyException(); + soft.assertThat(persist.deleteWithReferenced(objWithReferenced.withReferenced(Long.MAX_VALUE))) + .isFalse(); + soft.assertThatCode(() -> persist.fetchObj(objWithReferenced.id())).doesNotThrowAnyException(); + soft.assertThat(persist.deleteWithReferenced(readWithReferenced)).isTrue(); + soft.assertThatCode(() -> persist.fetchObj(objWithReferenced.id())) + .isInstanceOf(ObjNotFoundException.class); + + var objWithoutReferenced1 = + SimpleTestObj.builder().referenced(-1L).id(randomObjId()).text("foo").build(); + persist.storeObj(objWithoutReferenced1, true); + var readWithoutReferenced1 = persist.fetchObj(objWithoutReferenced1.id()); + soft.assertThat(readWithoutReferenced1).extracting(Obj::referenced, LONG).isEqualTo(-1L); + soft.assertThat(persist.deleteWithReferenced(objWithoutReferenced1)).isTrue(); + soft.assertThatCode(() -> persist.fetchObj(objWithoutReferenced1.id())) + .isInstanceOf(ObjNotFoundException.class); + + var objWithoutReferenced2 = + SimpleTestObj.builder().referenced(-1L).id(randomObjId()).text("foo").build(); + persist.storeObj(objWithoutReferenced2, true); + var readWithoutReferenced2 = persist.fetchObj(objWithoutReferenced2.id()); + soft.assertThat(readWithoutReferenced2).extracting(Obj::referenced, LONG).isEqualTo(-1L); + soft.assertThat(persist.deleteWithReferenced(objWithoutReferenced2)).isTrue(); + soft.assertThatCode(() -> persist.fetchObj(objWithoutReferenced2.id())) + .isInstanceOf(ObjNotFoundException.class); + } + @ParameterizedTest @MethodSource public void genericObj( diff --git a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/persist/Obj.java b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/persist/Obj.java index 347241347b2..3beee8eb53d 100644 --- a/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/persist/Obj.java +++ b/versioned/storage/common/src/main/java/org/projectnessie/versioned/storage/common/persist/Obj.java @@ -39,6 +39,18 @@ public interface Obj { *

The value of this attribute is generated exclusively by the {@link Persist} implementations. * *

This attribute is not consistent when using a caching {@link Persist}. + * + *

When reading an object, this value is either {@code 0}, which means that the object + * was written using a Nessie version that did not have this attribute, or a (positive) timestamp + * when the object was written. + * + *

When storing an object, this value must be {@code 0}. The only one + * exception is for tests that exercise the relevant code paths - those tests do also use {@code + * -1} as a sentinel to write "NULL". + * + *

In any case it is illegal to refer to and/or interpret this attribute from code + * that does not have to deal explicitly with this value, only code that runs maintenance + * operations shall use this value. */ @JsonIgnore @JacksonInject(OBJ_REFERENCED_KEY) diff --git a/versioned/storage/dynamodb/src/main/java/org/projectnessie/versioned/storage/dynamodb/DynamoDBPersist.java b/versioned/storage/dynamodb/src/main/java/org/projectnessie/versioned/storage/dynamodb/DynamoDBPersist.java index 84222e5deca..6c0052ed643 100644 --- a/versioned/storage/dynamodb/src/main/java/org/projectnessie/versioned/storage/dynamodb/DynamoDBPersist.java +++ b/versioned/storage/dynamodb/src/main/java/org/projectnessie/versioned/storage/dynamodb/DynamoDBPersist.java @@ -91,6 +91,7 @@ import software.amazon.awssdk.services.dynamodb.model.BatchGetItemResponse; import software.amazon.awssdk.services.dynamodb.model.Condition; import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; +import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest; import software.amazon.awssdk.services.dynamodb.model.DynamoDbException; import software.amazon.awssdk.services.dynamodb.model.ExpectedAttributeValue; import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; @@ -549,16 +550,21 @@ public void upsertObjs(@Nonnull Obj[] objs) throws ObjTooLargeException { public boolean deleteWithReferenced(@Nonnull Obj obj) { ObjId id = obj.id(); - Map expectedValues = - Map.of( - COL_OBJ_REFERENCED, - ExpectedAttributeValue.builder().value(fromS(Long.toString(obj.referenced()))).build()); + var deleteItemRequest = + DeleteItemRequest.builder().tableName(backend.tableObjs).key(objKeyMap(id)); + if (obj.referenced() != -1L) { + // We take a risk here in case the given object does _not_ have a referenced() value + // (old object). It's not possible in DynamoDB to check for '== 0 OR IS ABSENT/NULL'. + deleteItemRequest.expected( + Map.of( + COL_OBJ_REFERENCED, + ExpectedAttributeValue.builder() + .value(fromS(Long.toString(obj.referenced()))) + .build())); + } try { - backend - .client() - .deleteItem( - b -> b.tableName(backend.tableObjs).key(objKeyMap(id)).expected(expectedValues)); + backend.client().deleteItem(deleteItemRequest.build()); return true; } catch (ConditionalCheckFailedException checkFailedException) { return false; @@ -663,7 +669,7 @@ private T itemToObj( Map inner = item.get(serializer.attributeName()).m(); String versionToken = attributeToString(item, COL_OBJ_VERS); String referencedString = attributeToString(item, COL_OBJ_REFERENCED); - long referenced = referencedString != null ? Long.parseLong(referencedString) : 0L; + long referenced = referencedString != null ? Long.parseLong(referencedString) : -1L; @SuppressWarnings("unchecked") T typed = (T) serializer.fromMap(id, type, referenced, inner, versionToken); return typed; @@ -680,7 +686,10 @@ private Map objToItem( Map inner = new HashMap<>(); item.put(KEY_NAME, objKey(id)); item.put(COL_OBJ_TYPE, fromS(type.shortName())); - item.put(COL_OBJ_REFERENCED, fromS(Long.toString(referenced))); + if (obj.referenced() != -1) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + item.put(COL_OBJ_REFERENCED, fromS(Long.toString(referenced))); + } UpdateableObj.extractVersionToken(obj).ifPresent(token -> item.put(COL_OBJ_VERS, fromS(token))); int incrementalIndexSizeLimit = ignoreSoftSizeRestrictions ? Integer.MAX_VALUE : effectiveIncrementalIndexSizeLimit(); diff --git a/versioned/storage/dynamodb2/src/main/java/org/projectnessie/versioned/storage/dynamodb2/DynamoDB2Persist.java b/versioned/storage/dynamodb2/src/main/java/org/projectnessie/versioned/storage/dynamodb2/DynamoDB2Persist.java index 490b509787c..b3fe229f3a1 100644 --- a/versioned/storage/dynamodb2/src/main/java/org/projectnessie/versioned/storage/dynamodb2/DynamoDB2Persist.java +++ b/versioned/storage/dynamodb2/src/main/java/org/projectnessie/versioned/storage/dynamodb2/DynamoDB2Persist.java @@ -92,6 +92,7 @@ import software.amazon.awssdk.services.dynamodb.model.BatchGetItemResponse; import software.amazon.awssdk.services.dynamodb.model.Condition; import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; +import software.amazon.awssdk.services.dynamodb.model.DeleteItemRequest; import software.amazon.awssdk.services.dynamodb.model.DynamoDbException; import software.amazon.awssdk.services.dynamodb.model.ExpectedAttributeValue; import software.amazon.awssdk.services.dynamodb.model.GetItemResponse; @@ -550,16 +551,21 @@ public void upsertObjs(@Nonnull Obj[] objs) throws ObjTooLargeException { public boolean deleteWithReferenced(@Nonnull Obj obj) { ObjId id = obj.id(); - Map expectedValues = - Map.of( - COL_OBJ_REFERENCED, - ExpectedAttributeValue.builder().value(fromS(Long.toString(obj.referenced()))).build()); + var deleteItemRequest = + DeleteItemRequest.builder().tableName(backend.tableObjs).key(objKeyMap(id)); + if (obj.referenced() != -1L) { + // We take a risk here in case the given object does _not_ have a referenced() value + // (old object). It's not possible in DynamoDB to check for '== 0 OR IS ABSENT/NULL'. + deleteItemRequest.expected( + Map.of( + COL_OBJ_REFERENCED, + ExpectedAttributeValue.builder() + .value(fromS(Long.toString(obj.referenced()))) + .build())); + } try { - backend - .client() - .deleteItem( - b -> b.tableName(backend.tableObjs).key(objKeyMap(id)).expected(expectedValues)); + backend.client().deleteItem(deleteItemRequest.build()); return true; } catch (ConditionalCheckFailedException checkFailedException) { return false; @@ -663,7 +669,7 @@ private T itemToObj( ByteBuffer bin = item.get(COL_OBJ_VALUE).b().asByteBuffer(); String versionToken = attributeToString(item, COL_OBJ_VERS); String referencedString = attributeToString(item, COL_OBJ_REFERENCED); - long referenced = referencedString != null ? Long.parseLong(referencedString) : 0L; + long referenced = referencedString != null ? Long.parseLong(referencedString) : -1L; Obj obj = deserializeObj(id, referenced, bin, versionToken); return typeClass.cast(obj); } @@ -677,7 +683,10 @@ private Map objToItem( Map item = new HashMap<>(); item.put(KEY_NAME, objKey(id)); item.put(COL_OBJ_TYPE, fromS(type.shortName())); - item.put(COL_OBJ_REFERENCED, fromS(Long.toString(referenced))); + if (obj.referenced() != -1) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + item.put(COL_OBJ_REFERENCED, fromS(Long.toString(referenced))); + } UpdateableObj.extractVersionToken(obj).ifPresent(token -> item.put(COL_OBJ_VERS, fromS(token))); int incrementalIndexSizeLimit = ignoreSoftSizeRestrictions ? Integer.MAX_VALUE : effectiveIncrementalIndexSizeLimit(); diff --git a/versioned/storage/inmemory/src/main/java/org/projectnessie/versioned/storage/inmemory/InmemoryPersist.java b/versioned/storage/inmemory/src/main/java/org/projectnessie/versioned/storage/inmemory/InmemoryPersist.java index 1199d65a506..0909c9044f6 100644 --- a/versioned/storage/inmemory/src/main/java/org/projectnessie/versioned/storage/inmemory/InmemoryPersist.java +++ b/versioned/storage/inmemory/src/main/java/org/projectnessie/versioned/storage/inmemory/InmemoryPersist.java @@ -229,7 +229,8 @@ public boolean storeObj(@Nonnull Obj obj, boolean ignoreSoftSizeRestrictions) verifySoftRestrictions(obj); } - long referenced = config.currentTimeMicros(); + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + long referenced = obj.referenced() != -1L ? config.currentTimeMicros() : -1L; Obj withReferenced = obj.withReferenced(referenced); AtomicBoolean r = new AtomicBoolean(false); @@ -296,7 +297,8 @@ public boolean deleteWithReferenced(@Nonnull Obj obj) { if (v == null) { // not present return null; - } else if (v.referenced() != obj.referenced()) { + } + if (v.referenced() != obj.referenced()) { return v; } result.set(true); diff --git a/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/AbstractJdbcPersist.java b/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/AbstractJdbcPersist.java index 43370d947ee..a8b9c9a4491 100644 --- a/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/AbstractJdbcPersist.java +++ b/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/AbstractJdbcPersist.java @@ -32,6 +32,7 @@ import static org.projectnessie.versioned.storage.jdbc.SqlConstants.DELETE_OBJ; import static org.projectnessie.versioned.storage.jdbc.SqlConstants.DELETE_OBJ_CONDITIONAL; import static org.projectnessie.versioned.storage.jdbc.SqlConstants.DELETE_OBJ_REFERENCED; +import static org.projectnessie.versioned.storage.jdbc.SqlConstants.DELETE_OBJ_REFERENCED_NULL; import static org.projectnessie.versioned.storage.jdbc.SqlConstants.FETCH_OBJ_TYPE; import static org.projectnessie.versioned.storage.jdbc.SqlConstants.FIND_OBJS; import static org.projectnessie.versioned.storage.jdbc.SqlConstants.FIND_OBJS_TYPED; @@ -414,6 +415,9 @@ private Obj deserializeObj(ResultSet rs) throws SQLException { String objType = rs.getString(COL_OBJ_TYPE); String versionToken = rs.getString(COL_OBJ_VERS); long referenced = rs.getLong(COL_OBJ_REFERENCED); + if (rs.wasNull()) { + referenced = -1; + } ObjType type = objTypeByName(objType); ObjSerializer serializer = ObjSerializers.forType(type); return serializer.deserialize(rs, type, id, referenced, versionToken); @@ -444,10 +448,16 @@ protected final Void updateObjs(@Nonnull Connection conn, @Nonnull Obj[] objs) } protected final boolean deleteWithReferenced(@Nonnull Connection conn, @Nonnull Obj obj) { - try (PreparedStatement ps = conn.prepareStatement(DELETE_OBJ_REFERENCED)) { + var referenced = obj.referenced(); + var referencedPresent = referenced != -1L; + try (PreparedStatement ps = + conn.prepareStatement( + referencedPresent ? DELETE_OBJ_REFERENCED : DELETE_OBJ_REFERENCED_NULL)) { ps.setString(1, config.repositoryId()); serializeObjId(ps, 2, obj.id(), databaseSpecific); - ps.setLong(3, obj.referenced()); + if (referencedPresent) { + ps.setLong(3, referenced); + } return ps.executeUpdate() == 1; } catch (SQLException e) { throw unhandledSQLException(e); @@ -561,7 +571,12 @@ private void upsertObjsWrite( } else { ps.setNull(storeObjSqlParams.get(COL_OBJ_VERS), Types.VARCHAR); } - ps.setLong(storeObjSqlParams.get(COL_OBJ_REFERENCED), referenced); + if (obj.referenced() == -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + ps.setNull(storeObjSqlParams.get(COL_OBJ_REFERENCED), Types.BIGINT); + } else { + ps.setLong(storeObjSqlParams.get(COL_OBJ_REFERENCED), referenced); + } ObjSerializer serializer = ObjSerializers.forType(type); serializer.serialize( diff --git a/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/SqlConstants.java b/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/SqlConstants.java index a5551398aab..5b5e186e4f5 100644 --- a/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/SqlConstants.java +++ b/versioned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/SqlConstants.java @@ -73,6 +73,18 @@ final class SqlConstants { + "=? AND " + COL_OBJ_REFERENCED + "=?"; + static final String DELETE_OBJ_REFERENCED_NULL = + "DELETE FROM " + + TABLE_OBJS + + " WHERE " + + COL_REPO_ID + + "=? AND " + + COL_OBJ_ID + + "=? AND (" + + COL_OBJ_REFERENCED + + " IS NULL OR " + + COL_OBJ_REFERENCED + + "=0)"; static final String COL_REFS_NAME = "ref_name"; static final String COL_REFS_POINTER = "pointer"; diff --git a/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/AbstractJdbc2Persist.java b/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/AbstractJdbc2Persist.java index fc521875199..b539b374fa3 100644 --- a/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/AbstractJdbc2Persist.java +++ b/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/AbstractJdbc2Persist.java @@ -29,6 +29,7 @@ import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.DELETE_OBJ; import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.DELETE_OBJ_CONDITIONAL; import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.DELETE_OBJ_REFERENCED; +import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.DELETE_OBJ_REFERENCED_NULL; import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.FETCH_OBJ_TYPE; import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.FIND_OBJS; import static org.projectnessie.versioned.storage.jdbc2.SqlConstants.FIND_OBJS_TYPED; @@ -379,6 +380,9 @@ private Obj deserializeObj(ResultSet rs) throws SQLException { String versionToken = rs.getString(COL_OBJ_VERS); byte[] serialized = rs.getBytes(COL_OBJ_VALUE); long referenced = rs.getLong(COL_OBJ_REFERENCED); + if (rs.wasNull()) { + referenced = -1; + } return ProtoSerialization.deserializeObj(id, referenced, serialized, versionToken); } @@ -407,10 +411,16 @@ protected final Void updateObjs(@Nonnull Connection conn, @Nonnull Obj[] objs) } protected final boolean deleteWithReferenced(@Nonnull Connection conn, @Nonnull Obj obj) { - try (PreparedStatement ps = conn.prepareStatement(DELETE_OBJ_REFERENCED)) { + var referenced = obj.referenced(); + var referencedPresent = referenced != -1L; + try (PreparedStatement ps = + conn.prepareStatement( + referencedPresent ? DELETE_OBJ_REFERENCED : DELETE_OBJ_REFERENCED_NULL)) { ps.setString(1, config.repositoryId()); serializeObjId(ps, 2, obj.id(), databaseSpecific); - ps.setLong(3, obj.referenced()); + if (referencedPresent) { + ps.setLong(3, referenced); + } return ps.executeUpdate() == 1; } catch (SQLException e) { throw unhandledSQLException(e); @@ -528,7 +538,12 @@ private void upsertObjsWrite( } byte[] serialized = serializeObj(obj, incrementalIndexSizeLimit, indexSizeLimit, false); ps.setBytes(5, serialized); - ps.setLong(6, referenced); + if (obj.referenced() == -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + ps.setNull(6, Types.BIGINT); + } else { + ps.setLong(6, referenced); + } batchIndexToObjIndex.put(batchIndex++, i); ps.addBatch(); diff --git a/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/SqlConstants.java b/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/SqlConstants.java index 303a651e4a9..4131e28f63d 100644 --- a/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/SqlConstants.java +++ b/versioned/storage/jdbc2/src/main/java/org/projectnessie/versioned/storage/jdbc2/SqlConstants.java @@ -67,6 +67,18 @@ final class SqlConstants { + "=? AND " + COL_OBJ_REFERENCED + "=?"; + static final String DELETE_OBJ_REFERENCED_NULL = + "DELETE FROM " + + TABLE_OBJS + + " WHERE " + + COL_REPO_ID + + "=? AND " + + COL_OBJ_ID + + "=? AND (" + + COL_OBJ_REFERENCED + + " IS NULL OR " + + COL_OBJ_REFERENCED + + "=0)"; static final String COL_REFS_NAME = "ref_name"; static final String COL_REFS_POINTER = "pointer"; diff --git a/versioned/storage/mongodb/src/main/java/org/projectnessie/versioned/storage/mongodb/MongoDBPersist.java b/versioned/storage/mongodb/src/main/java/org/projectnessie/versioned/storage/mongodb/MongoDBPersist.java index 7010484efac..803ace67d3e 100644 --- a/versioned/storage/mongodb/src/main/java/org/projectnessie/versioned/storage/mongodb/MongoDBPersist.java +++ b/versioned/storage/mongodb/src/main/java/org/projectnessie/versioned/storage/mongodb/MongoDBPersist.java @@ -62,6 +62,7 @@ import com.mongodb.bulk.BulkWriteUpsert; import com.mongodb.client.FindIterable; import com.mongodb.client.MongoCursor; +import com.mongodb.client.model.Filters; import com.mongodb.client.model.InsertOneModel; import com.mongodb.client.model.ReplaceOneModel; import com.mongodb.client.model.ReplaceOptions; @@ -668,10 +669,14 @@ public boolean deleteWithReferenced(@Nonnull Obj obj) { ObjId id = obj.id(); try { + var referencedBson = + obj.referenced() != -1L + ? eq(COL_OBJ_REFERENCED, obj.referenced()) + : Filters.or( + eq(COL_OBJ_REFERENCED, 0L), Filters.not(Filters.exists(COL_OBJ_REFERENCED))); return backend .objs() - .findOneAndDelete( - and(eq(ID_PROPERTY_NAME, idObjDoc(id)), eq(COL_OBJ_REFERENCED, obj.referenced()))) + .findOneAndDelete(and(eq(ID_PROPERTY_NAME, idObjDoc(id)), referencedBson)) != null; } catch (RuntimeException e) { throw unhandledException(e); @@ -764,7 +769,8 @@ private T docToObj( Document inner = doc.get(serializer.fieldName(), Document.class); String versionToken = doc.getString(COL_OBJ_VERS); Long referenced = doc.getLong(COL_OBJ_REFERENCED); - return serializer.docToObj(id, type, referenced != null ? referenced : 0L, inner, versionToken); + return serializer.docToObj( + id, type, referenced != null ? referenced : -1L, inner, versionToken); } private Document objToDoc(@Nonnull Obj obj, long referenced, boolean ignoreSoftSizeRestrictions) @@ -779,7 +785,11 @@ private Document objToDoc(@Nonnull Obj obj, long referenced, boolean ignoreSoftS Document inner = new Document(); doc.put(ID_PROPERTY_NAME, idObjDoc(id)); doc.put(COL_OBJ_TYPE, type.shortName()); - doc.put(COL_OBJ_REFERENCED, referenced); + var objReferenced = obj.referenced(); + if (objReferenced != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + doc.put(COL_OBJ_REFERENCED, referenced); + } UpdateableObj.extractVersionToken(obj).ifPresent(token -> doc.put(COL_OBJ_VERS, token)); int incrementalIndexSizeLimit = ignoreSoftSizeRestrictions ? Integer.MAX_VALUE : effectiveIncrementalIndexSizeLimit(); diff --git a/versioned/storage/mongodb2/src/main/java/org/projectnessie/versioned/storage/mongodb2/MongoDB2Persist.java b/versioned/storage/mongodb2/src/main/java/org/projectnessie/versioned/storage/mongodb2/MongoDB2Persist.java index c08ec748910..b803c28a7d6 100644 --- a/versioned/storage/mongodb2/src/main/java/org/projectnessie/versioned/storage/mongodb2/MongoDB2Persist.java +++ b/versioned/storage/mongodb2/src/main/java/org/projectnessie/versioned/storage/mongodb2/MongoDB2Persist.java @@ -65,6 +65,7 @@ import com.mongodb.bulk.BulkWriteUpsert; import com.mongodb.client.FindIterable; import com.mongodb.client.MongoCursor; +import com.mongodb.client.model.Filters; import com.mongodb.client.model.InsertOneModel; import com.mongodb.client.model.ReplaceOneModel; import com.mongodb.client.model.ReplaceOptions; @@ -670,10 +671,14 @@ public boolean deleteWithReferenced(@Nonnull Obj obj) { ObjId id = obj.id(); try { + var referencedBson = + obj.referenced() != -1L + ? eq(COL_OBJ_REFERENCED, obj.referenced()) + : Filters.or( + eq(COL_OBJ_REFERENCED, 0L), Filters.not(Filters.exists(COL_OBJ_REFERENCED))); return backend .objs() - .findOneAndDelete( - and(eq(ID_PROPERTY_NAME, idObjDoc(id)), eq(COL_OBJ_REFERENCED, obj.referenced()))) + .findOneAndDelete(and(eq(ID_PROPERTY_NAME, idObjDoc(id)), referencedBson)) != null; } catch (RuntimeException e) { throw unhandledException(e); @@ -764,7 +769,8 @@ private T docToObj( Binary bin = doc.get(COL_OBJ_VALUE, Binary.class); String versionToken = doc.getString(COL_OBJ_VERS); Long referenced = doc.getLong(COL_OBJ_REFERENCED); - Obj obj = deserializeObj(id, referenced != null ? referenced : 0L, bin.getData(), versionToken); + Obj obj = + deserializeObj(id, referenced != null ? referenced : -1L, bin.getData(), versionToken); @SuppressWarnings("unchecked") T r = (T) obj; return r; @@ -780,7 +786,11 @@ private Document objToDoc(@Nonnull Obj obj, long referenced, boolean ignoreSoftS Document doc = new Document(); doc.put(ID_PROPERTY_NAME, idObjDoc(id)); doc.put(COL_OBJ_TYPE, type.shortName()); - doc.put(COL_OBJ_REFERENCED, referenced); + var objReferenced = obj.referenced(); + if (objReferenced != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + doc.put(COL_OBJ_REFERENCED, referenced); + } UpdateableObj.extractVersionToken(obj).ifPresent(token -> doc.put(COL_OBJ_VERS, token)); int incrementalIndexSizeLimit = ignoreSoftSizeRestrictions ? Integer.MAX_VALUE : effectiveIncrementalIndexSizeLimit(); diff --git a/versioned/storage/rocksdb/src/main/java/org/projectnessie/versioned/storage/rocksdb/RocksDBPersist.java b/versioned/storage/rocksdb/src/main/java/org/projectnessie/versioned/storage/rocksdb/RocksDBPersist.java index 00bdeb828a7..5311f29e661 100644 --- a/versioned/storage/rocksdb/src/main/java/org/projectnessie/versioned/storage/rocksdb/RocksDBPersist.java +++ b/versioned/storage/rocksdb/src/main/java/org/projectnessie/versioned/storage/rocksdb/RocksDBPersist.java @@ -349,7 +349,9 @@ public boolean storeObj(@Nonnull Obj obj, boolean ignoreSoftSizeRestrictions) ignoreSoftSizeRestrictions = true; r = false; } else { - obj = obj.withReferenced(referenced); + var objReferenced = obj.referenced(); + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() + obj = obj.withReferenced(objReferenced != -1L ? referenced : -1L); r = true; } @@ -461,7 +463,9 @@ public boolean deleteWithReferenced(@Nonnull Obj obj) { if (!existing.type().equals(obj.type())) { return false; } - if (existing.referenced() != obj.referenced()) { + var referenced = obj.referenced(); + if (existing.referenced() != referenced && referenced != -1L) { + // -1 is a sentinel for AbstractBasePersistTests.deleteWithReferenced() return false; }