Merge branch 'stable-3.5' into stable-3.6 * stable-3.5: Expire the owners cache after 1 minute and make it unlimited Optimize the 'path_owners_entries' cache eviction Introduce PathOwners cache from Ic7d61de07 Change-Id: I6ffad8525b95a5bdbce8122cd99f695bd798fd37
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java index d6fd574..9a8edb6 100644 --- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java +++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/GitRefListener.java
@@ -256,6 +256,8 @@ logger.warn("Could not open change: {}", cId, e); } catch (ReviewerManagerException e) { logger.warn("Could not add reviewers for change: {}", cId, e); + } catch (InvalidOwnersFileException e) { + logger.warn("Could not add reviewers for change: {} due to rules error", cId, e); } }
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java index 2c8c619..54c624e 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
@@ -37,24 +37,19 @@ this.accounts = accounts; } - public Optional<OwnersConfig> getOwnersConfig(byte[] yamlBytes) { - try { - final OwnersConfig ret = new OwnersConfig(); - JsonNode jsonNode = new ObjectMapper(new YAMLFactory()).readValue(yamlBytes, JsonNode.class); - Boolean inherited = - Optional.ofNullable(jsonNode.get("inherited")).map(JsonNode::asBoolean).orElse(true); - ret.setInherited(inherited); - ret.setLabel( - Optional.ofNullable(jsonNode.get("label")) - .map(JsonNode::asText) - .flatMap(LabelDefinition::parse)); - addClassicMatcher(jsonNode, ret); - addMatchers(jsonNode, ret); - return Optional.of(ret); - } catch (IOException e) { - log.warn("Unable to read YAML Owners file", e); - return Optional.empty(); - } + public OwnersConfig getOwnersConfig(byte[] yamlBytes) throws IOException { + final OwnersConfig ret = new OwnersConfig(); + JsonNode jsonNode = new ObjectMapper(new YAMLFactory()).readValue(yamlBytes, JsonNode.class); + Boolean inherited = + Optional.ofNullable(jsonNode.get("inherited")).map(JsonNode::asBoolean).orElse(true); + ret.setInherited(inherited); + ret.setLabel( + Optional.ofNullable(jsonNode.get("label")) + .map(JsonNode::asText) + .flatMap(LabelDefinition::parse)); + addClassicMatcher(jsonNode, ret); + addMatchers(jsonNode, ret); + return ret; } private void addClassicMatcher(JsonNode jsonNode, OwnersConfig ret) {
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/InvalidOwnersFileException.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/InvalidOwnersFileException.java new file mode 100644 index 0000000..1914625 --- /dev/null +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/InvalidOwnersFileException.java
@@ -0,0 +1,30 @@ +// Copyright (C) 2024 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; + +public class InvalidOwnersFileException extends Exception { + private static final long serialVersionUID = 1L; + + public InvalidOwnersFileException( + String project, String ownersPath, String branch, Throwable reason) { + super(exceptionMessage(project, ownersPath, branch), reason); + } + + private static String exceptionMessage(String project, String ownersPath, String branch) { + return String.format( + "Invalid owners file: %s, in project: %s, on branch %s", ownersPath, project, branch); + } +}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java index b904715..e7e437a 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -103,7 +103,8 @@ Map<String, FileDiffOutput> fileDiffMap, boolean expandGroups, String project, - PathOwnersEntriesCache cache) { + PathOwnersEntriesCache cache) + throws InvalidOwnersFileException { this( accounts, repositoryManager, @@ -125,7 +126,8 @@ DiffSummary diffSummary, boolean expandGroups, String project, - PathOwnersEntriesCache cache) { + PathOwnersEntriesCache cache) + throws InvalidOwnersFileException { this( accounts, repositoryManager, @@ -147,7 +149,8 @@ Set<String> modifiedPaths, boolean expandGroups, String project, - PathOwnersEntriesCache cache) { + PathOwnersEntriesCache cache) + throws InvalidOwnersFileException { this.repositoryManager = repositoryManager; this.repository = repository; this.parentProjectsNames = parentProjectsNames; @@ -156,10 +159,12 @@ this.accounts = accounts; this.expandGroups = expandGroups; - OwnersMap map = - branchWhenEnabled - .map(branch -> fetchOwners(project, branch, cache)) - .orElse(new OwnersMap()); + OwnersMap map; + if (branchWhenEnabled.isPresent()) { + map = fetchOwners(project, branchWhenEnabled.get(), cache); + } else { + map = new OwnersMap(); + } owners = Multimaps.unmodifiableSetMultimap(map.getPathOwners()); reviewers = Multimaps.unmodifiableSetMultimap(map.getPathReviewers()); matchers = map.getMatchers(); @@ -215,8 +220,10 @@ * Fetched the owners for the associated patch list. * * @return A structure containing matchers paths to owners + * @throws InvalidOwnersFileException when reading/parsing OWNERS file fails */ - private OwnersMap fetchOwners(String project, String branch, PathOwnersEntriesCache cache) { + private OwnersMap fetchOwners(String project, String branch, PathOwnersEntriesCache cache) + throws InvalidOwnersFileException { OwnersMap ownersMap = new OwnersMap(); try { // Using a `map` would have needed a try/catch inside the lamba, resulting in more code @@ -267,16 +274,17 @@ } } ownersMap.setLabel(Optional.ofNullable(currentEntry).flatMap(PathOwnersEntry::getLabel)); - return ownersMap; - } catch (IOException | ExecutionException e) { - log.warn("Invalid OWNERS file", e); - return ownersMap; + } catch (IOException e) { + log.warn("Opening repository {} failed", project, e); + } catch (ExecutionException e) { + log.warn("Reading OWNERS file for {} project, from cache failed", project, e); } + return ownersMap; } private List<ReadOnlyPathOwnersEntry> getPathOwnersEntries( List<Project.NameKey> projectNames, String branch, PathOwnersEntriesCache cache) - throws IOException, ExecutionException { + throws IOException, InvalidOwnersFileException, ExecutionException { ImmutableList.Builder<ReadOnlyPathOwnersEntry> pathOwnersEntries = ImmutableList.builder(); for (Project.NameKey projectName : projectNames) { try (Repository repo = repositoryManager.openRepository(projectName)) { @@ -290,7 +298,7 @@ private ReadOnlyPathOwnersEntry getPathOwnersEntryOrEmpty( String project, Repository repo, String branch, PathOwnersEntriesCache cache) - throws ExecutionException { + throws InvalidOwnersFileException, ExecutionException { return getPathOwnersEntry(project, repo, branch, cache) .map(v -> (ReadOnlyPathOwnersEntry) v) .orElse(PathOwnersEntry.EMPTY); @@ -298,27 +306,33 @@ private PathOwnersEntry getPathOwnersEntryOrNew( String project, Repository repo, String branch, PathOwnersEntriesCache cache) - throws ExecutionException { + throws InvalidOwnersFileException, ExecutionException { return getPathOwnersEntry(project, repo, branch, cache).orElseGet(PathOwnersEntry::new); } private Optional<PathOwnersEntry> getPathOwnersEntry( String project, Repository repo, String branch, PathOwnersEntriesCache cache) - throws ExecutionException { + throws InvalidOwnersFileException, ExecutionException { String rootPath = "OWNERS"; - return cache - .get(project, branch, rootPath, () -> getOwnersConfig(repo, rootPath, branch)) - .map( - conf -> - new PathOwnersEntry( + return unwrapInvalidOwnersFileException( + () -> + cache + .get( + project, + branch, rootPath, - conf, - accounts, - Optional.empty(), - Collections.emptySet(), - Collections.emptySet(), - Collections.emptySet(), - Collections.emptySet())); + () -> getOwnersConfig(project, repo, rootPath, branch)) + .map( + conf -> + new PathOwnersEntry( + rootPath, + conf, + accounts, + Optional.empty(), + Collections.emptySet(), + Collections.emptySet(), + Collections.emptySet(), + Collections.emptySet()))); } private void processMatcherPerPath( @@ -374,7 +388,7 @@ PathOwnersEntry rootEntry, Map<String, PathOwnersEntry> entries, PathOwnersEntriesCache cache) - throws ExecutionException { + throws InvalidOwnersFileException, ExecutionException { String[] parts = path.split("/"); PathOwnersEntry currentEntry = rootEntry; StringBuilder builder = new StringBuilder(); @@ -401,31 +415,33 @@ String ownersPath = partial + "OWNERS"; PathOwnersEntry pathFallbackEntry = currentEntry; currentEntry = - cache - .get( - project, - branch, - ownersPath, - () -> getOwnersConfig(repository, ownersPath, branch)) - .map( - c -> { - Optional<LabelDefinition> label = pathFallbackEntry.getLabel(); - final Set<Id> owners = pathFallbackEntry.getOwners(); - final Set<Id> reviewers = pathFallbackEntry.getReviewers(); - Collection<Matcher> inheritedMatchers = - pathFallbackEntry.getMatchers().values(); - Set<String> groupOwners = pathFallbackEntry.getGroupOwners(); - return new PathOwnersEntry( - ownersPath, - c, - accounts, - label, - owners, - reviewers, - inheritedMatchers, - groupOwners); - }) - .orElse(pathFallbackEntry); + unwrapInvalidOwnersFileException( + () -> + cache + .get( + project, + branch, + ownersPath, + () -> getOwnersConfig(project, repository, ownersPath, branch)) + .map( + c -> { + Optional<LabelDefinition> label = pathFallbackEntry.getLabel(); + final Set<Id> owners = pathFallbackEntry.getOwners(); + final Set<Id> reviewers = pathFallbackEntry.getReviewers(); + Collection<Matcher> inheritedMatchers = + pathFallbackEntry.getMatchers().values(); + Set<String> groupOwners = pathFallbackEntry.getGroupOwners(); + return new PathOwnersEntry( + ownersPath, + c, + accounts, + label, + owners, + reviewers, + inheritedMatchers, + groupOwners); + }) + .orElse(pathFallbackEntry)); entries.put(partial, currentEntry); } } @@ -480,12 +496,46 @@ /** * Returns the parsed FileOwnersConfig file for the given path if it exists. * - * @param ownersPath path to OWNERS file in the git repo - * @return config or null if it doesn't exist - * @throws IOException + * @return Optional(config) or Optional.empty if it doesn't exist + * @throws InvalidOwnersFileException when reading/parsing of the OWNERS file fails */ - private Optional<OwnersConfig> getOwnersConfig(Repository repo, String ownersPath, String branch) - throws IOException { - return getBlobAsBytes(repo, branch, ownersPath).flatMap(parser::getOwnersConfig); + private Optional<OwnersConfig> getOwnersConfig( + String project, Repository repo, String ownersPath, String branch) + throws InvalidOwnersFileException { + try { + Optional<byte[]> configBytes = getBlobAsBytes(repo, branch, ownersPath); + if (configBytes.isEmpty()) { + return Optional.empty(); + } + return Optional.of(parser.getOwnersConfig(configBytes.get())); + } catch (IOException e) { + throw new InvalidOwnersFileException(project, ownersPath, branch, e); + } + } + + @FunctionalInterface + private interface Executable<T> { + T call() throws ExecutionException; + } + + /** + * Unwraps the InvalidOwnersFileException from the ExecutionException so that callers can focus on + * handling InvalidOwnersFileException if/when needed. Note that ExecutionException is thrown only + * when loading values to PathOwnersEntriesCache fails. + * + * @throws InvalidOwnersFileException when call throws ExecutionException that is matchable with + * InvalidOwnersFileException + * @throws ExecutionException in all other cases + */ + private <T> T unwrapInvalidOwnersFileException(Executable<T> action) + throws InvalidOwnersFileException, ExecutionException { + try { + return action.call(); + } catch (ExecutionException e) { + if (e.getCause() instanceof InvalidOwnersFileException) { + throw (InvalidOwnersFileException) e.getCause(); + } + throw e; + } } }
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 cc482fe..aea33ff 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; @@ -43,7 +43,7 @@ .maximumWeight(Long.MAX_VALUE) .expireAfterWrite(Duration.ofSeconds(60)); 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); } @@ -109,7 +109,7 @@ } @Singleton - class OwnersRefUpdateListener implements GitReferenceUpdatedListener { + class OwnersRefUpdateListener implements GitBatchRefUpdateListener { private final PathOwnersEntriesCache cache; private final String allUsersName; @@ -120,16 +120,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/Config.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java index ae3fad2..e531a3f 100644 --- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java +++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/Config.java
@@ -85,7 +85,7 @@ return entry; } - Optional<OwnersConfig> getOwnersConfig(String string) { + OwnersConfig getOwnersConfig(String string) throws IOException { return parser.getOwnersConfig(string.getBytes(Charsets.UTF_8)); }
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-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java index 2e23f33..542ff6d 100644 --- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java +++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -339,14 +339,11 @@ } @Test - public void testParsingYamlWithLabelWithScore() { + public void testParsingYamlWithLabelWithScore() throws IOException { String yamlString = "inherited: true\nlabel: " + EXPECTED_LABEL + ",1\nowners:\n- " + USER_C_EMAIL_COM; - Optional<OwnersConfig> config = getOwnersConfig(yamlString); + OwnersConfig ownersConfig = getOwnersConfig(yamlString); - assertTrue(config.isPresent()); - - OwnersConfig ownersConfig = config.get(); assertTrue(ownersConfig.isInherited()); assertThat(ownersConfig.getLabel()).isPresent(); @@ -360,14 +357,11 @@ } @Test - public void testParsingYamlWithLabelWithoutScore() { + public void testParsingYamlWithLabelWithoutScore() throws IOException { String yamlString = "inherited: true\nlabel: " + EXPECTED_LABEL + "\nowners:\n- " + USER_C_EMAIL_COM; - Optional<OwnersConfig> config = getOwnersConfig(yamlString); + OwnersConfig ownersConfig = getOwnersConfig(yamlString); - assertTrue(config.isPresent()); - - OwnersConfig ownersConfig = config.get(); assertTrue(ownersConfig.isInherited()); assertThat(ownersConfig.getLabel()).isPresent();
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java index ac011b0..a035d93 100644 --- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java +++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
@@ -31,7 +31,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import org.junit.Before; @@ -85,11 +84,8 @@ regexMatcher(".*/a.*", ACCOUNT_D), partialRegexMatcher("Product.sql", ACCOUNT_A)); // the function to test - Optional<OwnersConfig> configNullable = getOwnersConfig(fullConfig); + OwnersConfig config = getOwnersConfig(fullConfig); // check classical configuration - assertThat(configNullable).isPresent(); - - OwnersConfig config = configNullable.get(); assertTrue(config.isInherited()); Set<String> owners = config.getOwners(); @@ -237,7 +233,7 @@ public void testMatchersOnlyConfig() throws Exception { replayAll(); - Optional<OwnersConfig> ownersConfigOpt = + OwnersConfig ownersConfig = getOwnersConfig( createConfig( false, @@ -245,9 +241,6 @@ suffixMatcher(".txt", ACCOUNT_B), genericMatcher(".*", ACCOUNT_A))); - assertThat(ownersConfigOpt).isPresent(); - OwnersConfig ownersConfig = ownersConfigOpt.get(); - assertThat(ownersConfig.getOwners()).isEmpty(); assertThat(ownersConfig.getMatchers()).isNotEmpty(); }
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java index bb2d474..4e8f3b0 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersApprovalHasPredicate.java
@@ -16,6 +16,7 @@ import com.google.gerrit.entities.SubmitRecord; import com.google.gerrit.extensions.annotations.PluginName; +import com.google.gerrit.server.project.SubmitRequirementEvaluationException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.SubmitRequirementPredicate; import com.google.gerrit.server.rules.SubmitRule; @@ -43,7 +44,15 @@ @Override public boolean match(ChangeData cd) { Optional<SubmitRecord> submitRecord = ownersSubmitRequirement.evaluate(cd); - return submitRecord.map(sr -> sr.status == SubmitRecord.Status.OK).orElse(true); + return submitRecord + .map( + sr -> { + if (sr.status == SubmitRecord.Status.RULE_ERROR) { + throw new SubmitRequirementEvaluationException(sr.errorMessage); + } + return sr.status == SubmitRecord.Status.OK; + }) + .orElse(true); } /**
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java index 7d2c4d5..be8a6ca 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersModule.java
@@ -40,13 +40,12 @@ .to(OwnerPredicateProvider.class) .asEagerSingleton(); install(new OwnersRestApiModule()); + install(new OwnersApprovalHasOperand.OwnerApprovalHasOperandModule()); if (pluginSettings.enableSubmitRequirement()) { install(new OwnersSubmitRequirement.OwnersSubmitRequirementModule()); - install(new OwnersApprovalHasOperand.OwnerApprovalHasOperandModule()); - } else { - logger.atInfo().log( - "OwnersSubmitRequirement is disabled therefore it will not be evaluated."); } + logger.atInfo().log( + "Global `owners.enableSubmitRequirement = %b`.", pluginSettings.enableSubmitRequirement()); } }
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java index c4d27e4..c077bb3 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersStoredValues.java
@@ -25,6 +25,7 @@ import com.google.gerrit.server.rules.StoredValues; import com.googlecode.prolog_cafe.lang.Prolog; import com.googlesource.gerrit.owners.common.Accounts; +import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; import com.googlesource.gerrit.owners.common.PathOwners; import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache; import com.googlesource.gerrit.owners.common.PluginSettings; @@ -73,6 +74,10 @@ settings.expandGroups(), projectState.getName(), cache); + } catch (InvalidOwnersFileException e) { + // re-throw exception as it is already logged but more importantly it is nicely + // handled by the prolog rules evaluator and results in prolog rule error + throw new IllegalStateException(e); } } };
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..0d8dac4 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; @@ -46,6 +47,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.owners.common.Accounts; +import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; import com.googlesource.gerrit.owners.common.LabelDefinition; import com.googlesource.gerrit.owners.common.PathOwners; import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache; @@ -164,6 +166,9 @@ String.format( "Missing approvals for path(s): [%s]", Joiner.on(", ").join(missingApprovals)))); + } catch (InvalidOwnersFileException e) { + logger.atSevere().withCause(e).log("Reading/parsing OWNERS file error."); + return Optional.of(ruleError(e.getMessage())); } catch (IOException e) { String msg = String.format( @@ -193,7 +198,7 @@ } private PathOwners getPathOwners(ChangeData cd, ProjectState projectState) - throws IOException, DiffNotAvailableException { + throws IOException, DiffNotAvailableException, InvalidOwnersFileException { metrics.countConfigLoads.increment(); try (Timer0.Context ctx = metrics.loadConfig.start()) { String branch = cd.change().getDest().branch(); @@ -342,7 +347,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) { @@ -363,4 +369,11 @@ submitRecord.requirements = List.of(SUBMIT_REQUIREMENT); return submitRecord; } + + private static SubmitRecord ruleError(String err) { + SubmitRecord submitRecord = new SubmitRecord(); + submitRecord.status = SubmitRecord.Status.RULE_ERROR; + submitRecord.errorMessage = err; + return submitRecord; + } }
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java index 5f5b1fb..a724239 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -39,6 +39,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.owners.common.Accounts; +import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; import com.googlesource.gerrit.owners.common.PathOwners; import com.googlesource.gerrit.owners.common.PathOwnersEntriesCache; import com.googlesource.gerrit.owners.common.PluginSettings; @@ -144,6 +145,9 @@ fileExpandedOwners.get(fileOwnerEntry.getKey()), ownersLabels, label)); return Response.ok(new FilesOwnersResponse(ownersLabels, filesWithPendingOwners)); + } catch (InvalidOwnersFileException e) { + logger.atSevere().withCause(e).log("Reading/parsing OWNERS file error."); + throw new ResourceConflictException(e.getMessage(), e); } }
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md index 5aaaeab..1d157c6 100644 --- a/owners/src/main/resources/Documentation/config.md +++ b/owners/src/main/resources/Documentation/config.md
@@ -1,40 +1,70 @@ +@PLUGIN@ configuration +====================== + ## Global configuration The global plugin configuration is read from the `$GERRIT_SITE/etc/owners.config` and is applied across all projects in Gerrit. owners.disable.branch -: List of branches regex where the resolution of owners is disabled. +: List of branches regex where the resolution of owners is disabled. -Example: + Example: -``` -[owners "disable"] - branch = refs/meta/config - branch = refs/heads/sandboxes.* -``` + ``` + [owners "disable"] + branch = refs/meta/config + branch = refs/heads/sandboxes.* + ``` owners.expandGroups : Expand owners and groups into account ids. If set to `false` all owners are left untouched, apart from e-mail addresses which have the domain dropped. Defaults to `true`. -Example: + Example: -``` -[owners] - expandGroups = false -``` + ``` + [owners] + expandGroups = false + ``` -owners.enableSubmitRequirement -: If set to `true` the approvals are evaluated through the owners submit rule without a need of - prolog predicate being added to a project. Defaults to `false`. +<a name="owners.enableSubmitRequirement">owners.enableSubmitRequirement</a> +: If set to `true` the approvals are evaluated through the owners plugin + provided submit rule without a need of prolog predicate being added to a + project or submit requirement configured in the `project.config` as it is + automatically applied to all projects. Defaults to `false`. -Example: + Example: -``` -[owners] - enableSubmitRequirement = true -``` + ``` + [owners] + enableSubmitRequirement = true + ``` + + > **Notes:** + > + > The `owners.enableSubmitRequirement = true` is a global + > setting and allows for quick site switch from `prolog` submit rule to + > plugin's provided submit rule. It is a drop-in replacement therefore, + > similarly to `prolog` rule, it cannot be overridden by Gerrit. In case + > when one-step migration is not feasible (e.g. when `prolog` rules need to + > be slowly phased out or when more control is needed over rule's + > applicability, submitability or ability to overide) one can configure + > submit requirement in `project.config` for a certain project (or + > hierarchy of projects), without turning it on globally, as + > `approval_owners` predicate is _always_ available. + > + > Please also note, that project's `rules.pl` should be removed in this + > case so that it doesn't interfere with a change evaluation. + > + > The minial configuration looks like below (see + > [submit requirements documentation](/Documentation/config-submit-requirements.html) for more details): + > ``` + > [submit-requirement "Owner-Approval"] + > description = Files needs to be approved by owners + > submittableIf = has:approval_owners + > ``` + cache."owners.path_owners_entries".memoryLimit : The cache is used to hold the parsed version of `OWNERS` files in the @@ -49,12 +79,12 @@ _Note that in opposite to the previous settings the modification needs to be performed in the `$GERRIT_SITE/etc/gerrit.config` file._ -Example + Example -``` -[cache "owners.path_owners_entries"] - memoryLimit = 2048 -``` + ``` + [cache "owners.path_owners_entries"] + memoryLimit = 2048 + ``` ## Configuration @@ -182,6 +212,9 @@ `Code-Review from owners` requirement is added making the change not submittable. +> See [notes](#owners.enableSubmitRequirement) for an example on how to enable +> submit requirements for a specific project only. + ### When `owners.enableSubmitRequirement = false` (default) And sample rules.pl that uses this predicate to enable the submit rule if @@ -255,6 +288,9 @@ As a result Gerrit would make the change Submittable only if 'John Doe' or 'Doug Smith' have provided at least a `Code-Review +1`. +> See [notes](#owners.enableSubmitRequirement) for an example on how to enable +> submit requirements for a specific project only. + ### When `owners.enableSubmitRequirement = false` (default) And a rule which makes submittable a change if at least one of the owners has @@ -315,6 +351,9 @@ A change cannot be submitted until 'John Doe' or 'Doug Smith' add a label `Owner-Approved`, independently from being able to provide any Code-Review. +> See [notes](#owners.enableSubmitRequirement) for an example on how to enable +> submit requirements for a specific project only. + ### When `owners.enableSubmitRequirement = false` (default) Finally, define prolog rules as shown in below (an amended version of @@ -364,6 +403,9 @@ `Code-Review+2` for a change to be submittable as default submit rule is still evaluated. +> See [notes](#owners.enableSubmitRequirement) for an example on how to enable +> submit requirements for a specific project only. + ## Example 5 - Owners based on matchers Often the ownership comes from the developer's skills and competencies and @@ -395,6 +437,9 @@ have provided their `Code-Review +2` feedback (as `Code-Review` is default label for owners submit requirement). +> See [notes](#owners.enableSubmitRequirement) for an example on how to enable +> submit requirements for a specific project only. + ### When `owners.enableSubmitRequirement = false` (default) And a rules.pl of:
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..7e057cd 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersApprovalHasOperandIT.java
@@ -16,6 +16,7 @@ package com.googlesource.gerrit.owners; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.ERROR; import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.SATISFIED; import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.UNSATISFIED; @@ -23,7 +24,8 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; -import com.google.gerrit.acceptance.config.GlobalPluginConfig; +import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.Project.NameKey; import com.google.gerrit.entities.SubmitRequirement; import com.google.gerrit.entities.SubmitRequirementExpression; import com.google.gerrit.extensions.api.changes.ChangeApi; @@ -32,11 +34,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,35 +44,12 @@ 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( - project, - SubmitRequirement.builder() - .setName(REQUIREMENT_NAME) - .setSubmittabilityExpression(SubmitRequirementExpression.create("has:approval_owners")) - .setAllowOverrideInChildProjects(false) - .build()); + enableSubmitRequirementsPredicateForProject(project); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldOwnersRequirementBeSatisfied() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2); @@ -81,12 +57,34 @@ PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(ListChangesOption.SUBMIT_REQUIREMENTS); - verifySubmitRequirements(changeNotReady.submitRequirements, REQUIREMENT_NAME, UNSATISFIED); + verifyChangeNotReady(changeNotReady); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.recommend()); ChangeInfo ownersVoteSufficient = forChange(r).get(ListChangesOption.SUBMIT_REQUIREMENTS); - verifySubmitRequirements(ownersVoteSufficient.submitRequirements, REQUIREMENT_NAME, SATISFIED); + verifyChangeReady(ownersVoteSufficient); + } + + @Override + protected void verifyChangeNotReady(ChangeInfo notReady) { + verifySubmitRequirements(notReady.submitRequirements, REQUIREMENT_NAME, UNSATISFIED); + } + + @Override + protected void verifyChangeReady(ChangeInfo ready) { + verifySubmitRequirements(ready.submitRequirements, REQUIREMENT_NAME, SATISFIED); + } + + @Override + protected void verifyRuleError(ChangeInfo change) { + verifySubmitRequirements(change.submitRequirements, REQUIREMENT_NAME, ERROR); + } + + @Override + protected void updateChildProjectConfiguration(NameKey childProject) throws Exception { + // submit requirement predicate has to be configured either on a project level or somewhere in + // the project hierarchy + enableSubmitRequirementsPredicateForProject(childProject); } private void verifySubmitRequirements( @@ -106,4 +104,15 @@ .map(r -> String.format("%s=%s", r.name, r.status)) .collect(toImmutableList()))); } + + private void enableSubmitRequirementsPredicateForProject(Project.NameKey forProject) + throws Exception { + configSubmitRequirement( + forProject, + SubmitRequirement.builder() + .setName(REQUIREMENT_NAME) + .setSubmittabilityExpression(SubmitRequirementExpression.create("has:approval_owners")) + .setAllowOverrideInChildProjects(false) + .build()); + } }
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersRegressionIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersRegressionIT.java new file mode 100644 index 0000000..2fc37ce --- /dev/null +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersRegressionIT.java
@@ -0,0 +1,115 @@ +// Copyright (C) 2024 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; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.SATISFIED; +import static com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status.UNSATISFIED; + +import com.google.gerrit.acceptance.LightweightPluginDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.TestPlugin; +import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.entities.RefNames; +import com.google.gerrit.extensions.api.changes.ChangeApi; +import com.google.gerrit.extensions.api.changes.ReviewInput; +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.extensions.restapi.RestApiException; +import java.util.Collection; +import org.junit.Test; + +@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule") +@UseLocalDisk +public class OwnersRegressionIT extends LightweightPluginDaemonTest { + private static final String GERRIT_SUBMIT_REQUIREMENT = "Code-Review"; + + @Test + public void + shouldNotAffectSubmittabilityWhenSubmitRequirementIsDisabledAndNotConfiguredForProject() + throws Exception { + // given that `.md` files are owned by `user1` + TestAccount user1 = accountCreator.user1(); + addOwnerFileWithMatchersToRoot(true, ".md", user1); + + // when + PushOneCommit.Result r = createChange("Add a file", "README.md", "foo"); + + // then + ChangeApi changeApi = forChange(r); + ChangeInfo changeNotReady = changeApi.get(); + assertThat(changeWithNoRequirementsIsSubmittable(changeNotReady)).isFalse(); + verifySubmitRequirement( + changeNotReady.submitRequirements, GERRIT_SUBMIT_REQUIREMENT, UNSATISFIED); + + // and when + changeApi.current().review(ReviewInput.approve()); + + // then + ChangeInfo changeReady = forChange(r).get(); + assertThat(changeWithNoRequirementsIsSubmittable(changeReady)).isTrue(); + verifySubmitRequirement(changeReady.submitRequirements, GERRIT_SUBMIT_REQUIREMENT, SATISFIED); + } + + private boolean changeWithNoRequirementsIsSubmittable(ChangeInfo change) { + return change.requirements.isEmpty() && change.submittable; + } + + private void verifySubmitRequirement( + Collection<SubmitRequirementResultInfo> requirements, String name, Status status) { + assertThat(requirements).hasSize(1); + + SubmitRequirementResultInfo requirement = requirements.iterator().next(); + if (!requirement.name.equals(name) || !(requirement.status == status)) { + throw new AssertionError( + String.format( + "No submit requirement %s with status %s (existing = %s)", + name, + status, + requirements.stream() + .map(r -> String.format("%s=%s", r.name, r.status)) + .collect(toImmutableList()))); + } + } + + private ChangeApi forChange(PushOneCommit.Result r) throws RestApiException { + return gApi.changes().id(r.getChangeId()); + } + + private void addOwnerFileWithMatchersToRoot(boolean inherit, String extension, TestAccount owner) + throws Exception { + // Add OWNERS file to root: + // + // inherited: true + // matchers: + // - suffix: extension + // owners: + // - u1.email() + pushFactory + .create( + admin.newIdent(), + testRepo, + "Add OWNER file", + "OWNERS", + String.format( + "inherited: %s\nmatchers:\n" + "- suffix: %s\n owners:\n%s", + inherit, extension, String.format(" - %s\n", owner.email()))) + .to(RefNames.fullName("master")) + .assertOkStatus(); + } +}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java index 0d5c2c5..0e7e40b 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
@@ -15,9 +15,64 @@ package com.googlesource.gerrit.owners; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; + import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.entities.Project.NameKey; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo; +import com.google.gerrit.extensions.common.SubmitRecordInfo; +import com.google.gerrit.server.config.SitePaths; +import com.google.inject.Inject; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.util.FS; @TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule") @UseLocalDisk -public class OwnersSubmitRequirementIT extends OwnersSubmitRequirementITAbstract {} +public class OwnersSubmitRequirementIT extends OwnersSubmitRequirementITAbstract { + private static final LegacySubmitRequirementInfo NOT_READY = + new LegacySubmitRequirementInfo("NOT_READY", "Owners", "owners"); + private static final LegacySubmitRequirementInfo READY = + new LegacySubmitRequirementInfo("OK", "Owners", "owners"); + + @Inject private SitePaths sitePaths; + + @Override + public void setUpTestPlugin() throws Exception { + // ensure that `owners.enableSubmitRequirements = true` for each integration test without + // relying on `GlobalPluginConfig` annotation + FileBasedConfig ownersConfig = + new FileBasedConfig(sitePaths.etc_dir.resolve("owners.config").toFile(), FS.DETECTED); + ownersConfig.setBoolean("owners", null, "enableSubmitRequirement", true); + ownersConfig.save(); + super.setUpTestPlugin(); + } + + @Override + protected void verifyChangeNotReady(ChangeInfo notReady) { + assertThat(notReady.requirements).containsExactly(NOT_READY); + } + + @Override + protected void verifyChangeReady(ChangeInfo ready) { + assertThat(ready.requirements).containsExactly(READY); + } + + @Override + protected void verifyRuleError(ChangeInfo change) { + assertThat( + change.submitRecords.stream() + .filter(record -> SubmitRecordInfo.Status.RULE_ERROR == record.status) + .filter(record -> record.errorMessage.startsWith("Invalid owners file: OWNERS")) + .findAny()) + .isPresent(); + } + + @Override + protected void updateChildProjectConfiguration(NameKey childProject) { + // there is no need to further customize project when `owners.enableSubmitRequirements = true` + // as it is globally enabled + } +}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java index d7cea60..2135b40 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementITAbstract.java
@@ -27,7 +27,6 @@ import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; -import com.google.gerrit.acceptance.config.GlobalPluginConfig; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.LabelFunction; @@ -39,7 +38,6 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ChangeInfo; -import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo; import com.google.gerrit.extensions.common.SubmitRecordInfo; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.project.testing.TestLabels; @@ -52,19 +50,10 @@ import org.junit.Test; abstract class OwnersSubmitRequirementITAbstract extends LightweightPluginDaemonTest { - private static final LegacySubmitRequirementInfo NOT_READY = - new LegacySubmitRequirementInfo("NOT_READY", "Owners", "owners"); - private static final LegacySubmitRequirementInfo READY = - new LegacySubmitRequirementInfo("OK", "Owners", "owners"); - @Inject protected RequestScopeOperations requestScopeOperations; @Inject private ProjectOperations projectOperations; @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireAtLeastOneApprovalForMatchingPathFromOwner() throws Exception { TestAccount admin2 = accountCreator.admin2(); TestAccount user1 = accountCreator.user1(); @@ -74,25 +63,21 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); changeApi.current().review(ReviewInput.approve()); ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get(); assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse(); - assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReadyAfterSelfApproval); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.approve()); ChangeInfo changeReady = forChange(r).get(); assertThat(changeReady.submittable).isTrue(); - assertThat(changeReady.requirements).containsExactly(READY); + verifyChangeReady(changeReady); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldNotRequireApprovalForNotMatchingPath() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileWithMatchersToRoot(true, ".md", admin2); @@ -110,10 +95,6 @@ } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireApprovalFromRootOwner() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileToRoot(true, admin2); @@ -122,25 +103,21 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); changeApi.current().review(ReviewInput.approve()); ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get(); assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse(); - assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReadyAfterSelfApproval); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.approve()); ChangeInfo changeReady = forChange(r).get(); assertThat(changeReady.submittable).isTrue(); - assertThat(changeReady.requirements).containsExactly(READY); + verifyChangeReady(changeReady); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldBlockOwnersApprovalForMaxNegativeVote() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileToRoot(true, admin2); @@ -149,23 +126,19 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.approve()); ChangeInfo changeReady = forChange(r).get(); assertThat(changeReady.submittable).isTrue(); - assertThat(changeReady.requirements).containsExactly(READY); + verifyChangeReady(changeReady); changeApi.current().review(ReviewInput.reject()); assertThat(forChange(r).get().submittable).isFalse(); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireVerifiedApprovalEvenIfCodeOwnerApproved() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileToRoot(true, admin2); @@ -175,12 +148,12 @@ PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); ChangeApi changeApi = forChange(r); assertThat(changeApi.get().submittable).isFalse(); - assertThat(changeApi.get().requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeApi.get()); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.approve()); assertThat(forChange(r).get().submittable).isFalse(); - assertThat(forChange(r).get().requirements).containsExactly(READY); + verifyChangeReady(forChange(r).get()); verifyHasSubmitRecord( forChange(r).get().submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.NEED); @@ -191,10 +164,6 @@ } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireCodeOwnerApprovalEvenIfVerifiedWasApproved() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileToRoot(true, admin2); @@ -204,27 +173,23 @@ PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); ChangeApi changeApi = forChange(r); assertThat(changeApi.get().submittable).isFalse(); - assertThat(changeApi.get().requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeApi.get()); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(new ReviewInput().label(LabelId.VERIFIED, 1)); ChangeInfo changeNotReady = forChange(r).get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); verifyHasSubmitRecord( changeNotReady.submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.OK); forChange(r).current().review(ReviewInput.approve()); ChangeInfo changeReady = forChange(r).get(); assertThat(changeReady.submittable).isTrue(); - assertThat(changeReady.requirements).containsExactly(READY); + verifyChangeReady(changeReady); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireConfiguredLabelByCodeOwner() throws Exception { TestAccount admin2 = accountCreator.admin2(); String labelId = "Foo"; @@ -235,26 +200,22 @@ PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); ChangeApi changeApi = forChange(r); assertThat(changeApi.get().submittable).isFalse(); - assertThat(changeApi.get().requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeApi.get()); changeApi.current().review(ReviewInput.approve()); ChangeInfo changeStillNotReady = changeApi.get(); assertThat(changeStillNotReady.submittable).isFalse(); - assertThat(changeStillNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeStillNotReady); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(new ReviewInput().label(labelId, 1)); ChangeInfo changeReady = forChange(r).get(); assertThat(changeReady.submittable).isTrue(); - assertThat(changeReady.requirements).containsExactly(READY); + verifyChangeReady(changeReady); verifyHasSubmitRecord(changeReady.submitRecords, labelId, SubmitRecordInfo.Label.Status.OK); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireConfiguredLabelByCodeOwnerEvenIfItIsNotConfiguredForProject() throws Exception { TestAccount admin2 = accountCreator.admin2(); @@ -264,19 +225,15 @@ PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); ChangeApi changeApi = forChange(r); assertThat(changeApi.get().submittable).isFalse(); - assertThat(changeApi.get().requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeApi.get()); changeApi.current().review(ReviewInput.approve()); ChangeInfo changeStillNotReady = changeApi.get(); assertThat(changeStillNotReady.submittable).isFalse(); - assertThat(changeStillNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeStillNotReady); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireConfiguredLabelScoreByCodeOwner() throws Exception { TestAccount admin2 = accountCreator.admin2(); addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2); @@ -285,25 +242,21 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); changeApi.current().review(ReviewInput.approve()); ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get(); assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse(); - assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReadyAfterSelfApproval); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.recommend()); ChangeInfo changeReadyWithOwnerScore = forChange(r).get(); assertThat(changeReadyWithOwnerScore.submittable).isTrue(); - assertThat(changeReadyWithOwnerScore.requirements).containsExactly(READY); + verifyChangeReady(changeReadyWithOwnerScore); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldConfiguredLabelScoreByCodeOwnerBeNotSufficientIfLabelRequiresMaxValue() throws Exception { TestAccount admin2 = accountCreator.admin2(); @@ -313,13 +266,13 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.recommend()); ChangeInfo ownersVoteNotSufficient = changeApi.get(); assertThat(ownersVoteNotSufficient.submittable).isFalse(); - assertThat(ownersVoteNotSufficient.requirements).containsExactly(READY); + verifyChangeReady(ownersVoteNotSufficient); verifyHasSubmitRecord( ownersVoteNotSufficient.submitRecords, LabelId.CODE_REVIEW, @@ -329,7 +282,7 @@ forChange(r).current().review(ReviewInput.approve()); ChangeInfo changeReadyWithMaxScore = forChange(r).get(); assertThat(changeReadyWithMaxScore.submittable).isTrue(); - assertThat(changeReadyWithMaxScore.requirements).containsExactly(READY); + verifyChangeReady(changeReadyWithMaxScore); verifyHasSubmitRecord( changeReadyWithMaxScore.submitRecords, LabelId.CODE_REVIEW, @@ -337,10 +290,6 @@ } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldConfiguredLabelScoreByCodeOwnersOverwriteSubmitRequirement() throws Exception { installLabel(TestLabels.codeReview().toBuilder().setFunction(LabelFunction.NO_OP).build()); @@ -351,25 +300,22 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.recommend()); ChangeInfo ownersVoteSufficient = forChange(r).get(); assertThat(ownersVoteSufficient.submittable).isTrue(); - assertThat(ownersVoteSufficient.requirements).containsExactly(READY); + verifyChangeReady(ownersVoteSufficient); } @Test - @GlobalPluginConfig( - pluginName = "owners", - name = "owners.enableSubmitRequirement", - value = "true") public void shouldRequireApprovalFromGrandParentProjectOwner() throws Exception { Project.NameKey parentProjectName = createProjectOverAPI("parent", allProjects, true, SubmitType.FAST_FORWARD_ONLY); Project.NameKey childProjectName = createProjectOverAPI("child", parentProjectName, true, SubmitType.FAST_FORWARD_ONLY); + updateChildProjectConfiguration(childProjectName); TestRepository<InMemoryRepository> childRepo = cloneProject(childProjectName); TestAccount admin2 = accountCreator.admin2(); @@ -380,20 +326,41 @@ ChangeApi changeApi = forChange(r); ChangeInfo changeNotReady = changeApi.get(); assertThat(changeNotReady.submittable).isFalse(); - assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReady); changeApi.current().review(ReviewInput.approve()); ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get(); assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse(); - assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY); + verifyChangeNotReady(changeNotReadyAfterSelfApproval); requestScopeOperations.setApiUser(admin2.id()); forChange(r).current().review(ReviewInput.approve()); ChangeInfo changeReady = forChange(r).get(); assertThat(changeReady.submittable).isTrue(); - assertThat(changeReady.requirements).containsExactly(READY); + verifyChangeReady(changeReady); } + @Test + public void shouldIndicateRuleErrorForBrokenOwnersFile() throws Exception { + addBrokenOwnersFileToRoot(); + + PushOneCommit.Result r = createChange("Add a file", "README.md", "foo"); + ChangeApi changeApi = forChange(r); + ChangeInfo changeNotReady = changeApi.get(); + assertThat(changeNotReady.submittable).isFalse(); + assertThat(changeNotReady.requirements).isEmpty(); + verifyRuleError(changeNotReady); + } + + protected abstract void verifyChangeNotReady(ChangeInfo notReady); + + protected abstract void verifyChangeReady(ChangeInfo ready); + + protected abstract void verifyRuleError(ChangeInfo change); + + protected abstract void updateChildProjectConfiguration(Project.NameKey childProject) + throws Exception; + private void verifyHasSubmitRecord( Collection<SubmitRecordInfo> records, String label, SubmitRecordInfo.Label.Status status) { assertThat( @@ -435,6 +402,10 @@ return gApi.changes().id(r.getChangeId()); } + private void addBrokenOwnersFileToRoot() throws Exception { + pushOwnersToMaster("{foo"); + } + private void addOwnerFileWithMatchersToRoot( boolean inherit, String extension, TestAccount... users) throws Exception { // Add OWNERS file to root:
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();
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java index 936f3d3..c39974c 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java
@@ -36,6 +36,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.server.project.testing.TestLabels; +import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; import com.googlesource.gerrit.owners.entities.FilesOwnersResponse; import com.googlesource.gerrit.owners.entities.GroupOwner; import com.googlesource.gerrit.owners.entities.Owner; @@ -223,6 +224,20 @@ assertThat(thrown).hasCauseThat().isInstanceOf(LabelNotFoundException.class); } + @Test + @UseLocalDisk + public void shouldThrowResourceConflictWhenOwnersFileIsBroken() throws Exception { + addBrokenOwnersFileToRoot(); + String changeId = createChange().getChangeId(); + + ResourceConflictException thrown = + assertThrows( + ResourceConflictException.class, + () -> ownersApi.apply(parseCurrentRevisionResource(changeId))); + assertThat(thrown).hasMessageThat().startsWith("Invalid owners file: OWNERS"); + assertThat(thrown).hasCauseThat().isInstanceOf(InvalidOwnersFileException.class); + } + protected void replaceCodeReviewWithLabel(LabelType label) throws Exception { try (ProjectConfigUpdate u = updateProject(allProjects)) { u.getConfig().getLabelSections().remove(LabelId.CODE_REVIEW); @@ -264,6 +279,10 @@ .containsExactly("a.txt", Sets.newHashSet(rootOwner, projectOwner)); } + private void addBrokenOwnersFileToRoot() throws Exception { + merge(createChange(testRepo, "master", "Add OWNER file", "OWNERS", "{foo", "")); + } + private void addOwnerFileToProjectConfig(Project.NameKey projectNameKey, boolean inherit) throws Exception { addOwnerFileToProjectConfig(projectNameKey, inherit, user);