From 26b8ddab93ee1223b93bcced8195067f67786525 Mon Sep 17 00:00:00 2001 From: Daniel Mohedano Date: Thu, 8 May 2025 16:04:02 +0200 Subject: [PATCH 1/6] normalize repo path before building git info --- .../src/main/java/datadog/trace/api/git/GitInfoProvider.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index 1eb767d33c3..ff9f7a8cb2f 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -9,6 +9,7 @@ import datadog.trace.api.civisibility.telemetry.tag.GitShaDiscrepancyType; import datadog.trace.api.civisibility.telemetry.tag.GitShaMatch; import datadog.trace.util.Strings; +import java.io.File; import java.nio.file.Paths; import java.util.ArrayList; import java.util.Collection; @@ -54,6 +55,9 @@ public GitInfo getGitInfo() { public GitInfo getGitInfo(@Nullable String repositoryPath) { if (repositoryPath == null) { repositoryPath = NULL_PATH_STRING; + } else if (!repositoryPath.endsWith(File.separator)) { + // normalize to correctly hit the cache + repositoryPath = repositoryPath + File.separator; } return gitInfoCache.computeIfAbsent(repositoryPath, this::buildGitInfo); } From 45224090b38fda61d7fdb8cd37379e86ee349551 Mon Sep 17 00:00:00 2001 From: Daniel Mohedano Date: Thu, 8 May 2025 16:59:33 +0200 Subject: [PATCH 2/6] unit test and usage of Paths to avoid issues with root directory --- .../trace/api/git/GitInfoProvider.java | 7 +- .../trace/api/git/GitInfoProviderTest.groovy | 131 ++++++++++-------- 2 files changed, 78 insertions(+), 60 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index ff9f7a8cb2f..754187a7bab 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -55,11 +55,10 @@ public GitInfo getGitInfo() { public GitInfo getGitInfo(@Nullable String repositoryPath) { if (repositoryPath == null) { repositoryPath = NULL_PATH_STRING; - } else if (!repositoryPath.endsWith(File.separator)) { - // normalize to correctly hit the cache - repositoryPath = repositoryPath + File.separator; } - return gitInfoCache.computeIfAbsent(repositoryPath, this::buildGitInfo); + + // normalize path to avoid creating two entries in the cache + return gitInfoCache.computeIfAbsent(Paths.get(repositoryPath).toString(), this::buildGitInfo); } private GitInfo buildGitInfo(String repositoryPath) { diff --git a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy index c6177f24901..c88344218aa 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy @@ -17,7 +17,7 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilder = givenABuilderReturning( new GitInfo("repoUrl", "branch", "tag", new CommitInfo("sha")) - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilder) @@ -36,11 +36,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, new CommitInfo(null)) - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo(null, "branch", "tag", new CommitInfo("sha")), 2 - ) + ) def gitInfoProvider = new GitInfoProvider() // registering provider with higher order first, to check that the registration logic will do proper reordering after the second registration @@ -61,11 +61,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", "", "", new CommitInfo("")) - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo(null, "branch", "tag", new CommitInfo("sha")) - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -85,11 +85,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", " ", " ", new CommitInfo(" ")) - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo(null, "branch", "tag", new CommitInfo("sha")) - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -109,17 +109,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - PersonInfo.NOOP, - PersonInfo.NOOP, - null))) + new CommitInfo("sha", + PersonInfo.NOOP, + PersonInfo.NOOP, + null))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -144,17 +144,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("", "", ""), - new PersonInfo("", "", ""), - ""))) + new CommitInfo("sha", + new PersonInfo("", "", ""), + new PersonInfo("", "", ""), + ""))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -179,17 +179,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo(" ", " ", " "), - new PersonInfo(" ", " ", " "), - " "))) + new CommitInfo("sha", + new PersonInfo(" ", " ", " "), + new PersonInfo(" ", " ", " "), + " "))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -214,17 +214,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - PersonInfo.NOOP, - PersonInfo.NOOP, - "message"))) + new CommitInfo("sha", + PersonInfo.NOOP, + PersonInfo.NOOP, + "message"))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("different sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("different sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -252,21 +252,21 @@ class GitInfoProviderTest extends Specification { def gitInfoA = new GitInfo("repoUrlA", null, null, new CommitInfo("shaA", - PersonInfo.NOOP, - PersonInfo.NOOP, - "message" + PersonInfo.NOOP, + PersonInfo.NOOP, + "message" )) def gitInfoB = new GitInfo("repoUrlA", null, null, new CommitInfo("shaB", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message" + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message" )) def gitInfoC = new GitInfo("repoUrlB", null, null, new CommitInfo("shaC", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message" + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message" )) def gitInfoBuilderA = givenABuilderReturning(gitInfoA, 1, GitProviderExpected.CI_PROVIDER, GitProviderDiscrepant.CI_PROVIDER) @@ -294,15 +294,15 @@ class GitInfoProviderTest extends Specification { def gitInfoA = new GitInfo("repoUrlA", null, null, new CommitInfo("shaA", - PersonInfo.NOOP, - PersonInfo.NOOP, - "message" + PersonInfo.NOOP, + PersonInfo.NOOP, + "message" )) def gitInfoB = new GitInfo("repoUrlA", null, null, new CommitInfo("shaA", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message" + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message" )) def gitInfoBuilderA = givenABuilderReturning(gitInfoA, 1, GitProviderExpected.CI_PROVIDER, GitProviderDiscrepant.CI_PROVIDER) @@ -324,11 +324,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("file://uselessUrl", null, null, new CommitInfo(null)), 1 - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("http://usefulUrl", null, null, new CommitInfo(null)), 2 - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -341,6 +341,25 @@ class GitInfoProviderTest extends Specification { actualGitInfo.repositoryURL == "http://usefulUrl" } + def "test caches git info regardless of path normalization"() { + setup: + def gitInfo = new GitInfo("repoUrl", "branch", "tag", new CommitInfo("sha")) + def gitInfoBuilder = Mock(GitInfoBuilder) + gitInfoBuilder.order() >> 1 + gitInfoBuilder.providerAsExpected() >> GitProviderExpected.USER_SUPPLIED + gitInfoBuilder.providerAsDiscrepant() >> GitProviderDiscrepant.USER_SUPPLIED + + def gitInfoProvider = new GitInfoProvider() + gitInfoProvider.registerGitInfoBuilder(gitInfoBuilder) + + when: + gitInfoProvider.getGitInfo(REPO_PATH) + gitInfoProvider.getGitInfo(REPO_PATH + File.separator) + + then: + 1 * gitInfoBuilder.build(REPO_PATH) >> gitInfo + } + private GitInfoBuilder givenABuilderReturning(GitInfo gitInfo) { givenABuilderReturning(gitInfo, 1) } From 6060bb4aabbc28a525db5a4ae17042f2377a764e Mon Sep 17 00:00:00 2001 From: Daniel Mohedano Date: Thu, 8 May 2025 17:13:13 +0200 Subject: [PATCH 3/6] spotless --- .../trace/api/git/GitInfoProvider.java | 1 - .../trace/api/git/GitInfoProviderTest.groovy | 112 +++++++++--------- 2 files changed, 56 insertions(+), 57 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index 754187a7bab..33a1e34694e 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -9,7 +9,6 @@ import datadog.trace.api.civisibility.telemetry.tag.GitShaDiscrepancyType; import datadog.trace.api.civisibility.telemetry.tag.GitShaMatch; import datadog.trace.util.Strings; -import java.io.File; import java.nio.file.Paths; import java.util.ArrayList; import java.util.Collection; diff --git a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy index c88344218aa..ce349aa5152 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy @@ -17,7 +17,7 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilder = givenABuilderReturning( new GitInfo("repoUrl", "branch", "tag", new CommitInfo("sha")) - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilder) @@ -36,11 +36,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, new CommitInfo(null)) - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo(null, "branch", "tag", new CommitInfo("sha")), 2 - ) + ) def gitInfoProvider = new GitInfoProvider() // registering provider with higher order first, to check that the registration logic will do proper reordering after the second registration @@ -61,11 +61,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", "", "", new CommitInfo("")) - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo(null, "branch", "tag", new CommitInfo("sha")) - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -85,11 +85,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", " ", " ", new CommitInfo(" ")) - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo(null, "branch", "tag", new CommitInfo("sha")) - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -109,17 +109,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - PersonInfo.NOOP, - PersonInfo.NOOP, - null))) + new CommitInfo("sha", + PersonInfo.NOOP, + PersonInfo.NOOP, + null))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -144,17 +144,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("", "", ""), - new PersonInfo("", "", ""), - ""))) + new CommitInfo("sha", + new PersonInfo("", "", ""), + new PersonInfo("", "", ""), + ""))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -179,17 +179,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo(" ", " ", " "), - new PersonInfo(" ", " ", " "), - " "))) + new CommitInfo("sha", + new PersonInfo(" ", " ", " "), + new PersonInfo(" ", " ", " "), + " "))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -214,17 +214,17 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("sha", - PersonInfo.NOOP, - PersonInfo.NOOP, - "message"))) + new CommitInfo("sha", + PersonInfo.NOOP, + PersonInfo.NOOP, + "message"))) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("repoUrl", null, null, - new CommitInfo("different sha", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message"))) + new CommitInfo("different sha", + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message"))) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) @@ -252,21 +252,21 @@ class GitInfoProviderTest extends Specification { def gitInfoA = new GitInfo("repoUrlA", null, null, new CommitInfo("shaA", - PersonInfo.NOOP, - PersonInfo.NOOP, - "message" + PersonInfo.NOOP, + PersonInfo.NOOP, + "message" )) def gitInfoB = new GitInfo("repoUrlA", null, null, new CommitInfo("shaB", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message" + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message" )) def gitInfoC = new GitInfo("repoUrlB", null, null, new CommitInfo("shaC", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message" + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message" )) def gitInfoBuilderA = givenABuilderReturning(gitInfoA, 1, GitProviderExpected.CI_PROVIDER, GitProviderDiscrepant.CI_PROVIDER) @@ -294,15 +294,15 @@ class GitInfoProviderTest extends Specification { def gitInfoA = new GitInfo("repoUrlA", null, null, new CommitInfo("shaA", - PersonInfo.NOOP, - PersonInfo.NOOP, - "message" + PersonInfo.NOOP, + PersonInfo.NOOP, + "message" )) def gitInfoB = new GitInfo("repoUrlA", null, null, new CommitInfo("shaA", - new PersonInfo("author name", "author email", "author date"), - new PersonInfo("committer name", "committer email", "committer date"), - "message" + new PersonInfo("author name", "author email", "author date"), + new PersonInfo("committer name", "committer email", "committer date"), + "message" )) def gitInfoBuilderA = givenABuilderReturning(gitInfoA, 1, GitProviderExpected.CI_PROVIDER, GitProviderDiscrepant.CI_PROVIDER) @@ -324,11 +324,11 @@ class GitInfoProviderTest extends Specification { setup: def gitInfoBuilderA = givenABuilderReturning( new GitInfo("file://uselessUrl", null, null, new CommitInfo(null)), 1 - ) + ) def gitInfoBuilderB = givenABuilderReturning( new GitInfo("http://usefulUrl", null, null, new CommitInfo(null)), 2 - ) + ) def gitInfoProvider = new GitInfoProvider() gitInfoProvider.registerGitInfoBuilder(gitInfoBuilderA) From 9cc396e039b4c69e1ed66a0b3b032d3f2b6b1d7e Mon Sep 17 00:00:00 2001 From: Daniel Mohedano Date: Fri, 9 May 2025 11:03:55 +0200 Subject: [PATCH 4/6] sanitize ciworkspace instead of using Path --- .../CiVisibilityRepoServices.java | 13 ++----------- .../datadog/trace/civisibility/ci/CIInfo.java | 12 +++--------- .../trace/civisibility/ci/CITagsProvider.java | 2 +- .../CompilerAidedSourcePathResolver.java | 3 ++- .../codeowners/CodeownersProviderTest.groovy | 10 +++++----- ...CompilerAidedSourcePathResolverTest.groovy | 2 +- .../trace/api/git/GitInfoProvider.java | 2 +- .../trace/api/git/GitInfoProviderTest.groovy | 19 ------------------- 8 files changed, 15 insertions(+), 48 deletions(-) diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java index 7ebbe41c005..d3c0280a84b 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java @@ -35,7 +35,6 @@ import datadog.trace.civisibility.source.index.RepoIndexProvider; import datadog.trace.civisibility.source.index.RepoIndexSourcePathResolver; import datadog.trace.util.Strings; -import java.io.File; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Map; @@ -72,7 +71,7 @@ public class CiVisibilityRepoServices { LOGGER.info("PR detected: {}", pullRequestInfo); } - repoRoot = appendSlashIfNeeded(getRepoRoot(ciInfo, services.gitClientFactory)); + repoRoot = getRepoRoot(ciInfo, services.gitClientFactory); moduleName = getModuleName(services.config, repoRoot, path); ciTags = new CITagsProvider().getCiTags(ciInfo, pullRequestInfo); @@ -126,7 +125,7 @@ private static PullRequestInfo buildPullRequestInfo( } private static String getRepoRoot(CIInfo ciInfo, GitClient.Factory gitClientFactory) { - String ciWorkspace = ciInfo.getNormalizedCiWorkspace(); + String ciWorkspace = ciInfo.getCiWorkspace(); if (Strings.isNotBlank(ciWorkspace)) { return ciWorkspace; @@ -146,14 +145,6 @@ private static String getRepoRoot(CIInfo ciInfo, GitClient.Factory gitClientFact } } - private static String appendSlashIfNeeded(String repoRoot) { - if (repoRoot != null && !repoRoot.endsWith(File.separator)) { - return repoRoot + File.separator; - } else { - return repoRoot; - } - } - static String getModuleName(Config config, @Nullable String repoRoot, Path path) { // if parent process is instrumented, it will provide build system's module name String parentModuleName = config.getCiVisibilityModuleName(); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java index 9fb2bbfb81f..fd68d31b9a9 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java @@ -210,19 +210,13 @@ public String getCiJobUrl() { } /** - * @deprecated This method is here only to satisfy CI spec tests. Use {@link - * #getNormalizedCiWorkspace()} + * @return Workspace path without the trailing separator */ - @Deprecated public String getCiWorkspace() { - return ciWorkspace; - } - - public String getNormalizedCiWorkspace() { String realCiWorkspace = FileUtils.toRealPath(ciWorkspace); - return (realCiWorkspace == null || realCiWorkspace.endsWith(File.separator)) + return (realCiWorkspace == null || !realCiWorkspace.endsWith(File.separator)) ? realCiWorkspace - : (realCiWorkspace + File.separator); + : (realCiWorkspace.substring(0, realCiWorkspace.length() - 1)); } public String getCiNodeName() { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CITagsProvider.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CITagsProvider.java index a41bbb28af9..53cedefd702 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CITagsProvider.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CITagsProvider.java @@ -22,7 +22,7 @@ public CITagsProvider() { } public Map getCiTags(CIInfo ciInfo, PullRequestInfo pullRequestInfo) { - String repoRoot = ciInfo.getNormalizedCiWorkspace(); + String repoRoot = ciInfo.getCiWorkspace(); GitInfo gitInfo = gitInfoProvider.getGitInfo(repoRoot); return new CITagsBuilder() diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/CompilerAidedSourcePathResolver.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/CompilerAidedSourcePathResolver.java index 73b51d78e70..c2aa8831780 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/CompilerAidedSourcePathResolver.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/CompilerAidedSourcePathResolver.java @@ -1,6 +1,7 @@ package datadog.trace.civisibility.source; import datadog.compiler.utils.CompilerUtils; +import java.io.File; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -9,7 +10,7 @@ public class CompilerAidedSourcePathResolver implements SourcePathResolver { private final String repoRoot; public CompilerAidedSourcePathResolver(String repoRoot) { - this.repoRoot = repoRoot; + this.repoRoot = repoRoot.endsWith(File.separator) ? repoRoot : repoRoot + File.separator; } @Nullable diff --git a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/codeowners/CodeownersProviderTest.groovy b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/codeowners/CodeownersProviderTest.groovy index dde2cb02748..27f578276e0 100644 --- a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/codeowners/CodeownersProviderTest.groovy +++ b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/codeowners/CodeownersProviderTest.groovy @@ -8,7 +8,7 @@ import java.nio.file.Files class CodeownersProviderTest extends Specification { - private static final String REPO_ROOT = "/repo/root/" + private static final String REPO_ROOT = "/repo/root" def "test codeowners loading: #path"() { setup: @@ -30,10 +30,10 @@ class CodeownersProviderTest extends Specification { where: path << [ - REPO_ROOT + "CODEOWNERS", - REPO_ROOT + ".github/CODEOWNERS", - REPO_ROOT + ".gitlab/CODEOWNERS", - REPO_ROOT + "docs/CODEOWNERS" + REPO_ROOT + "/CODEOWNERS", + REPO_ROOT + "/.github/CODEOWNERS", + REPO_ROOT + "/.gitlab/CODEOWNERS", + REPO_ROOT + "/docs/CODEOWNERS" ] } } diff --git a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/source/CompilerAidedSourcePathResolverTest.groovy b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/source/CompilerAidedSourcePathResolverTest.groovy index 581d2d95671..77b563fd96c 100644 --- a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/source/CompilerAidedSourcePathResolverTest.groovy +++ b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/source/CompilerAidedSourcePathResolverTest.groovy @@ -5,7 +5,7 @@ import spock.lang.Specification class CompilerAidedSourcePathResolverTest extends Specification { - public static final String REPO_ROOT = "/repo/root/" + public static final String REPO_ROOT = "/repo/root" public static final String SOURCE_PATH_VALUE = "/repo/root/path/to/AClassWithSourceInfoInjected.java" public static final String SOURCE_PATH_OUTSIDE_REPO_VALUE = "/outside/path/to/AClassWithSourceInfoInjected.java" diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index 33a1e34694e..e9b930f7de3 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -57,7 +57,7 @@ public GitInfo getGitInfo(@Nullable String repositoryPath) { } // normalize path to avoid creating two entries in the cache - return gitInfoCache.computeIfAbsent(Paths.get(repositoryPath).toString(), this::buildGitInfo); + return gitInfoCache.computeIfAbsent(repositoryPath, this::buildGitInfo); } private GitInfo buildGitInfo(String repositoryPath) { diff --git a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy index ce349aa5152..c6177f24901 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/git/GitInfoProviderTest.groovy @@ -341,25 +341,6 @@ class GitInfoProviderTest extends Specification { actualGitInfo.repositoryURL == "http://usefulUrl" } - def "test caches git info regardless of path normalization"() { - setup: - def gitInfo = new GitInfo("repoUrl", "branch", "tag", new CommitInfo("sha")) - def gitInfoBuilder = Mock(GitInfoBuilder) - gitInfoBuilder.order() >> 1 - gitInfoBuilder.providerAsExpected() >> GitProviderExpected.USER_SUPPLIED - gitInfoBuilder.providerAsDiscrepant() >> GitProviderDiscrepant.USER_SUPPLIED - - def gitInfoProvider = new GitInfoProvider() - gitInfoProvider.registerGitInfoBuilder(gitInfoBuilder) - - when: - gitInfoProvider.getGitInfo(REPO_PATH) - gitInfoProvider.getGitInfo(REPO_PATH + File.separator) - - then: - 1 * gitInfoBuilder.build(REPO_PATH) >> gitInfo - } - private GitInfoBuilder givenABuilderReturning(GitInfo gitInfo) { givenABuilderReturning(gitInfo, 1) } From 53f52c0a0dd52ec6293fd26cfc8caf48985d0515 Mon Sep 17 00:00:00 2001 From: Daniel Mohedano Date: Fri, 9 May 2025 11:10:13 +0200 Subject: [PATCH 5/6] fix spotless --- .../src/main/java/datadog/trace/civisibility/ci/CIInfo.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java index fd68d31b9a9..eb3ad32102f 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java @@ -209,9 +209,7 @@ public String getCiJobUrl() { return ciJobUrl; } - /** - * @return Workspace path without the trailing separator - */ + /** @return Workspace path without the trailing separator */ public String getCiWorkspace() { String realCiWorkspace = FileUtils.toRealPath(ciWorkspace); return (realCiWorkspace == null || !realCiWorkspace.endsWith(File.separator)) From de21bd10c227224eb540f02d63e6191ab3ae5a38 Mon Sep 17 00:00:00 2001 From: Daniel Mohedano Date: Fri, 9 May 2025 13:10:40 +0200 Subject: [PATCH 6/6] add unit tests and take into account root path --- .../datadog/trace/civisibility/ci/CIInfo.java | 16 +++++++++----- .../trace/civisibility/ci/CIInfoTest.groovy | 22 +++++++++++++++++++ .../trace/api/git/GitInfoProvider.java | 1 - 3 files changed, 33 insertions(+), 6 deletions(-) create mode 100644 dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/ci/CIInfoTest.groovy diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java index eb3ad32102f..a2d221197bc 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/ci/CIInfo.java @@ -170,7 +170,7 @@ public CIInfo( this.ciPipelineNumber = ciPipelineNumber; this.ciPipelineUrl = ciPipelineUrl; this.ciJobUrl = ciJobUrl; - this.ciWorkspace = ciWorkspace; + this.ciWorkspace = sanitizeWorkspace(ciWorkspace); this.ciNodeName = ciNodeName; this.ciNodeLabels = ciNodeLabels; this.ciEnvVars = ciEnvVars; @@ -209,14 +209,20 @@ public String getCiJobUrl() { return ciJobUrl; } - /** @return Workspace path without the trailing separator */ - public String getCiWorkspace() { - String realCiWorkspace = FileUtils.toRealPath(ciWorkspace); - return (realCiWorkspace == null || !realCiWorkspace.endsWith(File.separator)) + private String sanitizeWorkspace(String workspace) { + String realCiWorkspace = FileUtils.toRealPath(workspace); + return (realCiWorkspace == null + || !realCiWorkspace.endsWith(File.separator) + || realCiWorkspace.length() == 1) // root path "/" ? realCiWorkspace : (realCiWorkspace.substring(0, realCiWorkspace.length() - 1)); } + /** @return Workspace path without the trailing separator */ + public String getCiWorkspace() { + return ciWorkspace; + } + public String getCiNodeName() { return ciNodeName; } diff --git a/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/ci/CIInfoTest.groovy b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/ci/CIInfoTest.groovy new file mode 100644 index 00000000000..a8686c3483d --- /dev/null +++ b/dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/ci/CIInfoTest.groovy @@ -0,0 +1,22 @@ +package datadog.trace.civisibility.ci + + +import spock.lang.Specification + +class CIInfoTest extends Specification { + + def "test ci workspace is correctly sanitized #iterationIndex"() { + def builder = CIInfo.builder(null) + builder.ciWorkspace(workspacePath) + def info = builder.build() + + info.ciWorkspace == sanitizedPath + + where: + workspacePath | sanitizedPath + null | null + "/" | "/" + "/repo/path" | "/repo/path" + "/repo/path/" | "/repo/path" + } +} diff --git a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java index e9b930f7de3..9550a16236e 100644 --- a/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java +++ b/internal-api/src/main/java/datadog/trace/api/git/GitInfoProvider.java @@ -56,7 +56,6 @@ public GitInfo getGitInfo(@Nullable String repositoryPath) { repositoryPath = NULL_PATH_STRING; } - // normalize path to avoid creating two entries in the cache return gitInfoCache.computeIfAbsent(repositoryPath, this::buildGitInfo); }