Add javascript code snippet validation #662

ThomasHickman · 2018-02-15T19:20:46Z

This fixes #594. Note: I refractored most of exec_js (in sandbox_js) into exec_js_process, so that I could use a function that runs a javascript process, inputs the results to stdin and captures them on stdout, while having a timeout.

Fixes common-workflow-language#594. To do this, it is needed to add jshint and refractor js_sandbox to so that it can be useful for this.

mr-c · 2018-02-15T20:02:34Z

cwltool/linting/lint.js needs to be listed in MANIFEST.in

…options-file

mr-c · 2018-03-02T17:10:23Z

cwltool/validate_js.py

+        })
+    )
+
+    def dump_jshint_error():


This needs type information

mr-c · 2018-03-02T17:12:09Z

cwltool/validate_js.py

+            return []
+
+        return list(itertools.chain(*
+            map(lambda x: get_expressions(x[1], schema.items, SourceLine(tool, x[0])), enumerate(tool))


cwltool/validate_js.py:47: error: "Schema" has no attribute "items" from https://travis-ci.org/common-workflow-language/cwltool/jobs/348306873#L618

mr-c · 2018-03-02T17:13:43Z

cwltool/load_tool.py

@@ -187,7 +188,9 @@ def validate_document(document_loader,  # type: Loader
                      fetcher_constructor=None,  # type: FetcherConstructorType
                      skip_schemas=None,  # type: bool
                      overrides=None,  # type: List[Dict]
-                      metadata=None,  # type: Optional[Dict]
+                      metadata=None,  # type: Optional[Dict],


The extra comma at the end of the type hint needs to be removed

mr-c · 2018-03-02T17:18:29Z

Jenkins, test this please

mr-c · 2018-03-02T17:24:07Z

cwltool/validate_js.py

+    linter_folder = resource_filename(__name__, "jshint")
+
+    returncode, stdout, stderr = exec_js_process(
+        path.join(linter_folder, "jshint_wrapper.js"),


Here you should use get_data from https://github.com/common-workflow-language/cwltool/blob/master/tests/util.py (maybe that should be moved to the cwltool package?)

This is why the conformance tests are failing 🙂

This reverts commit d4b5168.

mr-c · 2018-03-05T16:42:03Z

@ThomasHickman You are getting closer! :-)

ThomasHickman · 2018-03-05T16:53:10Z

@mr-c any idea what I'm still doing wrong? :)

mr-c · 2018-03-05T16:59:51Z

@ThomasHickman I haven't had a chance to try myself yet, but the next stage is to create a new virtualenv, pip install this branch into it; and then execute the CWL conformance tests, debugging any failures directly.

https://github.com/common-workflow-language/common-workflow-language/blob/master/CONFORMANCE_TESTS.md#usage with RUNNER=cwltool

mr-c · 2018-03-10T14:29:32Z

Much closer!

~/common-workflow-language/v1.0$ cwltool --debug  v1.0/template-tool.cwl v1.0/cat-job.json
/home/michael/common-workflow-language/env/bin/cwltool 1.0.20180310142018
Resolved 'v1.0/template-tool.cwl' to 'file:///home/michael/common-workflow-language/v1.0/v1.0/template-tool.cwl'
I'm sorry, I couldn't load this CWL file.  The error was:
Traceback (most recent call last):
  File "/home/michael/common-workflow-language/env/local/lib/python2.7/site-packages/cwltool/main.py", line 492, in main
    validate_js_options=validate_js_options)
  File "/home/michael/common-workflow-language/env/local/lib/python2.7/site-packages/cwltool/load_tool.py", line 288, in validate_document
    validate_js_expressions(processobj, avsc_names.names[workflowobj["class"]], validate_js_options)
  File "/home/michael/common-workflow-language/env/local/lib/python2.7/site-packages/cwltool/validate_js.py", line 170, in validate_js_expressions
    expression_lib_errors, expression_lib_globals = jshint_js("\n".join(expression_lib), default_globals, jshint_options)
  File "/home/michael/common-workflow-language/env/local/lib/python2.7/site-packages/cwltool/validate_js.py", line 112, in jshint_js
    context=jshint_functions_text
  File "/home/michael/common-workflow-language/env/local/lib/python2.7/site-packages/cwltool/sandboxjs.py", line 264, in exec_js_process
    while not process_finished() and tm.is_alive():
  File "/home/michael/common-workflow-language/env/local/lib/python2.7/site-packages/cwltool/sandboxjs.py", line 186, in process_finished
    return stdout_buf.getvalue().decode().endswith(PROCESS_FINISHED_STR) and \
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 28449: ordinal not in range(128)

mr-c · 2018-03-10T18:08:20Z

Thanks again @ThomasHickman ; this is working everywhere but Windows. Ping me once that is fixed I'll do a final review and we can get this merged!

… js-linting

ThomasHickman · 2018-03-12T12:20:54Z

@mr-c I've fixed the windows issue. Do you want to do a final review now?

Note from above: I've removed the get_data to get_test_data change as I don't need get_data in cwltool.utils and this change affects a lot of files

tetron · 2018-03-12T14:22:53Z

The reason for using resource_stream is to support delivery as a Python egg. Assuming files are on disk doesn't work when the module is stored in a zip archive (I don't know if wheels have similar behavior.)

mr-c · 2018-03-12T16:24:27Z

Yep, we know. But then it won't work if someone executes `cwltool.py`, which is a shortcut for running cwltool without installing it first. On Mar 12, 2018 15:22, "Peter Amstutz" <[email protected]> wrote: The reason for using resource_stream is to support delivery as a Python egg. Assuming files are on disk doesn't work when the module is stored in a zip archive (I don't know if wheels have similar behavior.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#662 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABROCJ_lP5ws8RvHHK8jN6qPr8dN8cefks5tdoS-gaJpZM4SHXiy> .

tetron · 2018-03-12T16:43:37Z

So if we want to be clever about it, the wrapper method should try resource_stream() first and then fall back to searching for the file?

ThomasHickman · 2018-03-12T16:51:02Z

@tetron yes, this is already implemented in the util file in the tests folder:

cwltool/tests/util.py

Line 8 in addcaa2

def get_data(filename):

Also, if we are going to modify all resource_stream functions such that you can run code in place, wouldn't it be better to implement that in a separate PR?

mr-c · 2018-03-12T18:39:54Z

Sure, the decision that needs to be made can be moved to a separate issue for discussion and (hopefully) quick implementation. @tetron, did you have any other feedback on this PR?

tetron · 2018-03-12T19:12:33Z

A few comments -

Consider moving the call to JS validation out of load_tool and into Process.__init__(), that's where other secondary validation (input/output port type checking) currently happens.
It looks like get_expressions traverse the schema and tool record in parallel. This is a good strategy but somewhat expensive, a simpler/faster strategy would be to walk the schema once to determine the set of fields that are permitted to contain expressions, and then walk the tool object searching for those fields.
If jshint finds an obvious error (syntax error, for example) it should raise a ValidationException

However I don't want to hold up merging a useful feature on these, if @mr-c has tested it and it seems to be working well, I'm okay with merging now and doing a followup cleanup PR.

ThomasHickman · 2018-03-12T20:16:08Z

@tetron Those seem fair enough comments. I also noticed while having a look at (1) that command line parameters were not getting propagated to validate_js in load_tool - the best way to solve this is to implement point (1), so this is done above

mr-c · 2018-03-12T23:12:36Z

As before, as long as follow up issues are filed so we don't completely forget them I'm fine merging this now 😁

codecov-io · 2018-03-22T19:43:20Z

Codecov Report

Merging #662 into master will increase coverage by 0.17%.
The diff coverage is 68.53%.

@@            Coverage Diff             @@
##           master     #662      +/-   ##
==========================================
+ Coverage    62.4%   62.57%   +0.17%     
==========================================
  Files          26       27       +1     
  Lines        4274     4414     +140     
  Branches     1147     1180      +33     
==========================================
+ Hits         2667     2762      +95     
- Misses       1324     1358      +34     
- Partials      283      294      +11

Impacted Files	Coverage Δ
cwltool/load_tool.py	`81.25% <ø> (ø)`	⬆️
cwltool/argparser.py	`80.88% <100%> (+0.18%)`	⬆️
cwltool/process.py	`67.93% <36.36%> (-0.62%)`	⬇️
cwltool/sandboxjs.py	`43.91% <54.83%> (-0.27%)`	⬇️
cwltool/validate_js.py	`79.61% <79.61%> (ø)`
cwltool/executors.py	`68.57% <0%> (-2.86%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1f9024...1786a7f. Read the comment docs.

mr-c · 2018-03-22T19:57:34Z

@ThomasHickman @tetron Please file follow up issues as requested

tetron · 2018-03-22T20:06:09Z

@mr-c #699 were there any others?

ThomasHickman added 3 commits February 15, 2018 19:16

Add validation for javascript expressions.

01c79e7

Fixes common-workflow-language#594. To do this, it is needed to add jshint and refractor js_sandbox to so that it can be useful for this.

Merge branch 'master' into js-linting

5182ab6

Fix unfixed merge confict

f9bcbd0

ThomasHickman added 5 commits February 15, 2018 20:32

Rename jshint files add add to MANIFEST.in

8543182

Fix issues found in testing

a320e10

Fix items in failing tests and add disable-js-validation and js-hint-…

df6fe7d

…options-file

Merge branch 'master' into js-linting

1eac465

draft2tool -> command_line_tool in validate_js

c4a6f16

mr-c requested changes Mar 2, 2018

View reviewed changes

ThomasHickman added 2 commits March 5, 2018 13:01

Move get_data from tests to cwltool

d4b5168

Fix issues found in testing

ddf4291

mr-c approved these changes Mar 5, 2018

View reviewed changes

ThomasHickman added 3 commits March 5, 2018 14:12

Merge branch 'master' into js-linting

b847c81

Revert "Move get_data from tests to cwltool" excluding utils.py

4f76771

This reverts commit d4b5168.

Add get_test_data

397711d

ThomasHickman and others added 8 commits March 5, 2018 21:39

Add folder checking and exception to get_data

4f126db

Corrected incorrect logic in get_data

b774648

Add jshint wrapper file existrance checking

149933f

Create cwlNodeEngineWithContext for jshint

1da4c43

Merge branch 'master' into js-linting

f56192c

remove unneeded import

4062fb9

include new javascript in dist

915f961

Merge branch 'master' into js-linting

5452b8a

mr-c added 2 commits March 10, 2018 19:48

add nodejs to appveyor

2d54920

use a fixed nodejs version for appveyor

530b7d0

ThomasHickman added 4 commits March 12, 2018 11:30

Try fix exec_js with context in windows

01fe931

Merge branch 'js-linting' of https://github.com/wtsi-hgi/cwltool into…

a8b1266

… js-linting

Use resource_stream for validate_js

b243a10

Revert get_data -> get_test_data

7926be4

ThomasHickman added 2 commits March 12, 2018 20:10

move validate_js_expressions to Process.__init__

7552315

Remove unused imports

5e08b88

ThomasHickman added 2 commits March 12, 2018 20:24

Correct Process type

50d9d8f

Further correct type errors

b28dfd5

Merge branch 'master' into js-linting

c295a04

tetron mentioned this pull request Mar 22, 2018

apply strict mode everywhere #627

Closed

tetron added 2 commits March 22, 2018 15:06

Merge branch 'master' into js-linting

17fa7fc

Merge branch 'master' into js-linting

1786a7f

tetron merged commit 9564713 into common-workflow-language:master Mar 22, 2018

tetron mentioned this pull request Mar 22, 2018

use get_data() consistently #699

Open

ThomasHickman deleted the js-linting branch March 22, 2018 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add javascript code snippet validation #662

Add javascript code snippet validation #662

ThomasHickman commented Feb 15, 2018

mr-c commented Feb 15, 2018

mr-c Mar 2, 2018

mr-c Mar 2, 2018

mr-c Mar 2, 2018

mr-c commented Mar 2, 2018

mr-c Mar 2, 2018

mr-c commented Mar 5, 2018

ThomasHickman commented Mar 5, 2018

mr-c commented Mar 5, 2018

mr-c commented Mar 10, 2018

mr-c commented Mar 10, 2018

ThomasHickman commented Mar 12, 2018

tetron commented Mar 12, 2018

mr-c commented Mar 12, 2018 via email

tetron commented Mar 12, 2018

ThomasHickman commented Mar 12, 2018 •

edited

Loading

mr-c commented Mar 12, 2018 via email

tetron commented Mar 12, 2018

ThomasHickman commented Mar 12, 2018

mr-c commented Mar 12, 2018

codecov-io commented Mar 22, 2018 •

edited

Loading

mr-c commented Mar 22, 2018

tetron commented Mar 22, 2018

Add javascript code snippet validation #662

Add javascript code snippet validation #662

Conversation

ThomasHickman commented Feb 15, 2018

mr-c commented Feb 15, 2018

mr-c Mar 2, 2018

Choose a reason for hiding this comment

mr-c Mar 2, 2018

Choose a reason for hiding this comment

mr-c Mar 2, 2018

Choose a reason for hiding this comment

mr-c commented Mar 2, 2018

mr-c Mar 2, 2018

Choose a reason for hiding this comment

mr-c commented Mar 5, 2018

ThomasHickman commented Mar 5, 2018

mr-c commented Mar 5, 2018

mr-c commented Mar 10, 2018

mr-c commented Mar 10, 2018

ThomasHickman commented Mar 12, 2018

tetron commented Mar 12, 2018

mr-c commented Mar 12, 2018 via email

tetron commented Mar 12, 2018

ThomasHickman commented Mar 12, 2018 • edited Loading

mr-c commented Mar 12, 2018 via email

tetron commented Mar 12, 2018

ThomasHickman commented Mar 12, 2018

mr-c commented Mar 12, 2018

codecov-io commented Mar 22, 2018 • edited Loading

Codecov Report

mr-c commented Mar 22, 2018

tetron commented Mar 22, 2018

ThomasHickman commented Mar 12, 2018 •

edited

Loading

codecov-io commented Mar 22, 2018 •

edited

Loading