@W-7792085@ Filter out invalid catalog files #132
Conversation
Handle situations where the json catalogs are out of sync with the file system because the referenced file no longer exists. Skip the file and show the user a warning.
Removed unnecessary temp variable.
messages/EventKeyTemplates.js
Outdated
| "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s" | ||
| }, | ||
| "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s", | ||
| "catalogFileNotFound": "Catalog file [%s] for language [%s] was not found.", |
There was a problem hiding this comment.
I wasn't sure if Catalog file is the correct term for this. It could be a jar or xml file that has been moved or deleted.
There was a problem hiding this comment.
Not sure if a user would understand what "catalog" is. How about "file path" so that we don't worry if this is an XML or JAR?
There was a problem hiding this comment.
I agree with @rmohan20 that we should avoid using catalog in our messages. Perhaps Custom rule path?
src/lib/pmd/PmdCatalogWrapper.ts
Outdated
|
|
||
| // Iterate through the custom paths. | ||
| customPathEntries.forEach(async (paths: Set<string>, langKey: string) => { | ||
| for (const [langKey, paths] of customPathEntries) { |
There was a problem hiding this comment.
forEach doesn't work correctly with async code. This blog post has more information https://codeburst.io/javascript-async-await-with-foreach-b6ba62bbf404 I'm not sure how the code on line 108 was succeeding before, there may have been a race condition that we never noticed.
Before changing this, I was seeing indeterminate behavior when testing. I looked through the code to see if we need to fix it anywhere else, I didn't find any.
There was a problem hiding this comment.
Great find! However, I always get a bit nervous about applying for-of to objects instead of iterators. Could we use customPathEntries.entries() instead? We could rename the variable to customPathMap so we're not calling xEntries.entries().
There was a problem hiding this comment.
Good point, will change.
|
|
||
| // Expect there to be 6 parameters. | ||
| expect(params.length).to.equal(6, 'Should have been 6 parameters'); | ||
| expect(params.length).to.equal(6, `Should have been 6 parameters: ${params}`); |
There was a problem hiding this comment.
This was failing before the stub of FileHandler#exists, i added some info to indicate what the contents were.
src/lib/pmd/PmdCatalogWrapper.ts
Outdated
|
|
||
| // Iterate through the custom paths. | ||
| customPathEntries.forEach(async (paths: Set<string>, langKey: string) => { | ||
| for (const [langKey, paths] of customPathEntries) { |
There was a problem hiding this comment.
Great find! However, I always get a bit nervous about applying for-of to objects instead of iterators. Could we use customPathEntries.entries() instead? We could rename the variable to customPathMap so we're not calling xEntries.entries().
messages/EventKeyTemplates.js
Outdated
| "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s" | ||
| }, | ||
| "pmdSkippedFile": "PMD failed to evaluate against file '%s'. Message: %s", | ||
| "catalogFileNotFound": "Catalog file [%s] for language [%s] was not found.", |
There was a problem hiding this comment.
I agree with @rmohan20 that we should avoid using catalog in our messages. Perhaps Custom rule path?
|
|
||
| // Expect there to be 6 parameters. | ||
| expect(params.length).to.equal(6, 'Should have been 6 parameters'); | ||
| expect(params.length).to.equal(6, `Should have been 6 parameters: ${params}`); |
| // Spoof a CustomPathManager that claims that a custom JAR exists for plsql, using a weird alias for that language. | ||
| const customJars: Map<string, Set<string>> = new Map(); | ||
| customJars.set('Pl/SqL', new Set([irrelevantPath])); | ||
| customJars.set(LANGUAGE.PLSQL, new Set([irrelevantPath])); |
There was a problem hiding this comment.
Part of the point of the test is to see if Pl/SqL can be properly de-aliased into plsql. Using the hardcoded constant undermines that.
There was a problem hiding this comment.
Whoops, thanks, missed that. Will fix this and below.
| const customJars: Map<string, Set<string>> = new Map(); | ||
| customJars.set('pl/sql', new Set([irrelevantPath])); | ||
| customJars.set('java', new Set()); | ||
| customJars.set(LANGUAGE.PLSQL, new Set([irrelevantPath])); |
There was a problem hiding this comment.
Again, part of the point is to make sure that pl/sql gets de-aliased into plsql. So leave the string as is.
Changed error message Restore unit tests that shouldn't use constants Use .entries() to iterate over map contents
Handle situations where the json catalogs are out of sync with the file system because the referenced file no longer exists.
Skip the file and show the user a warning.