Skip to content

Commit 67e94a0

Browse files
nikita-tkachenko-datadognikita-tkachenko-datadog
authored
Throw exception when using repo index to resolve source path for classes with identical names (#7806)
1 parent d0ae08a commit 67e94a0

File tree

16 files changed

+158
-42
lines changed

16 files changed

+158
-42
lines changed

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/ConcurrentCoverageStore.java

+9-3
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import datadog.trace.api.civisibility.coverage.CoverageProbes;
55
import datadog.trace.api.civisibility.coverage.CoverageStore;
66
import datadog.trace.api.civisibility.coverage.TestReport;
7+
import datadog.trace.civisibility.source.SourceResolutionException;
78
import java.util.Collection;
89
import java.util.Map;
910
import java.util.concurrent.ConcurrentHashMap;
@@ -36,13 +37,18 @@ private T create(Thread thread) {
3637

3738
@Override
3839
public boolean report(DDTraceId testSessionId, Long testSuiteId, long testSpanId) {
39-
report = report(testSessionId, testSuiteId, testSpanId, probes.values());
40-
return report != null && report.isNotEmpty();
40+
try {
41+
report = report(testSessionId, testSuiteId, testSpanId, probes.values());
42+
return report != null && report.isNotEmpty();
43+
} catch (SourceResolutionException e) {
44+
return false;
45+
}
4146
}
4247

4348
@Nullable
4449
protected abstract TestReport report(
45-
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<T> probes);
50+
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<T> probes)
51+
throws SourceResolutionException;
4652

4753
@Nullable
4854
@Override

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileCoverageStore.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import datadog.trace.api.civisibility.telemetry.tag.CoverageErrorType;
1212
import datadog.trace.civisibility.coverage.ConcurrentCoverageStore;
1313
import datadog.trace.civisibility.source.SourcePathResolver;
14+
import datadog.trace.civisibility.source.SourceResolutionException;
1415
import java.util.ArrayList;
1516
import java.util.Collection;
1617
import java.util.Collections;
@@ -46,7 +47,8 @@ private FileCoverageStore(
4647
@Nullable
4748
@Override
4849
protected TestReport report(
49-
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<FileProbes> probes) {
50+
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<FileProbes> probes)
51+
throws SourceResolutionException {
5052
try {
5153
Set<Class<?>> combinedClasses = Collections.newSetFromMap(new IdentityHashMap<>());
5254
Collection<String> combinedNonCodeResources = new HashSet<>();

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineCoverageStore.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import datadog.trace.api.civisibility.telemetry.tag.CoverageErrorType;
1212
import datadog.trace.civisibility.coverage.ConcurrentCoverageStore;
1313
import datadog.trace.civisibility.source.SourcePathResolver;
14+
import datadog.trace.civisibility.source.SourceResolutionException;
1415
import datadog.trace.civisibility.source.Utils;
1516
import java.io.InputStream;
1617
import java.util.ArrayList;
@@ -52,7 +53,8 @@ private LineCoverageStore(
5253
@Nullable
5354
@Override
5455
protected TestReport report(
55-
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<LineProbes> probes) {
56+
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<LineProbes> probes)
57+
throws SourceResolutionException {
5658
try {
5759
Map<Class<?>, ExecutionDataAdapter> combinedExecutionData = new IdentityHashMap<>();
5860
Collection<String> combinedNonCodeResources = new HashSet<>();

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java

+19-6
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import datadog.trace.civisibility.ipc.ModuleCoverageDataJacoco;
1010
import datadog.trace.civisibility.ipc.SignalResponse;
1111
import datadog.trace.civisibility.ipc.SignalType;
12+
import datadog.trace.civisibility.source.SourceResolutionException;
1213
import datadog.trace.civisibility.source.index.RepoIndex;
1314
import datadog.trace.civisibility.source.index.RepoIndexProvider;
1415
import datadog.trace.util.Strings;
@@ -312,13 +313,19 @@ private RepoIndexFileLocator(RepoIndex repoIndex, String repoRoot) {
312313

313314
@Override
314315
protected InputStream getSourceStream(String path) throws IOException {
315-
String relativePath = repoIndex.getSourcePath(path);
316-
if (relativePath == null) {
316+
try {
317+
String relativePath = repoIndex.getSourcePath(path);
318+
if (relativePath == null) {
319+
return null;
320+
}
321+
String absolutePath =
322+
repoRoot + (!repoRoot.endsWith(File.separator) ? File.separator : "") + relativePath;
323+
return new BufferedInputStream(Files.newInputStream(Paths.get(absolutePath)));
324+
325+
} catch (SourceResolutionException e) {
326+
LOGGER.debug("Could not resolve source for path {}", path, e);
317327
return null;
318328
}
319-
String absolutePath =
320-
repoRoot + (!repoRoot.endsWith(File.separator) ? File.separator : "") + relativePath;
321-
return new BufferedInputStream(Files.newInputStream(Paths.get(absolutePath)));
322329
}
323330
}
324331

@@ -354,7 +361,13 @@ private long getMergedCoveragePercentage(IBundleCoverage coverageBundle) {
354361
String fileName = sourceFile.getName();
355362
String pathRelativeToSourceRoot =
356363
(Strings.isNotBlank(packageName) ? packageName + "/" : "") + fileName;
357-
String pathRelativeToIndexRoot = repoIndex.getSourcePath(pathRelativeToSourceRoot);
364+
String pathRelativeToIndexRoot;
365+
try {
366+
pathRelativeToIndexRoot = repoIndex.getSourcePath(pathRelativeToSourceRoot);
367+
} catch (SourceResolutionException e) {
368+
LOGGER.debug("Could not resolve source for path {}", pathRelativeToSourceRoot, e);
369+
continue;
370+
}
358371

359372
BitSet sourceFileCoveredLines = getCoveredLines(sourceFile);
360373
// backendCoverageData contains data for all modules in the repo,

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestImpl.java

+9-2
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import datadog.trace.civisibility.decorator.TestDecorator;
3333
import datadog.trace.civisibility.source.MethodLinesResolver;
3434
import datadog.trace.civisibility.source.SourcePathResolver;
35+
import datadog.trace.civisibility.source.SourceResolutionException;
3536
import java.lang.reflect.Method;
3637
import java.util.Collection;
3738
import java.util.function.Consumer;
@@ -147,8 +148,14 @@ private void populateSourceDataTags(
147148
return;
148149
}
149150

150-
String sourcePath = sourcePathResolver.getSourcePath(testClass);
151-
if (sourcePath == null || sourcePath.isEmpty()) {
151+
String sourcePath;
152+
try {
153+
sourcePath = sourcePathResolver.getSourcePath(testClass);
154+
if (sourcePath == null || sourcePath.isEmpty()) {
155+
return;
156+
}
157+
} catch (SourceResolutionException e) {
158+
log.debug("Could not populate source path for {}", testClass, e);
152159
return;
153160
}
154161

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestSuiteImpl.java

+22-7
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,18 @@
1919
import datadog.trace.civisibility.decorator.TestDecorator;
2020
import datadog.trace.civisibility.source.MethodLinesResolver;
2121
import datadog.trace.civisibility.source.SourcePathResolver;
22+
import datadog.trace.civisibility.source.SourceResolutionException;
2223
import datadog.trace.civisibility.utils.SpanUtils;
2324
import java.lang.reflect.Method;
2425
import java.util.function.Consumer;
2526
import javax.annotation.Nullable;
27+
import org.slf4j.Logger;
28+
import org.slf4j.LoggerFactory;
2629

2730
public class TestSuiteImpl implements DDTestSuite {
2831

32+
private static final Logger log = LoggerFactory.getLogger(TestSuiteImpl.class);
33+
2934
private final AgentSpan.Context moduleSpanContext;
3035
private final AgentSpan span;
3136
private final String moduleName;
@@ -106,13 +111,9 @@ public TestSuiteImpl(
106111
span.setTag(Tags.TEST_STATUS, TestStatus.skip);
107112

108113
this.testClass = testClass;
109-
if (this.testClass != null) {
110-
if (config.isCiVisibilitySourceDataEnabled()) {
111-
String sourcePath = sourcePathResolver.getSourcePath(testClass);
112-
if (sourcePath != null && !sourcePath.isEmpty()) {
113-
span.setTag(Tags.TEST_SOURCE_FILE, sourcePath);
114-
}
115-
}
114+
115+
if (config.isCiVisibilitySourceDataEnabled()) {
116+
populateSourceDataTags(testClass, sourcePathResolver);
116117
}
117118

118119
testDecorator.afterStart(span);
@@ -129,6 +130,20 @@ public TestSuiteImpl(
129130
}
130131
}
131132

133+
private void populateSourceDataTags(Class<?> testClass, SourcePathResolver sourcePathResolver) {
134+
if (this.testClass == null) {
135+
return;
136+
}
137+
try {
138+
String sourcePath = sourcePathResolver.getSourcePath(testClass);
139+
if (sourcePath != null && !sourcePath.isEmpty()) {
140+
span.setTag(Tags.TEST_SOURCE_FILE, sourcePath);
141+
}
142+
} catch (SourceResolutionException e) {
143+
log.debug("Could not populate source path for {}", testClass, e);
144+
}
145+
}
146+
132147
@Override
133148
public void setTag(String key, Object value) {
134149
span.setTag(key, value);

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/BestEffortSourcePathResolver.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ public BestEffortSourcePathResolver(SourcePathResolver... delegates) {
1313

1414
@Nullable
1515
@Override
16-
public String getSourcePath(@Nonnull Class<?> c) {
16+
public String getSourcePath(@Nonnull Class<?> c) throws SourceResolutionException {
1717
for (SourcePathResolver delegate : delegates) {
1818
String sourcePath = delegate.getSourcePath(c);
1919
if (sourcePath != null) {
@@ -25,7 +25,7 @@ public String getSourcePath(@Nonnull Class<?> c) {
2525

2626
@Nullable
2727
@Override
28-
public String getResourcePath(@Nullable String relativePath) {
28+
public String getResourcePath(@Nullable String relativePath) throws SourceResolutionException {
2929
for (SourcePathResolver delegate : delegates) {
3030
String resourcePath = delegate.getResourcePath(relativePath);
3131
if (resourcePath != null) {

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/SourcePathResolver.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@ public interface SourcePathResolver {
99
* root. {@code null} is returned if the path could not be resolved
1010
*/
1111
@Nullable
12-
String getSourcePath(@Nonnull Class<?> c);
12+
String getSourcePath(@Nonnull Class<?> c) throws SourceResolutionException;
1313

1414
/**
1515
* @param relativePath Path to a resource in current run's repository, relative to a resource root
1616
* @return Path relative to repository root
1717
*/
1818
@Nullable
19-
String getResourcePath(@Nullable String relativePath);
19+
String getResourcePath(@Nullable String relativePath) throws SourceResolutionException;
2020
}

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/SourceResolutionException.java

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
package datadog.trace.civisibility.source;
2+
3+
public class SourceResolutionException extends Exception {
4+
5+
public SourceResolutionException(String message) {
6+
super(message);
7+
}
8+
}

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndex.java

+28-7
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package datadog.trace.civisibility.source.index;
22

3+
import datadog.trace.api.Config;
34
import datadog.trace.api.civisibility.domain.Language;
45
import datadog.trace.civisibility.ipc.Serializer;
6+
import datadog.trace.civisibility.source.SourceResolutionException;
57
import datadog.trace.civisibility.source.Utils;
68
import datadog.trace.util.ClassNameTrie;
79
import java.io.ByteArrayInputStream;
@@ -11,6 +13,7 @@
1113
import java.io.File;
1214
import java.io.IOException;
1315
import java.nio.ByteBuffer;
16+
import java.util.Collection;
1417
import java.util.Collections;
1518
import java.util.List;
1619
import java.util.Objects;
@@ -23,16 +26,25 @@ public class RepoIndex {
2326

2427
static final RepoIndex EMPTY =
2528
new RepoIndex(
26-
ClassNameTrie.Builder.EMPTY_TRIE, Collections.emptyList(), Collections.emptyList());
29+
ClassNameTrie.Builder.EMPTY_TRIE,
30+
Collections.emptyList(),
31+
Collections.emptyList(),
32+
Collections.emptyList());
2733

2834
private static final Logger log = LoggerFactory.getLogger(RepoIndex.class);
2935

3036
private final ClassNameTrie trie;
37+
private final Collection<String> duplicateTrieKeys;
3138
private final List<SourceRoot> sourceRoots;
3239
private final List<String> rootPackages;
3340

34-
RepoIndex(ClassNameTrie trie, List<SourceRoot> sourceRoots, List<String> rootPackages) {
41+
RepoIndex(
42+
ClassNameTrie trie,
43+
Collection<String> duplicateTrieKeys,
44+
List<SourceRoot> sourceRoots,
45+
List<String> rootPackages) {
3546
this.trie = trie;
47+
this.duplicateTrieKeys = duplicateTrieKeys;
3648
this.sourceRoots = sourceRoots;
3749
this.rootPackages = rootPackages;
3850
}
@@ -42,7 +54,7 @@ public List<String> getRootPackages() {
4254
}
4355

4456
@Nullable
45-
public String getSourcePath(@Nonnull Class<?> c) {
57+
public String getSourcePath(@Nonnull Class<?> c) throws SourceResolutionException {
4658
String topLevelClassName = Utils.stripNestedClassNames(c.getName());
4759
String sourcePath = doGetSourcePath(topLevelClassName);
4860
return sourcePath != null ? sourcePath : getFallbackSourcePath(c);
@@ -54,7 +66,7 @@ public String getSourcePath(@Nonnull Class<?> c) {
5466
* retrieved from the bytecode.
5567
*/
5668
@Nullable
57-
private String getFallbackSourcePath(@Nonnull Class<?> c) {
69+
private String getFallbackSourcePath(@Nonnull Class<?> c) throws SourceResolutionException {
5870
try {
5971
String fileName = Utils.getFileName(c);
6072
if (fileName == null) {
@@ -75,7 +87,8 @@ private String getFallbackSourcePath(@Nonnull Class<?> c) {
7587
}
7688

7789
@Nullable
78-
public String getSourcePath(@Nullable String pathRelativeToSourceRoot) {
90+
public String getSourcePath(@Nullable String pathRelativeToSourceRoot)
91+
throws SourceResolutionException {
7992
if (pathRelativeToSourceRoot == null) {
8093
return null;
8194
}
@@ -84,7 +97,13 @@ public String getSourcePath(@Nullable String pathRelativeToSourceRoot) {
8497
}
8598

8699
@Nullable
87-
private String doGetSourcePath(String key) {
100+
private String doGetSourcePath(String key) throws SourceResolutionException {
101+
if (Config.get().isCiVisibilityRepoIndexDuplicateKeyCheckEnabled()) {
102+
if (!duplicateTrieKeys.isEmpty() && duplicateTrieKeys.contains(key)) {
103+
throw new SourceResolutionException("There are multiple repo index entries for " + key);
104+
}
105+
}
106+
88107
int sourceRootIdx = trie.apply(key);
89108
if (sourceRootIdx < 0) {
90109
log.debug("Could not find source root for {}", key);
@@ -110,6 +129,7 @@ public ByteBuffer serialize() {
110129

111130
Serializer s = new Serializer();
112131
s.write(serializedTrie);
132+
s.write(duplicateTrieKeys);
113133
s.write(sourceRoots, SourceRoot::serialize);
114134
s.write(rootPackages);
115135
return s.flush();
@@ -132,9 +152,10 @@ public static RepoIndex deserialize(ByteBuffer buffer) {
132152
}
133153
}
134154

155+
Collection<String> duplicateTrieKeys = Serializer.readSet(buffer, Serializer::readString);
135156
List<SourceRoot> sourceRoots = Serializer.readList(buffer, SourceRoot::deserialize);
136157
List<String> rootPackages = Serializer.readStringList(buffer);
137-
return new RepoIndex(trie, sourceRoots, rootPackages);
158+
return new RepoIndex(trie, duplicateTrieKeys, sourceRoots, rootPackages);
138159
}
139160

140161
static final class SourceRoot {

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexBuilder.java

+14-1
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@
1313
import java.nio.file.Path;
1414
import java.nio.file.attribute.BasicFileAttributes;
1515
import java.util.Arrays;
16+
import java.util.Collection;
1617
import java.util.EnumSet;
1718
import java.util.HashMap;
19+
import java.util.HashSet;
1820
import java.util.Map;
1921
import java.util.concurrent.atomic.AtomicInteger;
2022
import org.slf4j.Logger;
@@ -94,6 +96,8 @@ private static final class RepoIndexingFileVisitor implements FileVisitor<Path>
9496
private final PackageResolver packageResolver;
9597
private final ResourceResolver resourceResolver;
9698
private final ClassNameTrie.Builder trieBuilder;
99+
private final Map<String, String> trieKeyToPath;
100+
private final Collection<String> duplicateTrieKeys;
97101
private final Map<RepoIndex.SourceRoot, Integer> sourceRoots;
98102
private final PackageTree packageTree;
99103
private final RepoIndexingStats indexingStats;
@@ -109,6 +113,8 @@ private RepoIndexingFileVisitor(
109113
this.resourceResolver = resourceResolver;
110114
this.repoRoot = repoRoot;
111115
trieBuilder = new ClassNameTrie.Builder();
116+
trieKeyToPath = new HashMap<>();
117+
duplicateTrieKeys = new HashSet<>();
112118
sourceRoots = new HashMap<>();
113119
packageTree = new PackageTree(config);
114120
indexingStats = new RepoIndexingStats();
@@ -161,6 +167,12 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
161167
if (!relativePath.isEmpty()) {
162168
String key = Utils.toTrieKey(relativePath);
163169
trieBuilder.put(key, sourceRootIdx);
170+
171+
String existingPath = trieKeyToPath.put(key, file.toString());
172+
if (existingPath != null) {
173+
log.warn("Duplicate repo index key: {} - {}", existingPath, file);
174+
duplicateTrieKeys.add(key);
175+
}
164176
}
165177
}
166178
} catch (Exception e) {
@@ -221,7 +233,8 @@ public RepoIndex getIndex() {
221233
roots[e.getValue()] = e.getKey();
222234
}
223235

224-
return new RepoIndex(trieBuilder.buildTrie(), Arrays.asList(roots), packageTree.asList());
236+
return new RepoIndex(
237+
trieBuilder.buildTrie(), duplicateTrieKeys, Arrays.asList(roots), packageTree.asList());
225238
}
226239
}
227240

0 commit comments

Comments
 (0)