SB3 Schema/Validation #21

kchadha · 2018-03-16T01:53:03Z

This PR is an improved (less complexity, less major code changes) version of #20 which is closed in favor of this PR.

Proposed Changes

Adding an sb3 schema (this is a first draft of the schema, which is still a WIP, feedback is welcome)
Modifying the validator to validate against the sb2 and sb3 schemas to determine if the given
input is valid against either of them. Note breaking change: since the validator is now checking against two different schemas, the 'error' returned by the validate function is now an object which includes both sb2 and sb3 errors if the input failed to validate against both specifications.
A callback wrapper around the JSZip promise interface
Change to the apis of unpack function and main exported function: These will provide the validated result as 2 element array of (1) the validated input with any appended metadata (which was what was previously being returned by these functions) and (2) either the unpacked zip if one was originally provided or null if a json string or file was provided.
Unpack now also tries to convert things that are neither strings nor buffers into buffers (e.g. scratch-vm gives scratch-parser an array buffer).

Test Coverage

The existing tests have been modified to accommodate these changes as necessary. Additional tests to be added for sb3 validation and for testing the new promise functionality. Both of these have been manually tested through scratch-vm and scratch-gui.

Additional Notes

This PR is related to the following PRs, and should be merged before either of them. The changes in these other PRs depend on the changes proposed by this PR.
scratchfoundation/scratch-vm#979
scratchfoundation/scratch-gui#1621

…ratch 3.0 projects. Added sb3 schema, renamed sb2 schema, and updated validate to check given input against both sb2 and sb3 schemas. Validate appends a projectVersion to the given project with 2 or 3 if the given project is valid against the corresponding schema. Validate now returns an object containing an error message and both the sb2 and sb3 validation errors upon failure to validate against both schemas. BREAKING CHANGE: Changes to validate API and what it returns (namely, error returned by validate is no longer a string, but is now an object which contains an error message string as well as the sb2 errors and the sb3 errors). Users of this library function should expect to parse the given error to figure out why validation failed.

…ise-based interface. Refactor m Refactor unpack and main exported function to return optional zip, if originally provided, in addition to the validated project. BREAKING CHANGE: Change to main api to return originally provided zip (or null if string was provided) along with validated project, in a 2-element array.

…in api function.

Fixed up existing test coverage to accommodate the recent breaking changes.

kchadha · 2018-03-16T02:04:15Z

@colbygk Addressed your comment on #20 in this PR.

Field and input values can be strings or numbers. TopLevel property is not required of blocks. VM version property should be more permissive and follow semantic release convention.

thisandagain

This looks great. I think my only suggestion would be to add a unit test for the new zip.js module that wraps JSZip, but the rest of the test coverage looks good. Hopefully we can simplify this a bit further when we move the analysis stuff into it's own module.

cwillisf

This all looks good, though I agree with the suggestion for a unit test for lib/zip.js. The comments I made are more about potential future work than this PR in particular.

lib/sb3_schema.json

+                },
+                "md5": {
+                    "type": "string",
+                    "pattern": "^[a-fA-F0-9]{32}.[a-zA-Z]+$"


lib/validate.js

+        validationError: 'Could not parse as a valid SB2 or SB3 project.',
+        sb2Errors: validateSb2.errors,
+        sb3Errors: validateSb3.errors
+    };


rschamp

I don't think any of my comments should block merging, and are just feedback for things that we might want to adjust in the future.

index.js

+    };
+
+    // First unpack the input (need this outside of the async waterfall so that
+    // unoackedProject can be refered to again)


lib/sb3_schema.json

+                "rotationCenterY": {
+                    "type": "number"
+                },
+                "skinId": {


lib/sb3_schema.json

+                "sampleCount": {
+                    "type": "integer"
+                },
+                "soundID": {


lib/sb3_schema.json

+                }
+            }
+        },
+        "block": {


lib/sb3_schema.json

+            ]
+        },
+
+        "targets": {


…dd descriptive error message to Added unit tests for unzip and added a user-friendly error message for unzip being unable to perform its duties of extracting a project.json from the given input (zip).

… from sb3 schema that aren't ne

…in sb3 schema to reflect the ac

kchadha · 2018-03-21T21:29:37Z

@rschamp, @cwillisf, @thisandagain,

I believe I have now made changes (or comments) that address your PR comments. I think this PR (or just those changes) needs a second glance before this PR is ready to go.

rschamp

👍

cwillisf

😎

kchadha added 5 commits March 15, 2018 18:00

docs($index): Documentation for new zip returning functionality of ma…

7580264

…in api function.

refactor($index): Selectively call analyze on 2.0 projects only.

b4cfddc

test($test): Fix up and add to existing tests.

11b186e

Fixed up existing test coverage to accommodate the recent breaking changes.

kchadha added the pr - needs review label Mar 16, 2018

kchadha requested review from colbygk, thisandagain, rschamp and cwillisf March 16, 2018 01:53

kchadha mentioned this pull request Mar 16, 2018

SB3 Schema/Validation and Refactor to support promises #20

Closed

refactor($sb3_schema): Allow capital letters in asset id ane md5

25e8e54

This was referenced Mar 16, 2018

SB3 Serialization & Load Project scratchfoundation/scratch-vm#979

Merged

Load Button Loading/Error State scratchfoundation/scratch-gui#1621

Merged

refactor($sb3_schema): Fix up schema to be more permissive.

03ca327

Field and input values can be strings or numbers. TopLevel property is not required of blocks. VM version property should be more permissive and follow semantic release convention.

rschamp mentioned this pull request Mar 16, 2018

Serialize sb3 blocks section to a more compact format #22

Closed

thisandagain approved these changes Mar 18, 2018

View reviewed changes

thisandagain assigned thisandagain and kchadha Mar 18, 2018

thisandagain added pr - good to merge and removed pr - needs review labels Mar 18, 2018

cwillisf approved these changes Mar 19, 2018

View reviewed changes

rschamp approved these changes Mar 20, 2018

View reviewed changes

kchadha added 3 commits March 20, 2018 12:59

test($lib/zip, $lib/unzip, $test): Rename zip to unzip for clarity. A…

767bb44

…dd descriptive error message to Added unit tests for unzip and added a user-friendly error message for unzip being unable to perform its duties of extracting a project.json from the given input (zip).

refactor($lib/sb3_schema $index (comment)): Remove runtime properties…

a8bfece

… from sb3 schema that aren't ne

refactor($lib/sb3_schema): Replace 'md5' property name with 'md5ext' …

d3f6367

…in sb3 schema to reflect the ac

kchadha added pr - needs review and removed pr - good to merge labels Mar 21, 2018

kchadha assigned rschamp and cwillisf Mar 21, 2018

rschamp approved these changes Mar 22, 2018

View reviewed changes

cwillisf approved these changes Mar 22, 2018

View reviewed changes

kchadha merged commit ee506e8 into scratchfoundation:master Mar 22, 2018

SB3 Schema/Validation #21

SB3 Schema/Validation #21

Uh oh!

Conversation

kchadha commented Mar 16, 2018

Proposed Changes

Test Coverage

Additional Notes

Uh oh!

kchadha commented Mar 16, 2018

Uh oh!

thisandagain left a comment

Choose a reason for hiding this comment

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

kchadha commented Mar 21, 2018

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!