Merge branch 'stable-3.5' into stable-3.6 * stable-3.5: Make sure that `owners` are JDK8 compatible in `stable-3.4` Extend 'GetFilesOwners' with Owners submit requirements Simplify `GetFilesOwners` Turn 'codeReviewLabelValue' to 'Stream' from 'Optional' Filter out the files owners based on the owners' code-review value Fix 'Redundant specification of type arguments <PathOwners>' warn Clean up GetFileOwners from unused variables Split owners integration tests into a separate build targets Follow whole chain of parents for OWNERS in submit requirements Avoid using Repository.resolve() when accessing exact ref-names Fix `OWNERS` file over-caching problem Follow the whole chain of parent projects for OWNERS Make code_review_user/1 a native plugin predicate Reuse the code_review_user Prolog rule across gerrit_owners.pl Change-Id: Ie499186cc5f7744e8f604798703a0788072ea34a
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java index 04afebd..901dbc8 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
@@ -17,7 +17,7 @@ import com.google.common.cache.RemovalNotification; import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.annotations.PluginName; -import com.google.gerrit.extensions.events.GitReferenceUpdatedListener; +import com.google.gerrit.extensions.events.GitBatchRefUpdateListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.cache.CacheRemovalListener; @@ -40,7 +40,7 @@ protected void configure() { cache(CACHE_NAME, Key.class, new TypeLiteral<Optional<OwnersConfig>>() {}); bind(PathOwnersEntriesCache.class).to(PathOwnersEntriesCacheImpl.class); - DynamicSet.bind(binder(), GitReferenceUpdatedListener.class) + DynamicSet.bind(binder(), GitBatchRefUpdateListener.class) .to(OwnersRefUpdateListener.class); DynamicSet.bind(binder(), CacheRemovalListener.class).to(OwnersCacheRemovalListener.class); } @@ -106,7 +106,7 @@ } @Singleton - class OwnersRefUpdateListener implements GitReferenceUpdatedListener { + class OwnersRefUpdateListener implements GitBatchRefUpdateListener { private final PathOwnersEntriesCache cache; private final String allUsersName; @@ -117,16 +117,17 @@ } @Override - public void onGitReferenceUpdated(Event event) { - if (supportedEvent(allUsersName, event)) { - cache.invalidate(event.getProjectName(), event.getRefName()); + public void onGitBatchRefUpdate(GitBatchRefUpdateListener.Event event) { + String projectName = event.getProjectName(); + if (!allUsersName.equals(projectName)) { + event.getUpdatedRefs().stream() + .filter(refUpdate -> supportedEvent(refUpdate.getRefName())) + .forEach(refUpdate -> cache.invalidate(projectName, refUpdate.getRefName())); } } - static boolean supportedEvent(String allUsersName, Event event) { - String refName = event.getRefName(); - return !allUsersName.equals(event.getProjectName()) - && (refName.equals(RefNames.REFS_CONFIG) || !RefNames.isGerritRef(refName)); + private boolean supportedEvent(String refName) { + return (refName.equals(RefNames.REFS_CONFIG) || !RefNames.isGerritRef(refName)); } }
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersBatchRefUpdateListenerTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersBatchRefUpdateListenerTest.java new file mode 100644 index 0000000..fe99ad3 --- /dev/null +++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersBatchRefUpdateListenerTest.java
@@ -0,0 +1,99 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// 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.googlesource.gerrit.owners.common; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.gerrit.entities.RefNames; +import com.google.gerrit.extensions.events.GitBatchRefUpdateListener; +import com.google.gerrit.server.config.AllProjectsNameProvider; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.AllUsersNameProvider; +import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache.OwnersRefUpdateListener; +import java.util.Arrays; +import java.util.Collection; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class OwnersBatchRefUpdateListenerTest { + @Parameterized.Parameters + public static Collection<Object[]> events() { + return Arrays.asList( + new Object[][] { + {mockEvent(ALL_USERS_NAME.get(), RefNames.REFS_CONFIG), 0}, + {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_CHANGES), 0}, + {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_SEQUENCES), 0}, + {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_CONFIG), 1}, + { + mockEvent( + AllProjectsNameProvider.DEFAULT, RefNames.REFS_CONFIG, RefNames.REFS_SEQUENCES), + 1 + }, + {mockEvent("foo", RefNames.fullName("bar")), 1}, + {mockEvent("foo", RefNames.REFS_CONFIG), 1}, + {mockEvent("foo", RefNames.REFS_CONFIG, RefNames.fullName("bar")), 2} + }); + } + + private static AllUsersName ALL_USERS_NAME = new AllUsersName(AllUsersNameProvider.DEFAULT); + + private final GitBatchRefUpdateListener.Event input; + private final int expectedTimes; + + public OwnersBatchRefUpdateListenerTest( + GitBatchRefUpdateListener.Event input, int expectedTimes) { + this.input = input; + this.expectedTimes = expectedTimes; + } + + @Test + public void shouldTriggerCacheInvalidationAccordingly() { + // given + PathOwnersEntriesCache cachMock = mock(PathOwnersEntriesCache.class); + OwnersRefUpdateListener listener = new OwnersRefUpdateListener(cachMock, ALL_USERS_NAME); + + // when + listener.onGitBatchRefUpdate(input); + + // then + verify(cachMock, times(expectedTimes)).invalidate(anyString(), anyString()); + } + + private static GitBatchRefUpdateListener.Event mockEvent(String project, String... refs) { + GitBatchRefUpdateListener.Event eventMock = mock(GitBatchRefUpdateListener.Event.class); + when(eventMock.getProjectName()).thenReturn(project); + Set<GitBatchRefUpdateListener.UpdatedRef> updatedRefs = + Stream.of(refs) + .map( + ref -> { + GitBatchRefUpdateListener.UpdatedRef updatedRef = + mock(GitBatchRefUpdateListener.UpdatedRef.class); + when(updatedRef.getRefName()).thenReturn(ref); + return updatedRef; + }) + .collect(Collectors.toSet()); + when(eventMock.getUpdatedRefs()).thenReturn(updatedRefs); + return eventMock; + } +}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersRefUpdateListenerTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersRefUpdateListenerTest.java deleted file mode 100644 index fa20c79..0000000 --- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/OwnersRefUpdateListenerTest.java +++ /dev/null
@@ -1,72 +0,0 @@ -// Copyright (C) 2023 The Android Open Source Project -// -// 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.googlesource.gerrit.owners.common; - -import static com.google.common.truth.Truth.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import com.google.gerrit.entities.RefNames; -import com.google.gerrit.extensions.events.GitReferenceUpdatedListener; -import com.google.gerrit.server.config.AllProjectsNameProvider; -import com.google.gerrit.server.config.AllUsersNameProvider; -import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache.OwnersRefUpdateListener; -import java.util.Arrays; -import java.util.Collection; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; - -@RunWith(Parameterized.class) -public class OwnersRefUpdateListenerTest { - @Parameterized.Parameters - public static Collection<Object[]> events() { - return Arrays.asList( - new Object[][] { - {mockEvent(ALL_USERS_NAME, null), false}, - {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_CHANGES), false}, - {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_SEQUENCES), false}, - {mockEvent(AllProjectsNameProvider.DEFAULT, RefNames.REFS_CONFIG), true}, - {mockEvent("foo", RefNames.fullName("bar")), true}, - {mockEvent("foo", RefNames.REFS_CONFIG), true} - }); - } - - private static String ALL_USERS_NAME = AllUsersNameProvider.DEFAULT; - - private final GitReferenceUpdatedListener.Event input; - private final boolean expected; - - public OwnersRefUpdateListenerTest(GitReferenceUpdatedListener.Event input, boolean expected) { - this.input = input; - this.expected = expected; - } - - @Test - public void shouldParseLabelDefinition() { - // when - boolean result = OwnersRefUpdateListener.supportedEvent(ALL_USERS_NAME, input); - - // then - assertThat(result).isEqualTo(expected); - } - - private static GitReferenceUpdatedListener.Event mockEvent(String project, String ref) { - GitReferenceUpdatedListener.Event eventMock = mock(GitReferenceUpdatedListener.Event.class); - when(eventMock.getProjectName()).thenReturn(project); - when(eventMock.getRefName()).thenReturn(ref); - return eventMock; - } -}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java index 0a2c5dd..3935a4b 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
@@ -37,6 +37,7 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.DiffNotAvailableException; import com.google.gerrit.server.patch.DiffOperations; +import com.google.gerrit.server.patch.DiffOptions; import com.google.gerrit.server.patch.filediff.FileDiffOutput; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; @@ -342,7 +343,8 @@ // For merge commits the default base is the auto-merge commit which should be used as base IOW // only the changes from it should be reviewed as changes against the parent 1 were already // reviewed - return diffOperations.listModifiedFilesAgainstParent(project, revision, 0); + return diffOperations.listModifiedFilesAgainstParent( + project, revision, 0, DiffOptions.DEFAULTS); } private static SubmitRecord notReady(String ownersLabel, String missingApprovals) {
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java index 9fdb4b9..55bfa3c 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java
@@ -32,11 +32,8 @@ import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.SubmitRequirementResultInfo; import com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status; -import com.google.gerrit.server.experiments.ExperimentFeaturesConstants; -import com.google.gerrit.testing.ConfigSuite; import com.googlesource.gerrit.owners.common.LabelDefinition; import java.util.Collection; -import org.eclipse.jgit.lib.Config; import org.junit.Before; import org.junit.Test; @@ -45,19 +42,6 @@ public class OwnersApprovalHasOperandIT extends OwnersSubmitRequirementITAbstract { private static final String REQUIREMENT_NAME = "Owner-Approval"; - // This configuration is needed on 3.5 only and should be removed during/after the merge to - // stable-3.6 as it is enabled there by default. - @ConfigSuite.Default - public static Config defaultConfig() { - Config cfg = new Config(); - cfg.setString( - "experiments", - null, - "enabled", - ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS); - return cfg; - } - @Before public void setup() throws Exception { configSubmitRequirement(
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java index 97113c0..bd6c232 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java
@@ -38,7 +38,6 @@ import com.google.gerrit.entities.Project; import com.googlesource.gerrit.owners.OwnersSubmitRequirement.LabelAndScore; import com.googlesource.gerrit.owners.common.LabelDefinition; -import java.sql.Timestamp; import java.time.Instant; import java.util.Arrays; import java.util.Collections; @@ -456,7 +455,7 @@ private static final PatchSetApproval approvedBy(Account.Id approving, String label, int value) { return PatchSetApproval.builder() .key(PatchSetApproval.key(mock(PatchSet.Id.class), approving, LabelId.create(label))) - .granted(Timestamp.from(Instant.now())) + .granted(Instant.now()) .realAccountId(approving) .value(value) .build();