Skip to content
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

Catch errors when parsing invalid JSON #2446

Merged
merged 1 commit into from
Dec 11, 2020
Merged

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Nov 3, 2020

Fix

Fixes: #1105

If you import an invalid JSON file it would fail silently. This ensures that we catch errors when JSON is invalid. I added the typing and default values because when there is an error we are passing undefined to <ImportProgress>.

Test

  1. Have a text file, add a json extension
  2. Try and import it
  3. See it fail and give you an error message

Release

Fix: Show error message when trying to import invalid JSON.

@belcherj belcherj added this to the 2.1.0 ❄️ milestone Nov 3, 2020
@belcherj belcherj requested a review from codebykat November 3, 2020 01:13
@belcherj belcherj self-assigned this Nov 3, 2020
@belcherj belcherj changed the title Catch errors with parsing invalid JSON Catch errors when parsing invalid JSON Nov 3, 2020
@belcherj belcherj modified the milestones: 2.1.0 ❄️, 2.2.0 Nov 3, 2020
} catch (error) {
this.emit('status', 'error', 'Invalid json file.');
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we're at it, since we still leave open the possibility of runtime crashes after this try/catch maybe we should consider adding a real parser to the input documents so that we only allow valid documents or valid lists of notes.

const parseJson = (input: string, default: unknown) => {
	try {
		return JSON.parse(input);
	} catch (e) {
		return default;
	}
}

const parseNote = (data: unknown, defaults: Partial<T.Note>) => {
	if (null === data || 'object' !== typeof data) {h
		return null;
	}

	

	return {
		...defaults,,
		modificationDate: convertDate(data.modificationDate || data.lastModified),}
}

const parseExportData = (input: string) => {
	const data = parseJson(input, null);
	if (null === data || 'object' !== typeof data) {
		return null;
	}

	const {activeNotes, trashedNotes} = data;
	if ((activeNotes && !Array.isArray(activeNotes)) || (trashedNotes && !Array.isArray(trashedNotes))) {
		return null;
	}

	return [
		(activeNotes || []).map(note => parseNote(note, {isTrashed: false})).filter(a => null !== a),
		(trashedNotes || []).map(note => parseNote(note, {isTrashed: true})).filter(a => null !== a)
	]
}

can't remember if isTrashed is the right property of the note. point is, we can do a full parse and prevent errors from creeping deeper into the app if we eliminate them here. we had this problem before with a very old account whose note object didn't contain the expected properties and the app failed to load - whitescreened it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to address this in another PR as we want to get this fix in before Friday and I am constrained on time this week.

Copy link
Member

@codebykat codebykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still crashes for me trying to import a random .json file (not a Simplenote export). Is that expected?

Screen Shot 2020-11-11 at 12 15 06 PM

Confirmed that it fails on a text file renamed to json.

@belcherj
Copy link
Contributor Author

Yeah, Dennis' comment should probably be addressed to fix that.

@codebykat
Copy link
Member

I'm okay with that being a follow-up PR too, this fixes one potential error out of two. It seems to me at first thought that it's far more likely that someone would import an actual JSON file that isn't the right format, than that they would import a non-JSON file with a JSON extension.

@codebykat
Copy link
Member

Merging this, let's follow up on the JSON validation here: #2517

@codebykat codebykat merged commit 2197b50 into develop Dec 11, 2020
@codebykat codebykat deleted the fix/import-invalid-json branch December 11, 2020 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No error is shown when user attempts to import invalid JSON file
3 participants