Don't create JSON if we don't need it #1516

maxjeffos · 2020-11-05T21:00:53Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Two important things wrt generating the Snyk-format JSON or SARIF-format JSON output from the test command:

don't do JSON.stringify at all if we don't need it
produce only the JSON formats we need: if we need Snyk-format JSON then generate that; if we need SARIF-format JSON, then generate that. If we need both (ex if the user uses --json and --sarif-file-output), then produce both.

We're also try/catching the JSON.stringify for the test output and, if it throws, we'll automatically try again without pretty-print. This may help in some scenarios where JSON.stringify is failing with RangeError: Invalid string length. Other JSON.stringify calls are likely to be tiny by comparison so we don't really need this for those, but we could consider it in the future.

Any background context you want to provide?

@lili2311 brought this up because a customer was having an issue related to failing to stringify JSON and found that we're creating the JSON output even when we don't need it and now that we're (optionally) doing SARIF output we're creating JSON twice - once for the regular Snyk format and again for the SARIF format.

What are the relevant tickets?

HAMMER-182

JackuB · 2020-11-09T16:41:55Z

test/format-test-results.spec.ts

+    it('should not create any JSON unless it is needed per options', () => {
+      const options = {} as Options;
+      const jsonStringifySpy = jest.spyOn(JSON, 'stringify');
+      extractDataToSendFromResults({}, {}, options);


Would it make sense to also check that the un/expected return values are unset - ''? Or that's covered by another test?

I updated the tests... had to add a couple fixtures to do this right. LMK what you think @JackuB I'll wait before merging.

orsagie · 2020-11-09T19:33:09Z

test/json.spec.ts

+describe('jsonStringifyLargeObject', () => {
+  it('works normally with a small object', () => {
+    const smallObject = {
+      name: 'Brian',


Suggested change

name: 'Brian',

name: 'Mozart',

Is there a small dog named Mozart @orsagie ?

orsagie · 2020-11-09T19:38:41Z

src/lib/json.ts

@@ -0,0 +1,20 @@
+const debug = require('debug')('snyk-json');


Considering this utility is purely for formatting output, I think it should sit next to the rest of the formatters src/cli/commands/test/formatters

We were thinking we'd probably use it for other parts of the code outside of formatters eventually, so I think it would be good to keep in its own file.

github-actions · 2020-11-09T21:05:56Z

	Warnings
⚠️	Looks like you added a new Tap test. Consider making it a Jest test instead. See files like `test/*.spec.ts` for examples. Files found: `test/fixtures/basic-npm/jsonData.json` `test/fixtures/basic-npm/results.json`

Generated by 🚫 dangerJS against f6d7217

maxjeffos · 2020-11-09T22:30:32Z

src/cli/commands/test/formatters/format-test-results.ts

-    : stringifiedJsonData;
+
+  let stringifiedData = '';
+  if (options.sarif || options.json) {


@SarahU you were last in here, so just wanted to make sure you were aware of this change. I'm pretty sure we don't need any value in stringifiedData unless --json or --sarif are set. The other two outputs - stringifiedJsonData and stringifiedSarifData are for when we want to save the JSON directly to a file using either --json-file-output or --sarif-file-output, respectively. @orkamara you may be interested in this as well.
This doesn't change the output for any variants of the flags I mentioned, just makes the code a bit more logical and consistent between the sarif and json options.

Hey Jeff. I'm not 100% sure. Hope it's covered by a test TBH but I assume the intention was to have it set as json as default rather that leaving it empty. I don't think that's a problem in particular unless you need it to be empty in any explicit scenario, you can probably leave it as defaulting to the Json data if you want? But if it's only used in the 2 scenarios you mention then probably fine as is. For me, I didn't want to create any changes I didn't need to as I'm not sure of all the combos of flag that can be used / are supported

Ok, sounds good @SarahU. This is quite complex and in the end I found some very weird scenario where a related test failed in CI only (could not reproduce on my mac) and Node v10. So I went with the original logic here just to be safe. I did quite a bit of manual testing with a bunch of options variants (--json, --json-file-output, --sarif, --sarif-file-output and combinations thereof) and I added a bunch of unit tests to that function.

JackuB · 2020-11-10T13:29:26Z

test/format-test-results.spec.ts

+
+import { extractDataToSendFromResults } from '../src/cli/commands/test/formatters/format-test-results';
+import { Options } from '../src/lib/types';
+const fs = require('fs');


Why not use import like the other files?

Suggested change

const fs = require('fs');

import * as fs from 'fs';

github-actions · 2020-11-11T00:00:32Z

Expected release notes (by @maxjeffos)

fixes:
don't create JSON if we don't need it (f6d7217)
add json module with jsonStringifyLargeObject (7012caa)

I hereby acknowledge these release notes are 🥙 AWESOME 🥙

snyksec · 2020-11-11T01:54:53Z

🎉 This PR is included in version 1.425.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch from a248640 to e2deb1c Compare November 5, 2020 23:18

fix: add json module with jsonStringifyLargeObject

7012caa

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch from e2deb1c to 54e48a1 Compare November 6, 2020 18:46

maxjeffos mentioned this pull request Nov 6, 2020

Retry large object stringify without pretty print #1510

Closed

3 tasks

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch from 54e48a1 to b6db27a Compare November 6, 2020 18:48

maxjeffos marked this pull request as ready for review November 6, 2020 19:00

maxjeffos requested review from a team as code owners November 6, 2020 19:00

ghost requested review from ekbsnyk and gitphill November 6, 2020 19:00

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch 2 times, most recently from 1ab9d0a to 5b3a1d2 Compare November 9, 2020 16:01

JackuB approved these changes Nov 9, 2020

View reviewed changes

orsagie reviewed Nov 9, 2020

View reviewed changes

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch from 5b3a1d2 to 8d6ad37 Compare November 9, 2020 21:03

maxjeffos requested a review from SarahU November 9, 2020 22:04

maxjeffos commented Nov 9, 2020

View reviewed changes

JackuB approved these changes Nov 10, 2020

View reviewed changes

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch 4 times, most recently from 256193f to 3a4bd57 Compare November 10, 2020 23:20

fix: don't create JSON if we don't need it

f6d7217

maxjeffos force-pushed the fix/dont-create-json-if-you-dont-need-it branch from 3a4bd57 to f6d7217 Compare November 11, 2020 00:00

maxjeffos merged commit 312ca91 into master Nov 11, 2020

maxjeffos deleted the fix/dont-create-json-if-you-dont-need-it branch November 11, 2020 01:50

snyksec added the released label Nov 11, 2020

JackuB mentioned this pull request Jun 24, 2021

fix: handle printing of large trees when using json and print-deps flags #2055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create JSON if we don't need it #1516

Don't create JSON if we don't need it #1516

maxjeffos commented Nov 5, 2020 •

edited

JackuB Nov 9, 2020

maxjeffos Nov 9, 2020

orsagie Nov 9, 2020

maxjeffos Nov 9, 2020

orsagie Nov 9, 2020

maxjeffos Nov 9, 2020

github-actions bot commented Nov 9, 2020 •

edited

maxjeffos Nov 9, 2020

SarahU Nov 9, 2020

maxjeffos Nov 10, 2020 •

edited

JackuB Nov 10, 2020

maxjeffos Nov 10, 2020

github-actions bot commented Nov 11, 2020 •

edited by maxjeffos

snyksec commented Nov 11, 2020

		@@ -0,0 +1,20 @@
		const debug = require('debug')('snyk-json');

Don't create JSON if we don't need it #1516

Don't create JSON if we don't need it #1516

Conversation

maxjeffos commented Nov 5, 2020 • edited

What does this PR do?

Any background context you want to provide?

What are the relevant tickets?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxjeffos Nov 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2020 • edited by maxjeffos

snyksec commented Nov 11, 2020

maxjeffos commented Nov 5, 2020 •

edited

github-actions bot commented Nov 9, 2020 •

edited

maxjeffos Nov 10, 2020 •

edited

github-actions bot commented Nov 11, 2020 •

edited by maxjeffos