Warn when a code owner config directly imports itself
It's never useful if an OWNERS file directly imports itself. If this is
done, it's almost certainly a mistake and the intention was to import
another OWNERS file. Report a warning if we see this.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I843389909edbf70325d262f6a18942c81c474643
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index e4714bd..cef6d52 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -855,6 +855,14 @@
PathCodeOwners.createKeyForImportedCodeOwnerConfig(
keyOfImportingCodeOwnerConfig, codeOwnerConfigReference);
+ if (isSelfImport(keyOfImportingCodeOwnerConfig, keyOfImportedCodeOwnerConfig)) {
+ return nonResolvableImport(
+ importType,
+ codeOwnerConfigFilePath,
+ "code owner config imports itself",
+ ValidationMessage.Type.WARNING);
+ }
+
Optional<ProjectState> projectState = projectCache.get(keyOfImportedCodeOwnerConfig.project());
if (!projectState.isPresent() || !isProjectReadable(keyOfImportedCodeOwnerConfig)) {
// we intentionally use the same error message for non-existing and non-readable projects so
@@ -944,6 +952,21 @@
return Optional.empty();
}
+ /** Whether the importing code owner config is the same as the imported code owner config. */
+ private boolean isSelfImport(
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig) {
+ return keyOfImportingCodeOwnerConfig.project().equals(keyOfImportedCodeOwnerConfig.project())
+ && keyOfImportingCodeOwnerConfig.ref().equals(keyOfImportedCodeOwnerConfig.ref())
+ && codeOwnersPluginConfiguration
+ .getBackend(keyOfImportingCodeOwnerConfig.branchNameKey())
+ .getFilePath(keyOfImportingCodeOwnerConfig)
+ .equals(
+ codeOwnersPluginConfiguration
+ .getBackend(keyOfImportedCodeOwnerConfig.branchNameKey())
+ .getFilePath(keyOfImportedCodeOwnerConfig));
+ }
+
private boolean isProjectReadable(CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig) {
try {
return permissionBackend
@@ -994,14 +1017,26 @@
CodeOwnerConfigImportType importType,
Path codeOwnerConfigFilePath,
String message) {
+ return nonResolvableImport(
+ importType,
+ codeOwnerConfigFilePath,
+ message,
+ codeOwnersPluginConfiguration.rejectNonResolvableImports(project)
+ ? ValidationMessage.Type.ERROR
+ : ValidationMessage.Type.WARNING);
+ }
+
+ private Optional<CommitValidationMessage> nonResolvableImport(
+ CodeOwnerConfigImportType importType,
+ Path codeOwnerConfigFilePath,
+ String message,
+ ValidationMessage.Type validationMessageType) {
return Optional.of(
new CommitValidationMessage(
String.format(
"invalid %s import in '%s': %s",
importType.getType(), codeOwnerConfigFilePath, message),
- codeOwnersPluginConfiguration.rejectNonResolvableImports(project)
- ? ValidationMessage.Type.ERROR
- : ValidationMessage.Type.WARNING));
+ validationMessageType));
}
private Optional<CommitValidationMessage> nonResolvableCodeOwner(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index 12b3360..3dfa9db 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -1023,6 +1023,102 @@
}
@Test
+ public void uploadConfigWithGlobalSelfImportReportsAWarning() throws Exception {
+ testUploadConfigWithSelfImport(CodeOwnerConfigImportType.GLOBAL);
+ }
+
+ @Test
+ public void uploadConfigWithPerFileSelfImportReportsAWarning() throws Exception {
+ testUploadConfigWithSelfImport(CodeOwnerConfigImportType.PER_FILE);
+ }
+
+ private void testUploadConfigWithSelfImport(CodeOwnerConfigImportType importType)
+ throws Exception {
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
+ // create a code owner config that imports itself
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/");
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getFilePath())
+ .setProject(project)
+ .build();
+ CodeOwnerConfig codeOwnerConfig =
+ createCodeOwnerConfigWithImport(
+ keyOfImportingCodeOwnerConfig, importType, codeOwnerConfigReference);
+
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(codeOwnerConfig));
+ assertOkWithWarnings(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid %s import in '%s': code owner config imports itself",
+ importType.getType(),
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getFilePath()));
+ }
+
+ @Test
+ public void canUploadConfigWithGlobalImportOfFileWithExtensionFromSameFolder() throws Exception {
+ testUploadConfigWithImportOfFileWithExtensionFromSameFolder(CodeOwnerConfigImportType.GLOBAL);
+ }
+
+ @Test
+ public void canUploadConfigWithPerFileImportOfFileWithExtensionFromSameFolder() throws Exception {
+ testUploadConfigWithImportOfFileWithExtensionFromSameFolder(CodeOwnerConfigImportType.PER_FILE);
+ }
+
+ private void testUploadConfigWithImportOfFileWithExtensionFromSameFolder(
+ CodeOwnerConfigImportType importType) throws Exception {
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
+ // create a code owner config that imports a code owner config from the same folder but with an
+ // extension in the file name
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = createCodeOwnerConfigKey("/");
+ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .fileName(getCodeOwnerConfigFileName() + "_extension")
+ .addCodeOwnerEmail(user.email())
+ .create();
+ GitUtil.fetch(testRepo, "refs/*:refs/*");
+ testRepo.reset(projectOperations.project(project).getHead("master"));
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportedCodeOwnerConfig)
+ .getFilePath())
+ .setProject(project)
+ .build();
+ CodeOwnerConfig codeOwnerConfig =
+ createCodeOwnerConfigWithImport(
+ keyOfImportingCodeOwnerConfig, importType, codeOwnerConfigReference);
+
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(keyOfImportingCodeOwnerConfig)
+ .getJGitFilePath(),
+ format(codeOwnerConfig));
+ r.assertOkStatus();
+ }
+
+ @Test
public void cannotUploadConfigWithGlobalImportFromNonExistingProject() throws Exception {
testUploadConfigWithImportFromNonExistingProject(CodeOwnerConfigImportType.GLOBAL);
}
@@ -1764,6 +1860,16 @@
return ObjectIds.abbreviateName(id, testRepo.getRevWalk().getObjectReader());
}
+ private String getCodeOwnerConfigFileName() {
+ CodeOwnerBackend backend = backendConfig.getDefaultBackend();
+ if (backend instanceof FindOwnersBackend) {
+ return FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME;
+ } else if (backend instanceof ProtoBackend) {
+ return ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME;
+ }
+ throw new IllegalStateException("unknown code owner backend: " + backend.getClass().getName());
+ }
+
private static void assertOkWithoutMessages(PushOneCommit.Result pushResult) {
pushResult.assertOkStatus();
pushResult.assertNotMessage("fatal");
