additional validation for cli arguments #3894

ltalirz · 2020-04-06T07:31:00Z

fixes #3301
fixes #3596
fixes #3897
fixes #3770

Start validating
- ~~labels (Improved input validation of verdi computer setup #3596)~~
- hostnames (missing validation of command line parameters #3301)
- emails (still allow aiida@localhost)
- entry point names
- profile names
- First name, last name and institution need to be non-empty (missing validation of command line parameters #3301)
Don't skip validation if user enters !: The InteractiveOption of the CLI always allowed users to pass ! in order to avoid setting a value, bypassing any further validation that may be provided by the type. Change this behavior such that entering ! results in an empty string being passed further to the type. Only if conversion by the type passes as well should a None value be returned.
add test for "!" does not operate as described for verdi commands #3770
reduce pytest output on travis to make it more readable (coverage + test timings)
update verdi setup docs for authentication via Unix sockets
increase code reuse in click options (specifically: verdi setup/quicksetup)

Note: Since existing databases may contain data that does not validate under the new rules, we have not introduced validations for CLI options that would make it impossible to work with existing data (e.g. for the label of nodes, etc.).
Those should be made in a separate PR.

Todo:

Open issue validation of Node labels #3927 for introduction of validation of labels

sphuber

Thanks @ltalirz , few comments and suggestions

aiida/cmdline/params/options/commands/computer.py

sphuber · 2020-04-06T09:51:46Z

aiida/cmdline/params/types/identifier.py



-class IdentifierParamType(click.ParamType, ABC):
+class IdentifierParamType(AlphanumericStringType, ABC):


I don't think we can make this change. It is true that for the ID and UUID interpretation of the IdentifierParamType the alpha-numeric rules apply, however, the LABEL is currently not restricted to this. By changing this people will no longer be able to load certain entities through their label that contain non-alpha-numeric character

Right, consider this more of a "draft" PR (somehow I always realize too late when something is a draft PR and I think one cannot change it afterwards).
I wanted to discuss what the validation of labels should be - both on a global level and for specific labels (computers, codes, ...).

aiida/cmdline/params/types/strings.py

codecov · 2020-04-13T14:28:11Z

Codecov Report

Merging #3894 into develop will increase coverage by 0.00%.
The diff coverage is 85.52%.

@@           Coverage Diff            @@
##           develop    #3894   +/-   ##
========================================
  Coverage    78.20%   78.20%           
========================================
  Files          460      460           
  Lines        33982    34027   +45     
========================================
+ Hits         26574    26611   +37     
- Misses        7408     7416    +8

Flag	Coverage Δ
#django	`70.23% <81.57%> (+<0.01%)`	⬆️
#sqlalchemy	`71.09% <85.52%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
aiida/cmdline/params/types/strings.py	`79.62% <79.62%> (ø)`
aiida/cmdline/params/arguments/__init__.py	`100.00% <100.00%> (ø)`
aiida/cmdline/params/options/commands/computer.py	`85.18% <100.00%> (ø)`
aiida/cmdline/params/options/commands/setup.py	`93.90% <100.00%> (-0.08%)`	⬇️
aiida/cmdline/params/options/interactive.py	`83.47% <100.00%> (ø)`
aiida/cmdline/params/types/identifier.py	`89.13% <100.00%> (+0.24%)`	⬆️
aiida/cmdline/params/types/plugin.py	`89.58% <100.00%> (+0.22%)`	⬆️
aiida/cmdline/params/types/profile.py	`66.66% <100.00%> (+0.95%)`	⬆️
aiida/transports/plugins/local.py	`80.46% <0.00%> (+0.25%)`	⬆️

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 c74048b...6d18745. Read the comment docs.

ltalirz · 2020-04-13T15:05:18Z

@sphuber I think this is ready for review now. See the description of the PR for the list of changes.

sphuber

Thanks @ltalirz . Only two comments: the first one is really minor and I only put it there since I though you would anyway have to do a few more touchups, but it seems all is good. There is just one question about making the ProfileParamType validation more strict, which could cause (light) problems for existing profiles. Just wanted to check if you had thought about this

sphuber · 2020-04-13T16:05:07Z

.github/config/profile.yaml

@@ -1,6 +1,6 @@
 ---
 profile: PLACEHOLDER_PROFILE
-email: aiida@localhost


Not crucial, but since we decided to keep this as allowed, maybe revert it

sphuber · 2020-04-13T16:17:15Z

aiida/cmdline/params/types/profile.py



-class ProfileParamType(click.ParamType):
+class ProfileParamType(LabelStringType):


Here we are making the rules more strict. There might be profiles out there that don't respect the rules of LabelStringType. Those will no longer be able to be used in the CLI. Of course, the profile name can easily be adapted manually in the config.json, just wanted to check if you thought about this. If you are ok with doing this, then also fine by me

I'm aware, but I think this change is still reasonable. My arguments:

as you say, it's something that can be easily fixed (and it's probably in the best interest of people to do so)

it can prevent issues during profile setup, which is the first thing new aiida users do

the profile name is not exported, so this does not invalidate any published data

* Start validating * hostnames * emails * entry point names * InteractiveOption: validate empty reply The `InteractiveOption` of the CLI always allowed users to pass `!` in order to avoid setting a value, bypassing any further validation that may be provided by the `type`. Change this behavior such that entering `!` results in an empty string being passed further to the `type`. Only if conversion by the type passes as well should a `None` value be returned. * increase code reuse in options * move IdentifierParamType back to how it was before * add test for verdi code duplicate return empty string, if passing `!` into string-type parameters.

with outputting test coverage to the terminal on travis, the test output becomes increasingly unreadable and it becomes difficult to find the information on failing tests (which is what one is usually looking for). * remove coverage information from test output on travis * limit printing of test timings to 50 longest-running tests

sphuber

Thanks a lot @ltalirz

ltalirz force-pushed the issue_3301_input_validation branch from 2534ba7 to 4f7e473 Compare April 6, 2020 07:40

sphuber requested changes Apr 6, 2020

View reviewed changes

ltalirz force-pushed the issue_3301_input_validation branch 2 times, most recently from ecfd32e to f209092 Compare April 6, 2020 12:02

ltalirz mentioned this pull request Apr 6, 2020

seemingly random changes in pre-commit #3885

Closed

ltalirz force-pushed the issue_3301_input_validation branch 2 times, most recently from c59c32f to cca6522 Compare April 6, 2020 20:09

ltalirz mentioned this pull request Apr 8, 2020

"!" does not operate as described for verdi commands #3770

Closed

ltalirz force-pushed the issue_3301_input_validation branch 3 times, most recently from 09a2508 to a58739e Compare April 13, 2020 13:35

ltalirz requested a review from sphuber April 13, 2020 15:05

sphuber reviewed Apr 13, 2020

View reviewed changes

sphuber mentioned this pull request Apr 13, 2020

Unpin the click dependency #3921

Merged

ltalirz added 4 commits April 13, 2020 21:30

update verdi docs

317c337

update database setup docs

47fb71d

ltalirz force-pushed the issue_3301_input_validation branch from 860d3b0 to cb55197 Compare April 13, 2020 19:35

clean up leftover changes

f5b4aa7

ltalirz force-pushed the issue_3301_input_validation branch from cb55197 to f5b4aa7 Compare April 13, 2020 19:47

ltalirz requested a review from sphuber April 13, 2020 19:52

Merge branch 'develop' into issue_3301_input_validation

6d18745

sphuber approved these changes Apr 13, 2020

View reviewed changes

sphuber merged commit 6ea9ecb into aiidateam:develop Apr 13, 2020

sphuber deleted the issue_3301_input_validation branch April 13, 2020 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

additional validation for cli arguments #3894

additional validation for cli arguments #3894

ltalirz commented Apr 6, 2020 •

edited

Loading

sphuber left a comment

sphuber Apr 6, 2020

ltalirz Apr 6, 2020 •

edited

Loading

codecov bot commented Apr 13, 2020 •

edited

Loading

ltalirz commented Apr 13, 2020 •

edited

Loading

sphuber left a comment

sphuber Apr 13, 2020

ltalirz Apr 13, 2020

sphuber Apr 13, 2020

ltalirz Apr 13, 2020

sphuber left a comment



		class IdentifierParamType(click.ParamType, ABC):
		class IdentifierParamType(AlphanumericStringType, ABC):



		class ProfileParamType(click.ParamType):
		class ProfileParamType(LabelStringType):

additional validation for cli arguments #3894

additional validation for cli arguments #3894

Conversation

ltalirz commented Apr 6, 2020 • edited Loading

sphuber left a comment

Choose a reason for hiding this comment

sphuber Apr 6, 2020

Choose a reason for hiding this comment

ltalirz Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 13, 2020 • edited Loading

Codecov Report

ltalirz commented Apr 13, 2020 • edited Loading

sphuber left a comment

Choose a reason for hiding this comment

sphuber Apr 13, 2020

Choose a reason for hiding this comment

ltalirz Apr 13, 2020

Choose a reason for hiding this comment

sphuber Apr 13, 2020

Choose a reason for hiding this comment

ltalirz Apr 13, 2020

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

ltalirz commented Apr 6, 2020 •

edited

Loading

ltalirz Apr 6, 2020 •

edited

Loading

codecov bot commented Apr 13, 2020 •

edited

Loading

ltalirz commented Apr 13, 2020 •

edited

Loading