-
Notifications
You must be signed in to change notification settings - Fork 669
feat: include errors for projects when printing JSON graph [UNIFY-1102] #6412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,51 @@ export async function printEffectiveDepGraph( | |
| }); | ||
| } | ||
|
|
||
| /** | ||
| * printEffectiveDepGraphError writes an error output for failed dependency graph resolution | ||
| * to the destination stream in a format consistent with printEffectiveDepGraph. | ||
| * This is used when --print-effective-graph-with-errors is set but dependency resolution failed. | ||
| */ | ||
| export async function printEffectiveDepGraphError( | ||
| errorTitle: string, | ||
| errorDetail: string, | ||
| normalisedTargetFile: string | undefined, | ||
| destination: Writable, | ||
| ): Promise<void> { | ||
| return new Promise((res, rej) => { | ||
| const effectiveGraphErrorOutput = { | ||
| error: { | ||
| id: 'SNYK-CLI-0000', | ||
| title: errorTitle, | ||
| detail: errorDetail, | ||
| }, | ||
| normalisedTargetFile, | ||
| }; | ||
|
|
||
| new ConcatStream( | ||
| new JsonStreamStringify(effectiveGraphErrorOutput), | ||
| Readable.from('\n'), | ||
| ) | ||
| .on('end', res) | ||
| .on('error', rej) | ||
| .pipe(destination); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if either --print-effective-graph or --print-effective-graph-with-errors is set. | ||
| */ | ||
| export function shouldPrintEffectiveDepGraph(opts: Options): boolean { | ||
| return !!opts['print-effective-graph']; | ||
| return ( | ||
| !!opts['print-effective-graph'] || | ||
| shouldPrintEffectiveDepGraphWithErrors(opts) | ||
|
Comment on lines
+170
to
+171
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * shouldPrintEffectiveDepGraphWithErrors checks if the --print-effective-graph-with-errors flag is set. | ||
| * This is used to determine if the effective dep-graph with errors should be printed. | ||
| */ | ||
| export function shouldPrintEffectiveDepGraphWithErrors(opts: Options): boolean { | ||
| return !!opts['print-effective-graph-with-errors']; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| invalid-json-syntax | ||
| "name": "invalid-project", | ||
| "version": "1.0.0" | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "name": "valid-project", | ||
| "version": "1.0.0", | ||
| "description": "A valid npm project for testing", | ||
| "main": "index.js" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "name": "with-vulnerable-lodash-dep", | ||
| "version": "1.2.3", | ||
| "description": "", | ||
| "main": "index.js", | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1" | ||
| }, | ||
| "keywords": [], | ||
| "license": "ISC", | ||
| "dependencies": { | ||
| "lodash": "4.17.15" | ||
| } | ||
| } |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,227 @@ | ||
| import { fakeServer } from '../../acceptance/fake-server'; | ||
| import { createProjectFromFixture } from '../util/createProject'; | ||
| import { runSnykCLI } from '../util/runSnykCLI'; | ||
| import { getServerPort } from '../util/getServerPort'; | ||
|
|
||
| jest.setTimeout(1000 * 30); | ||
|
|
||
| describe('`test` command with `--print-effective-graph-with-errors` option', () => { | ||
| let server; | ||
| let env: Record<string, string>; | ||
|
|
||
| beforeAll((done) => { | ||
| const port = getServerPort(process); | ||
| const baseApi = '/api/v1'; | ||
| env = { | ||
| ...process.env, | ||
| SNYK_API: 'http://localhost:' + port + baseApi, | ||
| SNYK_HOST: 'http://localhost:' + port, | ||
| SNYK_TOKEN: '123456789', | ||
| SNYK_INTEGRATION_NAME: 'JENKINS', | ||
| SNYK_INTEGRATION_VERSION: '1.2.3', | ||
| }; | ||
| server = fakeServer(baseApi, env.SNYK_TOKEN); | ||
| server.listen(port, () => { | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| server.restore(); | ||
| }); | ||
|
|
||
| afterAll((done) => { | ||
| server.close(() => { | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('works for project with no deps', async () => { | ||
| const project = await createProjectFromFixture('print-graph-no-deps'); | ||
| const { code, stdout } = await runSnykCLI( | ||
| 'test --print-effective-graph-with-errors', | ||
| { | ||
| cwd: project.path(), | ||
| env, | ||
| }, | ||
| ); | ||
|
|
||
| expect(code).toEqual(0); | ||
|
|
||
| const jsonOutput = JSON.parse(stdout); | ||
|
|
||
| expect(jsonOutput.normalisedTargetFile).toBe('package.json'); | ||
| expect(jsonOutput.depGraph).toMatchObject({ | ||
| pkgManager: { | ||
| name: 'npm', | ||
| }, | ||
| pkgs: [ | ||
| { | ||
| id: '[email protected]', | ||
| info: { | ||
| name: 'print-graph-no-deps', | ||
| version: '1.0.0', | ||
| }, | ||
| }, | ||
| ], | ||
| graph: { | ||
| rootNodeId: 'root-node', | ||
| nodes: [ | ||
| { | ||
| nodeId: 'root-node', | ||
| pkgId: '[email protected]', | ||
| deps: [], | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it('successfully outputs dep graph for single project success', async () => { | ||
| const project = await createProjectFromFixture( | ||
| 'npm/with-vulnerable-lodash-dep', | ||
| ); | ||
| server.setCustomResponse( | ||
| await project.readJSON('test-dep-graph-result.json'), | ||
| ); | ||
| const { code, stdout } = await runSnykCLI( | ||
| 'test --print-effective-graph-with-errors', | ||
| { | ||
| cwd: project.path(), | ||
| env, | ||
| }, | ||
| ); | ||
|
|
||
| expect(code).toEqual(0); | ||
|
|
||
| const jsonOutput = JSON.parse(stdout); | ||
|
|
||
| expect(jsonOutput.normalisedTargetFile).toBe('package-lock.json'); | ||
|
|
||
| expect(jsonOutput.depGraph).toMatchObject({ | ||
| pkgManager: { | ||
| name: 'npm', | ||
| }, | ||
| pkgs: [ | ||
| { | ||
| id: '[email protected]', | ||
| info: { | ||
| name: 'with-vulnerable-lodash-dep', | ||
| version: '1.2.3', | ||
| }, | ||
| }, | ||
| { | ||
| id: '[email protected]', | ||
| info: { | ||
| name: 'lodash', | ||
| version: '4.17.15', | ||
| }, | ||
| }, | ||
| ], | ||
| graph: { | ||
| rootNodeId: 'root-node', | ||
| nodes: [ | ||
| { | ||
| nodeId: 'root-node', | ||
| pkgId: '[email protected]', | ||
| deps: [ | ||
| { | ||
| nodeId: '[email protected]', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| nodeId: '[email protected]', | ||
| pkgId: '[email protected]', | ||
| deps: [], | ||
| info: { | ||
| labels: { | ||
| scope: 'prod', | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it('outputs both error JSON and dep graphs for mixed success/failure with --all-projects', async () => { | ||
| const project = await createProjectFromFixture( | ||
| 'print-graph-mixed-success-failure', | ||
| ); | ||
| server.setCustomResponse( | ||
| await project.readJSON('valid-project/test-dep-graph-result.json'), | ||
| ); | ||
| const { code, stdout, stderr } = await runSnykCLI( | ||
| 'test --all-projects --print-effective-graph-with-errors', | ||
| { | ||
| cwd: project.path(), | ||
| env, | ||
| }, | ||
| ); | ||
|
|
||
| // Should succeed (exit code 0) because at least one project succeeded | ||
| // Or exit code 2 if the error propagates | ||
| expect([0, 2]).toContain(code); | ||
|
|
||
| // Parse JSONL output | ||
| const lines = stdout | ||
| .trim() | ||
| .split('\n') | ||
| .filter((line) => line.trim()); | ||
|
|
||
| const jsonObjects: any[] = []; | ||
| for (const line of lines) { | ||
| try { | ||
| jsonObjects.push(JSON.parse(line)); | ||
| } catch { | ||
| // Skip non-JSON lines | ||
| } | ||
| } | ||
|
|
||
| // Should have at least one output (either success or error) | ||
| expect(jsonObjects.length).toBeGreaterThan(0); | ||
|
|
||
| // Find error outputs from printEffectiveDepGraphError (has error.id field) | ||
| const errorOutputs = jsonObjects.filter( | ||
| (obj) => | ||
| obj.error !== undefined && | ||
| obj.error.id === 'SNYK-CLI-0000' && | ||
| obj.normalisedTargetFile !== undefined, | ||
| ); | ||
| // Find success outputs (dep graphs) | ||
| const successOutputs = jsonObjects.filter( | ||
| (obj) => obj.depGraph !== undefined, | ||
| ); | ||
|
|
||
| // Should have at least one error output for the invalid project | ||
| expect(errorOutputs.length).toBeGreaterThanOrEqual(1); | ||
|
|
||
| // Validate error output structure | ||
| for (const errorOutput of errorOutputs) { | ||
| expect(errorOutput.error).toHaveProperty('id', 'SNYK-CLI-0000'); | ||
| expect(errorOutput.error).toHaveProperty('title'); | ||
| expect(errorOutput.error).toHaveProperty('detail'); | ||
| expect(errorOutput).toHaveProperty('normalisedTargetFile'); | ||
| expect(errorOutput.error.title).toMatch( | ||
| /^Failed to resolve dependencies for project/, | ||
| ); | ||
| expect(errorOutput.error.title).toContain( | ||
| errorOutput.normalisedTargetFile, | ||
| ); | ||
| } | ||
|
|
||
| // Should have at least one success output for the valid project | ||
| expect(successOutputs.length).toBeGreaterThanOrEqual(1); | ||
|
|
||
| // Validate success output structure | ||
| for (const successOutput of successOutputs) { | ||
| expect(successOutput).toHaveProperty('depGraph'); | ||
| expect(successOutput).toHaveProperty('normalisedTargetFile'); | ||
| expect(successOutput.depGraph).toHaveProperty('pkgManager'); | ||
| } | ||
|
|
||
| // stderr should contain the failure warning | ||
| expect(stderr).toMatch(/failed to get dependencies/i); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is there a reason why we do always use this generic error code? Why don't we try to use additional information from the original error instance we received from the plugins. Choosing this hardcoded value here will make it more and more difficult to correctly report on errors in the future. It would even be better if the string would be empty by default, so that the decision of mapping it to generic can be done somewhere else.
I hope this makes sense. If you want we can chat a bit more. Maybe look at the CustomError on how we made it possible to transparently handle error catalog errors if available.