Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist: enforce that null array elements are legal #7741

Merged
merged 1 commit into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -485,8 +485,10 @@ public void deleteObjs(@Nonnull @jakarta.annotation.Nonnull ObjId[] ids) {
try (Batcher<RowMutationEntry, Void> batcher =
backend.client().newBulkMutationBatcher(backend.tableObjs)) {
for (ObjId id : ids) {
ByteString key = dbKey(id);
batcher.add(RowMutationEntry.create(key).deleteRow());
if (id != null) {
ByteString key = dbKey(id);
batcher.add(RowMutationEntry.create(key).deleteRow());
}
}
} catch (ApiException e) {
throw apiException(e);
Expand Down Expand Up @@ -534,6 +536,9 @@ public void upsertObjs(@Nonnull @jakarta.annotation.Nonnull Obj[] objs)
try (Batcher<RowMutationEntry, Void> batcher =
backend.client().newBulkMutationBatcher(backend.tableObjs)) {
for (Obj obj : objs) {
if (obj == null) {
continue;
}
ObjId id = obj.id();
checkArgument(id != null, "Obj to store must have a non-null ID");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,20 @@ public void createObjectsWithUpsertThenFetchAndScan() throws Exception {
}
}

@Test
public void nullHandlingInArrays() throws ObjTooLargeException, ObjNotFoundException {
soft.assertThat(persist.storeObjs(new Obj[] {null})).containsExactly(false);
soft.assertThatCode(() -> persist.upsertObjs(new Obj[] {null})).doesNotThrowAnyException();
soft.assertThat(persist.fetchObjs(new ObjId[] {null})).containsExactly((Obj) null);
soft.assertThatCode(() -> persist.deleteObjs(new ObjId[] {null})).doesNotThrowAnyException();
soft.assertThat(persist.storeObjs(new Obj[] {null, null})).containsExactly(false, false);
soft.assertThatCode(() -> persist.upsertObjs(new Obj[] {null, null}))
.doesNotThrowAnyException();
soft.assertThat(persist.fetchObjs(new ObjId[] {null, null})).containsExactly(null, null);
soft.assertThatCode(() -> persist.deleteObjs(new ObjId[] {null, null}))
.doesNotThrowAnyException();
}

public static String randomContentId() {
return randomUUID().toString();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ boolean storeObj(@Nonnull @jakarta.annotation.Nonnull Obj obj, boolean ignoreSof
* <p>In case an object failed to be stored, it is undefined whether other objects have been
* stored or not.
*
* @param objs array with {@link Obj}s to store. {@code null} array elements are legal, the
* corresponding elements in the returned array will be {@code false}.
* @return an array with {@code boolean}s indicating whether the corresponding objects were
* created ({@code true}) or already present ({@code false}), see {@link #storeObj(Obj)}
* @throws ObjTooLargeException thrown when a hard database row/item size limit has been hit, or a
Expand All @@ -282,6 +284,8 @@ boolean storeObj(@Nonnull @jakarta.annotation.Nonnull Obj obj, boolean ignoreSof
*
* <p>In case an object failed to be deleted, it is undefined whether other objects have been
* deleted or not.
*
* @param ids array with {@link ObjId}s to delete. {@code null} array elements are legal.
*/
void deleteObjs(@Nonnull @jakarta.annotation.Nonnull ObjId[] ids);

Expand All @@ -302,6 +306,7 @@ boolean storeObj(@Nonnull @jakarta.annotation.Nonnull Obj obj, boolean ignoreSof
* <p>In case an object failed to be updated, it is undefined whether other objects have been
* updated or not.
*
* @param objs array with {@link Obj}s to upsert. {@code null} array elements are legal.
* @see #upsertObj( Obj)
*/
void upsertObjs(@Nonnull @jakarta.annotation.Nonnull Obj[] objs) throws ObjTooLargeException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,9 @@ public void deleteObj(@Nonnull @jakarta.annotation.Nonnull ObjId id) {
public void deleteObjs(@Nonnull @jakarta.annotation.Nonnull ObjId[] ids) {
try (BatchWrite batchWrite = new BatchWrite(backend, backend.tableObjs)) {
for (ObjId id : ids) {
batchWrite.addDelete(objKey(id));
if (id != null) {
batchWrite.addDelete(objKey(id));
}
}
}
}
Expand Down Expand Up @@ -571,12 +573,14 @@ public void upsertObjs(@Nonnull @jakarta.annotation.Nonnull Obj[] objs)
// DynamoDB does not support "PUT IF NOT EXISTS" in a BatchWriteItemRequest/PutItem
try (BatchWrite batchWrite = new BatchWrite(backend, backend.tableObjs)) {
for (Obj obj : objs) {
ObjId id = obj.id();
checkArgument(id != null, "Obj to store must have a non-null ID");
if (obj != null) {
ObjId id = obj.id();
checkArgument(id != null, "Obj to store must have a non-null ID");

Map<String, AttributeValue> item = objToItem(obj, id, false);
Map<String, AttributeValue> item = objToItem(obj, id, false);

batchWrite.addPut(item);
batchWrite.addPut(item);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,9 @@ public boolean[] storeObjs(@Nonnull @jakarta.annotation.Nonnull Obj[] objs)
throws ObjTooLargeException {
boolean[] r = new boolean[objs.length];
for (int i = 0; i < objs.length; i++) {
r[i] = storeObj(objs[i]);
if (objs[i] != null) {
r[i] = storeObj(objs[i]);
}
}
return r;
}
Expand All @@ -287,7 +289,9 @@ public void deleteObj(@Nonnull @jakarta.annotation.Nonnull ObjId id) {
@Override
public void deleteObjs(@Nonnull @jakarta.annotation.Nonnull ObjId[] ids) {
for (ObjId id : ids) {
deleteObj(id);
if (id != null) {
deleteObj(id);
}
}
}

Expand All @@ -301,7 +305,9 @@ public void upsertObj(@Nonnull @jakarta.annotation.Nonnull Obj obj) throws ObjTo
public void upsertObjs(@Nonnull @jakarta.annotation.Nonnull Obj[] objs)
throws ObjTooLargeException {
for (Obj obj : objs) {
upsertObj(obj);
if (obj != null) {
upsertObj(obj);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,8 @@ private boolean[] upsertObjs(
// Sadly an INSERT INTO ... ON CONFLICT DO UPDATE SET ... does not work with parameters in the
// UPDATE SET clause. Since the JDBC connection is configured with auto-commit=false, we can
// just DELETE the updates to be upserted and INSERT them again.
deleteObjs(conn, stream(objs).map(Obj::id).toArray(ObjId[]::new));
deleteObjs(
conn, stream(objs).map(obj -> obj == null ? null : obj.id()).toArray(ObjId[]::new));
}

try (PreparedStatement ps = conn.prepareStatement(databaseSpecific.wrapInsert(STORE_OBJ))) {
Expand Down Expand Up @@ -634,6 +635,9 @@ protected final void deleteObjs(
int batchSize = 0;

for (ObjId id : ids) {
if (id == null) {
continue;
}
ps.setString(1, config.repositoryId());
serializeObjId(ps, 2, id);
ps.addBatch();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Collections.singleton;
import static org.projectnessie.versioned.storage.common.persist.Reference.reference;
import static org.projectnessie.versioned.storage.rocksdb.RocksDBBackend.keyPrefix;
import static org.projectnessie.versioned.storage.rocksdb.RocksDBBackend.rocksDbException;
import static org.projectnessie.versioned.storage.serialize.ProtoSerialization.deserializeObj;
Expand Down Expand Up @@ -424,7 +423,9 @@ public void deleteObj(@Nonnull @jakarta.annotation.Nonnull ObjId id) {
@Override
public void deleteObjs(@Nonnull @jakarta.annotation.Nonnull ObjId[] ids) {
for (ObjId id : ids) {
deleteObj(id);
if (id != null) {
deleteObj(id);
}
}
}

Expand Down Expand Up @@ -455,7 +456,9 @@ public void upsertObj(@Nonnull @jakarta.annotation.Nonnull Obj obj) throws ObjTo
public void upsertObjs(@Nonnull @jakarta.annotation.Nonnull Obj[] objs)
throws ObjTooLargeException {
for (Obj obj : objs) {
upsertObj(obj);
if (obj != null) {
upsertObj(obj);
}
}
}

Expand Down