Conversation
SummaryThis PR adds end-to-end CSS S2260 (parsing error) support by tracking the language that produced a parsing error and routing it to the appropriate rule repository. The change flows through: JS/TS bridge (detecting CSS via file extensions), gRPC transformation (mapping errors to language-specific repos), and Java analysis (resolving rule keys by parsing error language). Rule counts increase by 1 for each language (JS: 479→480, TS: 496→497, CSS: 29→30). What reviewers should knowCritical precondition: This PR depends on merging SonarSource/rspec#6373 first. The spec.sha pin allows CI to resolve the correct rspec revision while that PR is pending — remove it before merging this PR. Key areas to review:
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
...javascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/AnalysisProcessor.java
Outdated
Show resolved
Hide resolved
...javascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/AnalysisProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
The new commit correctly switches the fast-QA parsing-error test from log-level assertions to issue-based assertions (css:S2260). The change itself is clean. However, both previously flagged issues in AnalysisProcessor.java remain unaddressed and still need to be fixed before merge.
| code: ErrorCode, | ||
| language?: ParsingErrorLanguage, | ||
| ): ParsingErrorLanguage { | ||
| if (language !== undefined) { |
There was a problem hiding this comment.
if (language) is not enough?
| function inferLanguageFromFilePath(filePath: unknown): ParsingErrorLanguage | undefined { | ||
| if (typeof filePath !== 'string') { | ||
| return undefined; | ||
| } | ||
| const path = filePath.toLowerCase(); | ||
| if ( | ||
| path.endsWith('.css') || | ||
| path.endsWith('.scss') || | ||
| path.endsWith('.sass') || | ||
| path.endsWith('.less') | ||
| ) { | ||
| return 'css'; | ||
| } | ||
| if ( | ||
| path.endsWith('.ts') || | ||
| path.endsWith('.tsx') || | ||
| path.endsWith('.cts') || | ||
| path.endsWith('.mts') | ||
| ) { | ||
| return 'ts'; | ||
| } | ||
| return 'js'; | ||
| } |
There was a problem hiding this comment.
how do you get coverage on this code? Also, falling back to js is meh, maybe should log if unexpected, so that at least we see in logs if there is something we don't expect
| error: { | ||
| message: string; | ||
| code: ErrorCode; | ||
| data?: { line: number }; | ||
| }, |
There was a problem hiding this comment.
you don't wanna name this type?
| message: string; | ||
| code: ErrorCode; | ||
| line: number | undefined; | ||
| language: ParsingErrorLanguage; |
There was a problem hiding this comment.
Isn't this ParsingError from packages/jsts/src/analysis/projectAnalysis/projectAnalysis.ts ?
| if ('parsingError' in fileResult) { | ||
| expect(fileResult.parsingError!.code).toBe(ErrorCode.Parsing); | ||
| expect(fileResult.parsingError!.line).toBe(3); | ||
| expect('parsingErrors' in fileResult).toBe(true); |
There was a problem hiding this comment.
if you'd do an assert, the follwoing if would be unnecessary
| // CSS parsing errors are expected for certain preprocessor files (e.g. Sass) | ||
| // and should not abort the analysis in failFast mode. | ||
| LOG.warn("Failed to analyze CSS file [{}]: {}", file, message); |
There was a problem hiding this comment.
this seems unnecessary. We are unifying to simplify the code. I don't think there should be a special case here
| @Test | ||
| void should_raise_css_parsing_error_issue_when_language_is_css() { | ||
| var fileLinesContextFactory = mock(FileLinesContextFactory.class); | ||
| when(fileLinesContextFactory.createFor(any())).thenReturn(mock(FileLinesContext.class)); | ||
| var cssRules = mock(CssRules.class); | ||
| when(cssRules.getActiveSonarKey("CssSyntaxError")).thenReturn(RuleKey.of("css", "S2260")); | ||
| var processor = new AnalysisProcessor( | ||
| mock(NoSonarFilter.class), | ||
| fileLinesContextFactory, | ||
| cssRules | ||
| ); | ||
| var checks = mock(JsTsChecks.class); | ||
| when(checks.parsingErrorRuleKey()).thenReturn(RuleKey.of("javascript", "S2260")); | ||
|
|
||
| var context = new JsTsContext<SensorContextTester>(SensorContextTester.create(baseDir)); | ||
| var file = TestInputFileBuilder.create("moduleKey", "file.css") | ||
| .setLanguage("css") | ||
| .setContents("a {\n color: red;\n") | ||
| .build(); | ||
|
|
||
| var response = new AnalysisResponse( | ||
| List.of(new ParsingError("Unclosed block", 2, ParsingErrorCode.PARSING, "css")), | ||
| List.of(), | ||
| List.of(), | ||
| List.of(), | ||
| new Metrics(), | ||
| List.of(), | ||
| null | ||
| ); | ||
|
|
||
| processor.processResponse(context, checks, file, response); | ||
|
|
||
| assertThat(context.getSensorContext().allIssues()).hasSize(1); | ||
| assertThat(context.getSensorContext().allIssues().iterator().next().ruleKey()).isEqualTo( | ||
| RuleKey.of("css", "S2260") | ||
| ); | ||
| } | ||
|
|
||
| @Test | ||
| void should_resolve_parsing_error_rule_key_using_parsing_error_language() { | ||
| var fileLinesContextFactory = mock(FileLinesContextFactory.class); | ||
| when(fileLinesContextFactory.createFor(any())).thenReturn(mock(FileLinesContext.class)); | ||
| var cssRules = mock(CssRules.class); | ||
| when(cssRules.getActiveSonarKey("CssSyntaxError")).thenReturn(RuleKey.of("css", "S2260")); | ||
| var processor = new AnalysisProcessor( | ||
| mock(NoSonarFilter.class), | ||
| fileLinesContextFactory, | ||
| cssRules | ||
| ); | ||
| var checks = mock(JsTsChecks.class); | ||
| when(checks.parsingErrorRuleKey(Language.JAVASCRIPT)).thenReturn( | ||
| RuleKey.of("javascript", "S2260") | ||
| ); | ||
| when(checks.parsingErrorRuleKey()).thenReturn(RuleKey.of("javascript", "S2260")); | ||
|
|
||
| var context = new JsTsContext<SensorContextTester>(SensorContextTester.create(baseDir)); | ||
| var file = TestInputFileBuilder.create("moduleKey", "file.html") | ||
| .setLanguage("web") | ||
| .setContents("<script>\nconst a = ;\n</script>") | ||
| .build(); | ||
|
|
||
| var response = new AnalysisResponse( | ||
| List.of(new ParsingError("Unexpected token", 2, ParsingErrorCode.PARSING, "js")), | ||
| List.of(), | ||
| List.of(), | ||
| List.of(), | ||
| new Metrics(), | ||
| List.of(), | ||
| null | ||
| ); | ||
|
|
||
| processor.processResponse(context, checks, file, response); | ||
|
|
||
| assertThat(context.getSensorContext().allIssues()).hasSize(1); | ||
| assertThat(context.getSensorContext().allIssues().iterator().next().ruleKey()).isEqualTo( | ||
| RuleKey.of("javascript", "S2260") | ||
| ); | ||
| } | ||
|
|
||
| @Test | ||
| void should_resolve_css_parsing_error_in_html_using_css_rule_key() { | ||
| var fileLinesContextFactory = mock(FileLinesContextFactory.class); | ||
| when(fileLinesContextFactory.createFor(any())).thenReturn(mock(FileLinesContext.class)); | ||
| var cssRules = mock(CssRules.class); | ||
| when(cssRules.getActiveSonarKey("CssSyntaxError")).thenReturn(RuleKey.of("css", "S2260")); | ||
| var processor = new AnalysisProcessor( | ||
| mock(NoSonarFilter.class), | ||
| fileLinesContextFactory, | ||
| cssRules | ||
| ); | ||
| var checks = mock(JsTsChecks.class); | ||
| when(checks.parsingErrorRuleKey(Language.JAVASCRIPT)).thenReturn( | ||
| RuleKey.of("javascript", "S2260") | ||
| ); | ||
| when(checks.parsingErrorRuleKey()).thenReturn(RuleKey.of("javascript", "S2260")); | ||
|
|
||
| var context = new JsTsContext<SensorContextTester>(SensorContextTester.create(baseDir)); | ||
| var file = TestInputFileBuilder.create("moduleKey", "file.html") | ||
| .setLanguage("web") | ||
| .setContents("<style>\na {\n</style>") | ||
| .build(); | ||
|
|
||
| var response = new AnalysisResponse( | ||
| List.of(new ParsingError("Unclosed block", 2, ParsingErrorCode.PARSING, "css")), | ||
| List.of(), | ||
| List.of(), | ||
| List.of(), | ||
| new Metrics(), | ||
| List.of(), | ||
| null | ||
| ); | ||
|
|
||
| processor.processResponse(context, checks, file, response); | ||
|
|
||
| assertThat(context.getSensorContext().allIssues()).hasSize(1); | ||
| assertThat(context.getSensorContext().allIssues().iterator().next().ruleKey()).isEqualTo( | ||
| RuleKey.of("css", "S2260") | ||
| ); | ||
| } | ||
|
|
||
| @Test | ||
| void should_process_multiple_parsing_errors_for_the_same_file() { | ||
| var fileLinesContextFactory = mock(FileLinesContextFactory.class); | ||
| when(fileLinesContextFactory.createFor(any())).thenReturn(mock(FileLinesContext.class)); | ||
| var cssRules = mock(CssRules.class); | ||
| when(cssRules.getActiveSonarKey("CssSyntaxError")).thenReturn(RuleKey.of("css", "S2260")); | ||
| var processor = new AnalysisProcessor( | ||
| mock(NoSonarFilter.class), | ||
| fileLinesContextFactory, | ||
| cssRules | ||
| ); | ||
| var checks = mock(JsTsChecks.class); | ||
| when(checks.parsingErrorRuleKey(Language.JAVASCRIPT)).thenReturn( | ||
| RuleKey.of("javascript", "S2260") | ||
| ); | ||
| when(checks.parsingErrorRuleKey()).thenReturn(RuleKey.of("javascript", "S2260")); | ||
|
|
||
| var context = new JsTsContext<SensorContextTester>(SensorContextTester.create(baseDir)); | ||
| var file = TestInputFileBuilder.create("moduleKey", "file.html") | ||
| .setLanguage("web") | ||
| .setContents("<script>\nconst a = ;\n</script>\n<style>\na {\n</style>") | ||
| .build(); | ||
|
|
||
| var response = new AnalysisResponse( | ||
| List.of( | ||
| new ParsingError("Unexpected token", 2, ParsingErrorCode.PARSING, "js"), | ||
| new ParsingError("Unclosed block", 5, ParsingErrorCode.PARSING, "css") | ||
| ), | ||
| List.of(), | ||
| List.of(), | ||
| List.of(), | ||
| new Metrics(), | ||
| List.of(), | ||
| null | ||
| ); | ||
|
|
||
| processor.processResponse(context, checks, file, response); | ||
|
|
||
| assertThat(context.getSensorContext().allIssues()) | ||
| .extracting(issue -> issue.ruleKey().repository()) | ||
| .containsExactlyInAnyOrder("javascript", "css"); | ||
| } |
There was a problem hiding this comment.
I dont think we should be adding this test code here. The business logic is in the bridge. We shouldn't be testing anything here.
There was a problem hiding this comment.
Remember to remove this, PR is accepted
There was a problem hiding this comment.
Conclusion: Both previously flagged issues are resolved. The new commit is clean — the toLanguage() null guard is in place, CssRules.CSS_PARSING_ERROR_STYLELINT_KEY replaces the duplicated string, and the overall approach of moving from a singular parsingError to a parsingErrors array (to support HTML/Vue files that run both JS/TS and CSS pipelines) is well-structured and consistently applied across all callers.
There was a problem hiding this comment.
Conclusion
Solid, well-structured PR that properly wires CSS parsing errors through all layers — the design is clean and the test coverage is thorough. One real bug needs fixing before merge: the Express error middleware can silently fail to send a response when language inference throws.
🤖 Generated with GitHub Actions
…in standalone parser
# Conflicts: # sonar-plugin/standalone/src/main/java/org/sonar/plugins/javascript/standalone/StandaloneParser.java
vdiez
force-pushed
the
codex/js-1354-css-s2260-special-case
branch
from
d2a575d to
1c1ae11
Compare
|
2 participants
Ticket
Merge Preconditions (Important)
S2260).rspec.shapin to56fd355bb9da489cb56b040e926f0aa45585ba33so CI resolves the correct rspec revision while rspec#6373 is not merged.rspec.shaand regenerate/sync against the merged rspec HEAD.Summary
This implements CSS support for Sonar key
S2260(parsing errors) end-to-end, including:S2260CssSyntaxErrorDetailed Changes
1. Rule metadata deployment and Java generation
S2260as a special-case rule during resource copy:tools/deploy-rule-data.tsS2260check class mapped to stylelint keyCssSyntaxError, and include it in generatedCssRules:tools/generate-java-rule-classes.tstools/templates/java/css-rules.templateCssRulesgeneration now excludesCssSyntaxErrorfrom stylelint runtime rule list (getStylelintRules) while preserving Sonar rule-key mapping.2. Language-aware parsing errors in JS/TS/CSS pipeline
js/ts/css):packages/jsts/src/analysis/projectAnalysis/projectAnalysis.tspackages/bridge/src/errors/middleware.tspackages/jsts/src/analysis/projectAnalysis/analyzeFile.tsS2260issue repo is selected by parsing-error language:js -> javascriptts -> typescriptcss -> csspackages/grpc/src/transformers/response.ts3. Java bridge + analysis processing
sonar-plugin/bridge/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServer.javasonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/JsTsChecks.javaS2260viaCssSyntaxErrormapping,sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/analysis/AnalysisProcessor.java4. Tests
Added/updated tests covering new behavior:
packages/bridge/tests/errors/middleware.test.tspackages/grpc/tests/transformers.test.tspackages/grpc/tests/server.test.tssonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/analysis/JsTsChecksTest.javasonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/analysis/AnalysisProcessorTest.javaValidation
Executed successfully:
npm run sync-rspec-all(with pinnedrspec.sha)npx tsx --tsconfig packages/tsconfig.test.json --test packages/bridge/tests/errors/middleware.test.ts packages/grpc/tests/transformers.test.ts packages/grpc/tests/server.test.tsmvn -pl sonar-plugin/sonar-javascript-plugin -am "-Dtest=AnalysisProcessorTest,JsTsChecksTest" "-Dsurefire.failIfNoSpecifiedTests=false" testNotes
README.mdrule counts were updated by generation (count-rules) as part of the build flow.