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");