Avoid re-running previously validated access checks on commit retries. (

#7524)

This is mainly to speed up the retry cycle so as to improve
throughput and reduce individual commit times.

Authorization is not dependent on commit history.

Races between authorization data changes and commits were possible
even before this change, because there are no guarantees as to the
relative timing of AuthZ checks (even without retries) and finilizing
the commit.

servers/services/src/main/java/org/projectnessie/services/authz/RetriableAccessChecker.java

@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2023 Dremio
+ *
+ * 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 org.projectnessie.services.authz;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Map;
+import java.util.function.Supplier;
+
+/**
+ * A utility class for performing access check in contexts where operations may have to be re-tried
+ * due to optimistic locking or similar mechanisms. Each {@link #newAttempt() attempt} forms a new
+ * batch of access checks, but they are not re-validated on subsequent attempts unless the new batch
+ * of checks is different from the already validated one. The order of checks matters.
+ */
+public final class RetriableAccessChecker {
+  private final Supplier<BatchAccessChecker> validator;
+  private Collection<Check> validatedChecks;
+  private Map<Check, String> result;
+
+  public RetriableAccessChecker(Supplier<BatchAccessChecker> validator) {
+    Preconditions.checkNotNull(validator);
+    this.validator = validator;
+  }
+
+  public BatchAccessChecker newAttempt() {
+    return new Attempt();
+  }
+
+  private class Attempt extends AbstractBatchAccessChecker {
+    @Override
+    public Map<Check, String> check() {
+      // Shallow collection copy to ensure that we use what was current at the time of check
+      // in the equals call below (in case checks are added to this instance later, for whatever
+      // reason). Note that elements are immutable.
+      Collection<Check> currentChecks = new ArrayList<>(getChecks());
+
+      if (validatedChecks != null && result != null && validatedChecks.equals(currentChecks)) {
+        return result;
+      }
+
+      BatchAccessChecker checker = validator.get();
+      currentChecks.forEach(checker::can);
+      result = checker.check();
+      validatedChecks = currentChecks;
+      return result;
+    }
+  }
+}

servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java

@@ -101,6 +101,7 @@
 import org.projectnessie.services.authz.AuthzPaginationIterator;
 import org.projectnessie.services.authz.BatchAccessChecker;
 import org.projectnessie.services.authz.Check;
+import org.projectnessie.services.authz.RetriableAccessChecker;
 import org.projectnessie.services.cel.CELUtil;
 import org.projectnessie.services.config.ServerConfig;
 import org.projectnessie.services.hash.HashValidator;
@@ -1065,8 +1066,16 @@ public CommitResponse commitMultipleOperations(
   }
 
   private CommitValidator createCommitValidator(BranchName branchName) {
+    // Commits routinely run retries due to collisions on updating the HEAD of the branch.
+    // Authorization is not dependent on the commit history, only on the collection of access
+    // checks, which reflect the current commit. On retries, the commit data relevant to access
+    // checks almost never changes. Therefore, we use RetriableAccessChecker to avoid re-validating
+    // access checks (which could be a time-consuming operation) on subsequent retries, unless
+    // authorization input data changes.
+    RetriableAccessChecker accessChecker = new RetriableAccessChecker(this::startAccessCheck);
     return validation -> {
-      BatchAccessChecker check = startAccessCheck().canCommitChangeAgainstReference(branchName);
+      BatchAccessChecker check = accessChecker.newAttempt();
+      check.canCommitChangeAgainstReference(branchName);
       validation
           .operations()
           .forEach(

servers/services/src/test/java/org/projectnessie/services/authz/TestRetriableAccessChecker.java

@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2023 Dremio
+ *
+ * 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 org.projectnessie.services.authz;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.projectnessie.model.IdentifiedContentKey.IdentifiedElement.identifiedElement;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.junit.jupiter.api.Test;
+import org.projectnessie.model.Content;
+import org.projectnessie.model.ContentKey;
+import org.projectnessie.model.IdentifiedContentKey;
+import org.projectnessie.versioned.BranchName;
+
+class TestRetriableAccessChecker {
+
+  private int checkCount;
+  private final List<Check> checked = new ArrayList<>();
+  private final Map<Check, String> result = new HashMap<>();
+
+  private final Supplier<BatchAccessChecker> validator =
+      () ->
+          new AbstractBatchAccessChecker() {
+            @Override
+            public Map<Check, String> check() {
+              checkCount++;
+              checked.clear();
+              checked.addAll(getChecks());
+              return result;
+            }
+          };
+
+  @Test
+  void checkAndThrow() {
+    RetriableAccessChecker checker = new RetriableAccessChecker(validator);
+    Check check = Check.check(Check.CheckType.CREATE_ENTITY);
+    result.put(check, "test123");
+    assertThatThrownBy(() -> checker.newAttempt().can(check).checkAndThrow())
+        .isInstanceOf(AccessCheckException.class)
+        .hasMessage("test123");
+    assertThat(checked).containsExactly(check);
+    assertThat(checkCount).isEqualTo(1);
+  }
+
+  @Test
+  void repeatedCheck() {
+    RetriableAccessChecker checker = new RetriableAccessChecker(validator);
+    Check c1 = Check.check(Check.CheckType.CREATE_ENTITY);
+    Check c2 = Check.check(Check.CheckType.CREATE_REFERENCE);
+    assertThat(checker.newAttempt().can(c1).can(c2).check()).isEmpty();
+    assertThat(checked).containsExactly(c1, c2);
+    assertThat(checkCount).isEqualTo(1);
+
+    // repeating the same checks in same order does not re-trigger validation
+    assertThat(checker.newAttempt().can(c1).can(c2).check()).isEmpty();
+    assertThat(checked).containsExactly(c1, c2);
+    assertThat(checkCount).isEqualTo(1);
+
+    // repeating the same checks in different order triggers re-validation
+    assertThat(checker.newAttempt().can(c2).can(c1).check()).isEmpty();
+    assertThat(checked).containsExactly(c2, c1);
+    assertThat(checkCount).isEqualTo(2);
+  }
+
+  @Test
+  void dataChangeBetweenAttempts() {
+    IdentifiedContentKey.IdentifiedElement ns1 = identifiedElement("namespace", "id1");
+    IdentifiedContentKey.IdentifiedElement ns2 = identifiedElement("namespace", "id2");
+    IdentifiedContentKey.IdentifiedElement tableElement = identifiedElement("table", "id3");
+
+    IdentifiedContentKey t1 =
+        IdentifiedContentKey.builder()
+            .type(Content.Type.ICEBERG_TABLE)
+            .contentKey(ContentKey.of(ns1.element(), tableElement.element()))
+            .addElements(ns1, tableElement)
+            .build();
+
+    // Note: only the namespace ID is different between `t1` and `t2`
+    IdentifiedContentKey t2 =
+        IdentifiedContentKey.builder()
+            .type(Content.Type.ICEBERG_TABLE)
+            .contentKey(ContentKey.of(ns2.element(), tableElement.element()))
+            .addElements(ns2, tableElement)
+            .build();
+
+    RetriableAccessChecker checker = new RetriableAccessChecker(validator);
+    BranchName ref = BranchName.of("test");
+    assertThat(checker.newAttempt().canCreateEntity(ref, t1).check()).isEmpty();
+    assertThat(checked)
+        .containsExactly(Check.canViewReference(ref), Check.canCreateEntity(ref, t1));
+    assertThat(checkCount).isEqualTo(1);
+
+    // repeating the attempt with different checks' data results in re-validation
+    assertThat(checker.newAttempt().canCreateEntity(ref, t2).check()).isEmpty();
+    assertThat(checked)
+        .containsExactly(Check.canViewReference(ref), Check.canCreateEntity(ref, t2));
+    assertThat(checkCount).isEqualTo(2);
+  }
+}