Merge branch '7.0.x' into pr/4387

sfdcsteve · Feb 26, 2023 · 65725c1 · 65725c1
2 parents 71dbeb2 + 22871ad
commit 65725c1
Showing 10 changed files with 226 additions and 60 deletions.
diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cache/AbstractAnalysisCache.java b/pmd-core/src/main/java/net/sourceforge/pmd/cache/AbstractAnalysisCache.java
@@ -66,25 +66,36 @@ public AbstractAnalysisCache() {
     @Override
     public boolean isUpToDate(final TextDocument document) {
         try (TimedOperation ignored = TimeTracker.startOperation(TimedOperationCategory.ANALYSIS_CACHE, "up-to-date check")) {
-            // There is a new file being analyzed, prepare entry in updated cache
-            final AnalysisResult updatedResult = new AnalysisResult(document.getCheckSum(), new ArrayList<>());
-            updatedResultsCache.put(document.getPathId(), updatedResult);
-
-            // Now check the old cache
-            final AnalysisResult analysisResult = fileResultsCache.get(document.getPathId());
+            final AnalysisResult cachedResult = fileResultsCache.get(document.getPathId());
+            final AnalysisResult updatedResult;
 
             // is this a known file? has it changed?
-            final boolean result = analysisResult != null
-                && analysisResult.getFileChecksum() == updatedResult.getFileChecksum();
-
-            if (result) {
-                LOG.debug("Incremental Analysis cache HIT");
+            final boolean upToDate = cachedResult != null
+                && cachedResult.getFileChecksum() == document.getCheckSum();
+
+            if (upToDate) {
+                LOG.trace("Incremental Analysis cache HIT");
+
+                /*
+                 * Update cached violation "filename" to match the appropriate text document,
+                 * so we can honor relativized paths for the current run
+                 */
+                final String displayName = document.getDisplayName();
+                cachedResult.getViolations().forEach(v -> ((CachedRuleViolation) v).setFileDisplayName(displayName));
+
+                // copy results over
+                updatedResult = cachedResult;
             } else {
-                LOG.debug("Incremental Analysis cache MISS - {}",
-                          analysisResult != null ? "file changed" : "no previous result found");
+                LOG.trace("Incremental Analysis cache MISS - {}",
+                          cachedResult != null ? "file changed" : "no previous result found");
+
+                // New file being analyzed, create new empty entry
+                updatedResult = new AnalysisResult(document.getCheckSum(), new ArrayList<>());
             }
 
-            return result;
+            updatedResultsCache.put(document.getPathId(), updatedResult);
+
+            return upToDate;
         }
     }
 
@@ -119,7 +130,7 @@ public void checkValidity(final RuleSets ruleSets, final ClassLoader auxclassPat
             boolean cacheIsValid = cacheExists();
 
             if (cacheIsValid && ruleSets.getChecksum() != rulesetChecksum) {
-                LOG.info("Analysis cache invalidated, rulesets changed.");
+                LOG.debug("Analysis cache invalidated, rulesets changed.");
                 cacheIsValid = false;
             }
 
@@ -131,7 +142,7 @@ public void checkValidity(final RuleSets ruleSets, final ClassLoader auxclassPat
 
                 if (cacheIsValid && currentAuxClassPathChecksum != auxClassPathChecksum) {
                     // TODO some rules don't need that (in fact, some languages)
-                    LOG.info("Analysis cache invalidated, auxclasspath changed.");
+                    LOG.debug("Analysis cache invalidated, auxclasspath changed.");
                     cacheIsValid = false;
                 }
             } else {
@@ -140,7 +151,7 @@ public void checkValidity(final RuleSets ruleSets, final ClassLoader auxclassPat
 
             final long currentExecutionClassPathChecksum = FINGERPRINTER.fingerprint(getClassPathEntries());
             if (cacheIsValid && currentExecutionClassPathChecksum != executionClassPathChecksum) {
-                LOG.info("Analysis cache invalidated, execution classpath changed.");
+                LOG.debug("Analysis cache invalidated, execution classpath changed.");
                 cacheIsValid = false;
             }
 
@@ -211,19 +222,12 @@ public FileVisitResult visitFile(final Path file,
 
     @Override
     public FileAnalysisListener startFileAnalysis(TextDocument file) {
-        String fileName = file.getPathId();
-        AnalysisResult analysisResult = updatedResultsCache.get(fileName);
-        if (analysisResult == null) {
-            analysisResult = new AnalysisResult(file.getCheckSum());
-        }
-        final AnalysisResult nonNullAnalysisResult = analysisResult;
+        final String fileName = file.getPathId();
 
         return new FileAnalysisListener() {
             @Override
             public void onRuleViolation(RuleViolation violation) {
-                synchronized (nonNullAnalysisResult) {
-                    nonNullAnalysisResult.addViolation(violation);
-                }
+                updatedResultsCache.get(fileName).addViolation(violation);
             }
 
             @Override

diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cache/CachedRuleViolation.java b/pmd-core/src/main/java/net/sourceforge/pmd/cache/CachedRuleViolation.java
@@ -33,25 +33,31 @@ public final class CachedRuleViolation implements RuleViolation {
     private final CachedRuleMapper mapper;
 
     private final String description;
-    private final FileLocation location;
     private final String ruleClassName;
     private final String ruleName;
     private final String ruleTargetLanguage;
     private final Map<String, String> additionalInfo;
 
+    private FileLocation location;
+
     private CachedRuleViolation(final CachedRuleMapper mapper, final String description,
-                                final String fileName, final String ruleClassName, final String ruleName,
+                                final String filePathId, final String ruleClassName, final String ruleName,
                                 final String ruleTargetLanguage, final int beginLine, final int beginColumn,
                                 final int endLine, final int endColumn,
                                 final Map<String, String> additionalInfo) {
         this.mapper = mapper;
         this.description = description;
-        this.location = FileLocation.range(fileName, TextRange2d.range2d(beginLine, beginColumn, endLine, endColumn));
+        this.location = FileLocation.range(filePathId, TextRange2d.range2d(beginLine, beginColumn, endLine, endColumn));
         this.ruleClassName = ruleClassName;
         this.ruleName = ruleName;
         this.ruleTargetLanguage = ruleTargetLanguage;
         this.additionalInfo = additionalInfo;
     }
+
+    void setFileDisplayName(String displayName) {
+        this.location = FileLocation.range(displayName,
+                TextRange2d.range2d(getBeginLine(), getBeginColumn(), getEndLine(), getEndColumn()));
+    }
 
     @Override
     public Rule getRule() {
@@ -78,13 +84,13 @@ public Map<String, String> getAdditionalInfo() {
      * Helper method to load a {@link CachedRuleViolation} from an input stream.
      *
      * @param stream The stream from which to load the violation.
-     * @param fileName The name of the file on which this rule was reported.
+     * @param filePathId The name of the file on which this rule was reported.
      * @param mapper The mapper to be used to obtain rule instances from the active rulesets.
      * @return The loaded rule violation.
      * @throws IOException
      */
     /* package */ static CachedRuleViolation loadFromStream(final DataInputStream stream,
-            final String fileName, final CachedRuleMapper mapper) throws IOException {
+            final String filePathId, final CachedRuleMapper mapper) throws IOException {
         final String description = stream.readUTF();
         final String ruleClassName = stream.readUTF();
         final String ruleName = stream.readUTF();
@@ -94,7 +100,7 @@ public Map<String, String> getAdditionalInfo() {
         final int endLine = stream.readInt();
         final int endColumn = stream.readInt();
         final Map<String, String> additionalInfo = readAdditionalInfo(stream);
-        return new CachedRuleViolation(mapper, description, fileName, ruleClassName, ruleName, ruleTargetLanguage,
+        return new CachedRuleViolation(mapper, description, filePathId, ruleClassName, ruleName, ruleTargetLanguage,
                                        beginLine, beginColumn, endLine, endColumn, additionalInfo);
     }
 
@@ -129,7 +135,7 @@ public Map<String, String> getAdditionalInfo() {
         FileLocation location = violation.getLocation();
         stream.writeInt(location.getStartPos().getLine());
         stream.writeInt(location.getStartPos().getColumn());
-        stream.writeInt(location.getEndPos().getColumn());
+        stream.writeInt(location.getEndPos().getLine());
         stream.writeInt(location.getEndPos().getColumn());
         Map<String, String> additionalInfo = violation.getAdditionalInfo();
         stream.writeInt(additionalInfo.size());

diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cache/FileAnalysisCache.java b/pmd-core/src/main/java/net/sourceforge/pmd/cache/FileAnalysisCache.java
@@ -74,21 +74,21 @@ private void loadFromFile(final File cacheFile) {
 
                         // Cached results
                         while (inputStream.available() > 0) {
-                            final String fileName = inputStream.readUTF();
+                            final String filePathId = inputStream.readUTF();
                             final long checksum = inputStream.readLong();
 
                             final int countViolations = inputStream.readInt();
                             final List<RuleViolation> violations = new ArrayList<>(countViolations);
                             for (int i = 0; i < countViolations; i++) {
-                                violations.add(CachedRuleViolation.loadFromStream(inputStream, fileName, ruleMapper));
+                                violations.add(CachedRuleViolation.loadFromStream(inputStream, filePathId, ruleMapper));
                             }
 
-                            fileResultsCache.put(fileName, new AnalysisResult(checksum, violations));
+                            fileResultsCache.put(filePathId, new AnalysisResult(checksum, violations));
                         }
 
-                        LOG.info("Analysis cache loaded");
+                        LOG.debug("Analysis cache loaded from {}", cacheFile);
                     } else {
-                        LOG.info("Analysis cache invalidated, PMD version changed.");
+                        LOG.debug("Analysis cache invalidated, PMD version changed.");
                     }
                 } catch (final EOFException e) {
                     LOG.warn("Cache file {} is malformed, will not be used for current analysis", cacheFile.getPath());
@@ -132,7 +132,7 @@ public void persist() {
                 for (final Map.Entry<String, AnalysisResult> resultEntry : updatedResultsCache.entrySet()) {
                     final List<RuleViolation> violations = resultEntry.getValue().getViolations();
 
-                    outputStream.writeUTF(resultEntry.getKey()); // the full filename
+                    outputStream.writeUTF(resultEntry.getKey()); // the path id
                     outputStream.writeLong(resultEntry.getValue().getFileChecksum());
 
                     outputStream.writeInt(violations.size());
@@ -141,9 +141,9 @@ public void persist() {
                     }
                 }
                 if (cacheFileShouldBeCreated) {
-                    LOG.info("Analysis cache created");
+                    LOG.debug("Analysis cache created");
                 } else {
-                    LOG.info("Analysis cache updated");
+                    LOG.debug("Analysis cache updated");
                 }
             } catch (final IOException e) {
                 LOG.error("Could not persist analysis cache to file: {}", e.getMessage());

diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/document/NioTextFile.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/document/NioTextFile.java
@@ -45,7 +45,8 @@ class NioTextFile extends BaseCloseable implements TextFile {
         this.charset = charset;
         this.languageVersion = languageVersion;
         // using the URI here, that handles files inside zip archives automatically (schema "jar:file:...!/path/inside/zip")
-        this.pathId = path.toUri().toString();
+        // normalization ensures cannonical paths
+        this.pathId = path.normalize().toUri().toString();
     }
 
     @Override

diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/cache/FileAnalysisCacheTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/cache/FileAnalysisCacheTest.java
@@ -43,6 +43,8 @@
 import net.sourceforge.pmd.lang.document.TextFile;
 import net.sourceforge.pmd.lang.document.TextFileContent;
 import net.sourceforge.pmd.lang.document.TextRange2d;
+import net.sourceforge.pmd.lang.rule.ParametricRuleViolation;
+import net.sourceforge.pmd.reporting.FileAnalysisListener;
 
 class FileAnalysisCacheTest {
 
@@ -111,16 +113,19 @@ void testStoreOnUnwritableFileShouldntThrow() throws IOException {
     void testStorePersistsFilesWithViolations() throws IOException {
         final FileAnalysisCache cache = new FileAnalysisCache(newCacheFile);
         cache.checkValidity(mock(RuleSets.class), mock(ClassLoader.class));
+        final FileAnalysisListener cacheListener = cache.startFileAnalysis(sourceFile);
+
         cache.isUpToDate(sourceFile);
 
         final RuleViolation rv = mock(RuleViolation.class);
         when(rv.getFilename()).thenReturn(sourceFile.getDisplayName());
-        when(rv.getLocation()).thenReturn(FileLocation.range(sourceFile.getDisplayName(), TextRange2d.range2d(1, 2, 3, 4)));
+        final TextRange2d textLocation = TextRange2d.range2d(1, 2, 3, 4);
+        when(rv.getLocation()).thenReturn(FileLocation.range(sourceFile.getDisplayName(), textLocation));
         final net.sourceforge.pmd.Rule rule = mock(net.sourceforge.pmd.Rule.class, Mockito.RETURNS_SMART_NULLS);
         when(rule.getLanguage()).thenReturn(mock(Language.class));
         when(rv.getRule()).thenReturn(rule);
 
-        cache.startFileAnalysis(sourceFile).onRuleViolation(rv);
+        cacheListener.onRuleViolation(rv);
         cache.persist();
 
         final FileAnalysisCache reloadedCache = new FileAnalysisCache(newCacheFile);
@@ -130,8 +135,65 @@ void testStorePersistsFilesWithViolations() throws IOException {
 
         final List<RuleViolation> cachedViolations = reloadedCache.getCachedViolations(sourceFile);
         assertEquals(1, cachedViolations.size(), "Cached rule violations count mismatch");
+        final RuleViolation cachedViolation = cachedViolations.get(0);
+        assertEquals(sourceFile.getDisplayName(), cachedViolation.getFilename());
+        assertEquals(textLocation.getStartLine(), cachedViolation.getBeginLine());
+        assertEquals(textLocation.getStartColumn(), cachedViolation.getBeginColumn());
+        assertEquals(textLocation.getEndLine(), cachedViolation.getEndLine());
+        assertEquals(textLocation.getEndColumn(), cachedViolation.getEndColumn());
     }
 
+
+    @Test
+    void testDisplayNameIsRespected() throws Exception {
+        // This checks that the display name of the file is respected even if
+        // the file is assigned a different display name across runs. The path
+        // id is saved into the cache file, and the cache implementation updates the
+        // display name of the violations to match their current display name.
+
+        final net.sourceforge.pmd.Rule rule = mock(net.sourceforge.pmd.Rule.class, Mockito.RETURNS_SMART_NULLS);
+        when(rule.getLanguage()).thenReturn(mock(Language.class));
+
+        final TextRange2d textLocation = TextRange2d.range2d(1, 2, 3, 4);
+
+        TextFile mockFile = mock(TextFile.class);
+        when(mockFile.getDisplayName()).thenReturn("display0");
+        when(mockFile.getPathId()).thenReturn("pathId");
+        when(mockFile.getLanguageVersion()).thenReturn(dummyVersion);
+        when(mockFile.readContents()).thenReturn(TextFileContent.fromCharSeq("abc"));
+
+        final FileAnalysisCache cache = new FileAnalysisCache(newCacheFile);
+        cache.checkValidity(mock(RuleSets.class), mock(ClassLoader.class));
+
+        try (TextDocument doc0 = TextDocument.create(mockFile)) {
+            cache.isUpToDate(doc0);
+            try (FileAnalysisListener listener = cache.startFileAnalysis(doc0)) {
+                listener.onRuleViolation(new ParametricRuleViolation(rule, FileLocation.range(doc0.getDisplayName(), textLocation), "message"));
+            }
+        } finally {
+            cache.persist();
+        }
+
+        reloadWithOneViolation(mockFile);
+        // now try with another display name
+        when(mockFile.getDisplayName()).thenReturn("display2");
+        reloadWithOneViolation(mockFile);
+    }
+
+    private void reloadWithOneViolation(TextFile mockFile) throws IOException {
+        final FileAnalysisCache reloadedCache = new FileAnalysisCache(newCacheFile);
+        reloadedCache.checkValidity(mock(RuleSets.class), mock(ClassLoader.class));
+        try (TextDocument doc1 = TextDocument.create(mockFile)) {
+            assertTrue(reloadedCache.isUpToDate(doc1),
+                       "Cache believes unmodified file with violations is not up to date");
+            List<RuleViolation> cachedViolations = reloadedCache.getCachedViolations(doc1);
+            assertEquals(1, cachedViolations.size(), "Cached rule violations count mismatch");
+            final RuleViolation cachedViolation = cachedViolations.get(0);
+            assertEquals(mockFile.getDisplayName(), cachedViolation.getFilename());
+        }
+    }
+
+
     @Test
     void testCacheValidityWithNoChanges() throws IOException {
         final RuleSets rs = mock(RuleSets.class);

diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/cache/internal/ZipFileFingerprinterTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/cache/internal/ZipFileFingerprinterTest.java
@@ -40,6 +40,32 @@ void zipEntryMetadataDoesNotAffectFingerprint() throws IOException {
         assertEquals(baselineFingerprint, updateFingerprint(file));
         assertNotEquals(originalFileSize, file.length());
     }
+
+    @Test
+    void zipEntryOrderDoesNotAffectFingerprint() throws IOException {
+        final File zipFile = tempDir.resolve("foo.jar").toFile();
+        final ZipEntry fooEntry = new ZipEntry("lib/Foo.class");
+        final ZipEntry barEntry = new ZipEntry("lib/Bar.class");
+        overwriteZipFileContents(zipFile, fooEntry, barEntry);
+        final long baselineFingerprint = getBaseLineFingerprint(zipFile);
+
+        // swap order
+        overwriteZipFileContents(zipFile, barEntry, fooEntry);
+        assertEquals(baselineFingerprint, updateFingerprint(zipFile));
+    }
+
+    @Test
+    void nonClassZipEntryDoesNotAffectFingerprint() throws IOException {
+        final File zipFile = tempDir.resolve("foo.jar").toFile();
+        final ZipEntry fooEntry = new ZipEntry("lib/Foo.class");
+        final ZipEntry barEntry = new ZipEntry("bar.properties");
+        overwriteZipFileContents(zipFile, fooEntry);
+        final long baselineFingerprint = getBaseLineFingerprint(zipFile);
+
+        // add a properties file to the jar
+        overwriteZipFileContents(zipFile, fooEntry, barEntry);
+        assertEquals(baselineFingerprint, updateFingerprint(zipFile));
+    }
 
     @Override
     protected ClasspathEntryFingerprinter newFingerPrinter() {