Two Schemas for `workflows/config.yaml`, package validation, default package name #2364

fiskus · 2021-09-28T09:52:09Z

Base JSON Schema was updated
Added catalog specific JSON Schema
Added default package name via package_handle in catalog Schema
Added package name validation via handle_pattern in base Schema
Added entries validation via entries_schema in base Schema
Updated Ajv and dependent code

codecov · 2021-09-29T07:32:38Z

Codecov Report

Merging #2364 (cb212cf) into master (9b7f9c0) will increase coverage by 0.05%.
The diff coverage is 49.09%.

@@            Coverage Diff             @@
##           master    #2364      +/-   ##
==========================================
+ Coverage   42.96%   43.02%   +0.05%     
==========================================
  Files         496      496              
  Lines       23601    23725     +124     
  Branches     3081     3128      +47     
==========================================
+ Hits        10141    10208      +67     
- Misses      12645    12699      +54     
- Partials      815      818       +3

Flag	Coverage Δ
api-python	`89.82% <ø> (ø)`
catalog	`14.35% <49.09%> (+0.33%)`	⬆️
lambda	`93.87% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
catalog/app/containers/Bucket/PackageCopyDialog.js	`0.00% <0.00%> (ø)`
...alog/app/containers/Bucket/PackageCreateDialog.tsx	`0.00% <ø> (ø)`
...iners/Bucket/PackageDialog/MetaInputErrorHelper.js	`35.71% <0.00%> (ø)`
...iners/Bucket/PackageDialog/PackageCreationForm.tsx	`0.00% <0.00%> (ø)`
...g/app/containers/Bucket/PackageDirectoryDialog.tsx	`0.00% <0.00%> (ø)`
.../containers/Bucket/PackageDialog/PackageDialog.tsx	`24.33% <20.00%> (-0.67%)`	⬇️
catalog/app/containers/Bucket/requests/package.ts	`34.09% <25.00%> (-1.05%)`	⬇️
catalog/app/containers/Bucket/errors.tsx	`47.82% <66.66%> (+0.57%)`	⬆️
catalog/app/utils/workflows.ts	`89.61% <81.81%> (-10.39%)`	⬇️
catalog/app/utils/json-schema/json-schema.ts	`57.65% <90.90%> (+4.65%)`	⬆️
... and 2 more

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 9b7f9c0...cb212cf. Read the comment docs.

shared/schemas/workflows.yml.json

Co-authored-by: Sergey Fedoseev <[email protected]>

nl0

package entries validation still doesn't check all the entries, just the new ones;
there's a bug in the config validation/parsing related to presence/absence of the catalog version (more details inline);
probably it'd be cleaner to get the current schema versions from the actual schema objects instead of hardcoding them.

nl0 · 2021-10-11T09:03:38Z

catalog/app/utils/workflows.ts

-  if (!semver.satisfies(semver.coerce(baseVersion) || '0.0.0', baseSchemaVersion)) {
-    errors.push(new Error(`Catalog supports ${baseSchemaVersion} version of base Schema`))
+
+  const invalidBaseVersion = validateConfigVersion(baseVersion, '1.1.0')


probably this would've been a little cleaner if validateConfigVersion threw an error (instead of returning it) and this block was wrapped in a try/catch, e.g.

try { validateConfigVersion(baseVersion, '1.1.0') } catch (e) { errors.push(e) }

also, schema version should be taken directly from the schema object, not hardcoded

We hardcoded the version in the filename, so I don't see a problem to hardcode it here too. If incrementing versions become frequent operation, we could optimize it

Both validate functions return undefined or error, as validate functions for form. I think, consistency make more sense here

validateConfig throws error, so consistency doesn't matter here

We hardcoded the version in the filename, so I don't see a problem to hardcode it here too. If incrementing versions become frequent operation, we could optimize it

well, the problem is that you're introducing second source of truth (which should be manually synced with the first one), where it could be easily avoided, but i'm fine with this as long as you're conscious about it.

i'm not sure i fully get it about consistency (receiving mixed signals from two messages), but this call is yours too, i've just suggested how to make the code a little more concise

shared/schemas/workflows-config-1.1.0.json

catalog/app/containers/Bucket/PackageDialog/PackageCreationForm.tsx

catalog/app/containers/Bucket/PackageDirectoryDialog.tsx

nl0 · 2021-10-11T09:30:35Z

catalog/app/utils/workflows.ts

@@ -129,6 +216,7 @@ export function parse(workflowsYaml: string): WorkflowsConfig {
  const successors = data.successors || {}
  return {
    isWorkflowRequired: data.is_workflow_required !== false,
+    packageName: parsePackageNameTemplates(data.catalog?.package_handle),


a logic bug here: when there's no catalog version, config gets validated against the base schema (w/o catalog extension), but catalog-specific fields get parsed anyways, which can lead to accessing invalid data stored in catalog-specific fields (since they are always considered valid by the base schema). i see two ways to resolve this: either always validate against the catalog schema (assume it's of correct version by default and just check the data) or add some conditions to the config parsing logic that will prevent from reading catalog fields when there's no catalog version provided

I chose to remove catalog property if user didn't proved catalog version in config

tho probably it makes sense to tell the user about it, at least a warning in the console would help

shared/schemas/workflows-config-1.1.0.json

Co-authored-by: Alexei Mochalov <[email protected]>

nl0

Just two minor notes:
~~1. added files should take precedence over existing files when validating package contents;~~
2. some indication (e.g. console message) about dropping catalog-specific fields if catalog version is missing would be helpful.

catalog/app/containers/Bucket/PackageDialog/PackageCreationForm.tsx

nl0

Manual testing report

Catalog issues

[Minor bug, possible security issue] version is not validated before being used, validation happens after that,
bc a schema needs to be selected first based on that version.
This allows passing anything into version and it will be used as is --
this may be unsafe bc it's possible there are some vulnerabilites in semver
lib that can be exploited by passing it some specially constructed malicious
string, for example, so probably we should check version format first.
[Minor UX enhancement] When catalog version is missing, catalog-specific fields get removed from
the config, but it's not indicated to the user in any way -- this can be
very confusing. We should tell the user about that -- for example, by
printing a console message if their config has catalog-specific fields and
doesn't specify catalog version.
[Minor UX enhancement] Error message about incompatible config version doesn't specify which version
is incompatible -- base or catalog.
[Minor UX enhancement] Error message about invalid workflows config: consider separating errors
with newlines instead of commas for readability.
[Minor bug] When opening "package from folder" dialog while in the bucket root, "packages"
handle template is used when present (because directory is an empty string,
and the condition is mistakenly triggered -- see inline notes).
[Minor UX enhancement] username should be passed to the "files" handle template as well -- this
can make templates a little more flexible and useful.
[Bug] package_handle overrides existing package name in "revise" and "promote"
dialogs -- I believe it shouldn't, because we're working with the existing
package and should keep the name by default.
[Bug] Package name validation against handle_pattern isn't triggered when switching
workflows -- it's only triggered when editing the name afterwards. This allows
to bypass name validation.
[Minor UX enhancement] When entries_schema contains invalid id (not pointing to any known schema),
and also when schema contains invalid JSON (parsing error), validation is
skipped entirely without a notice -- we should show some error message instead.
[Bug] When schema url points to a non-existent file, and also when a schema file
contains invalid schema object, UI gets permanently locked because of an unhandled
exception after validation attempt.
[Minor UX enhancement] Validation messages shown below the files input could use some margin between
them or a common container.
[Minor UX enhancement] Backend validation messages should be displayed the same way as the frontend
messages -- below the files input, preferably with the same visual treatment.
Also, the messages received from the backend are very uninformative, but that
probably should be addressed in the backend PR.
[Minor UX enhancement] Backend validation errors contain leading quote -- not sure if it's a backend
or a catalog.

Backend issues (posting here just FYI)

Package contents aren't validated properly: after validation passed in the
catalog and files were uploaded, backend call returns an error:
Package entries failed validation: [] is too short. This prevents from
creating a package even tho the entries are valid.
Validation is skipped when schema is invalid (not valid JSON schema),
unrecognized fields (possible typos) are ignored -- probably should be more strict
and throw errors to be consistent with the catalog.
Very unclear message when one of allOf schemas fail, e.g.:
Package entries failed validation: None of [{'logical_key': '1.md', 'size': 2}, {'logical_key': '2.md', 'size': 2}, {'logical_key': 'README.md', 'size': 14}] are valid under the given schema.,
when in reality all of the items are valid, but missing one item required via contains.
Backend validation errors are very uninformative, they should at least specify
paths were errors were encountered (try validating min/max size, for example).
Missing $ref support -- I realize it's a known limitation and lifting it is
out of scope of the current effort, but it doesn't seem too hard to at least
support local refs (for example, ajv supports them out of the box AFAIK).

catalog/app/containers/Bucket/PackageDialog/PackageDialog.tsx

fiskus · 2021-10-14T07:13:36Z

[Bug] Package name validation against handle_pattern isn't triggered when switching
workflows -- it's only triggered when editing the name afterwards. This allows
to bypass name validation.

Validation is triggered, but we show error only when:

submit failed
value was modified

validation.mp4

nl0 · 2021-10-14T08:25:59Z

[Bug] Package name validation against handle_pattern isn't triggered when switching
workflows -- it's only triggered when editing the name afterwards. This allows
to bypass name validation.

Validation is triggered, but we show error only when:
* submit failed

* value was modified

it's not

Screen.Recording.2021-10-14.at.13.21.15.mov

fiskus · 2021-10-14T08:56:45Z

Reproduced with your example, thanks. Technically, validation is triggered but with the previous workflow, didn't figure out why yet

catalog/app/containers/Bucket/PackageDialog/PackageDialog.tsx

fiskus added 9 commits September 27, 2021 16:13

update ajv

25adc4d

update workflows JSON Schema

6b89d60

fix workflows Schema and catalog specific Schema

0e14ee9

apply new Schema validation for catalog workflows

773c5ef

fix Schema format

e032908

this feature is not ready yet

83cf642

fix tests according to ajv update

49b4a4b

rewrite ajv options

7254f02

fix ajv errors format

914ba38

fiskus added 15 commits September 29, 2021 15:01

fix schema

b778edb

add package_handle support

43d94bd

add hook for getting package name template

e5e5b24

minor changes

0d9d5f3

use new approach for changing name on workflow change

10017fe

add tests

60d3f80

add return type

1545cc7

add object_schema property

907bf40

handle versioning too

7a83ab9

use handle_pattern template

3406757

show pattern error

191f1f2

add entries validator

7c76725

adjust UI

956ad11

show files error

728f98c

Merge branch 'master' of github.com:quiltdata/quilt into two-schemas

3fdb01b

sir-sigurd reviewed Oct 6, 2021

View reviewed changes

shared/schemas/workflows.yml.json Outdated Show resolved Hide resolved

fiskus and others added 4 commits October 7, 2021 08:44

clear error message on files add

919b335

Update shared/schemas/workflows.yml.json

1d9372f

Co-authored-by: Sergey Fedoseev <[email protected]>

use object version

047393e

use regex format too

5607921

fiskus added 2 commits October 11, 2021 08:14

update version Schema

dabd012

use workflow value intsead of workflow getter

660dea7

fiskus requested a review from nl0 October 11, 2021 05:22

nl0 requested changes Oct 11, 2021

View reviewed changes

fiskus and others added 5 commits October 11, 2021 13:21

Apply suggestions from code review

27955f1

Co-authored-by: Alexei Mochalov <[email protected]>

add workflow calculation inline

94f1268

remove catalog if there is no catalog version in workflows/config.yaml

0acd939

use all files state to calc actual files

ffae8c5

fix schema

6087f56

fiskus requested a review from nl0 October 11, 2021 14:16

nl0 approved these changes Oct 11, 2021

View reviewed changes

catalog/app/containers/Bucket/PackageDialog/PackageCreationForm.tsx Show resolved Hide resolved

Merge branch 'master' of github.com:quiltdata/quilt into two-schemas

bc6f703

nl0 reviewed Oct 13, 2021

View reviewed changes

catalog/app/containers/Bucket/PackageDialog/PackageDialog.tsx Outdated Show resolved Hide resolved

catalog/app/containers/Bucket/PackageDialog/PackageDialog.tsx Outdated Show resolved Hide resolved

fiskus added 2 commits October 14, 2021 09:28

Merge branch 'master' of github.com:quiltdata/quilt into two-schemas

c6b1fee

fix mistypings

607df74

catch Ajv errors

89fae6e

dont rewrite existing name

18b61eb

nl0 reviewed Oct 14, 2021

View reviewed changes

catalog/app/containers/Bucket/PackageDialog/PackageDialog.tsx Outdated Show resolved Hide resolved

fiskus added 5 commits October 14, 2021 12:14

adjust unit-test

1e64883

divide empty string made on purpose and lack of value

a5b71c7

fix unit test

fb51a25

exporting and dont use exported is confusing

1ea1d7f

Merge branch 'master' of github.com:quiltdata/quilt into two-schemas

cb212cf

fiskus merged commit b4a1dfb into master Oct 14, 2021

fiskus deleted the two-schemas branch October 14, 2021 17:49

This was referenced Oct 22, 2021

Package handle template #2137

Closed

Validate package contents #2313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two Schemas for `workflows/config.yaml`, package validation, default package name #2364

Two Schemas for `workflows/config.yaml`, package validation, default package name #2364

fiskus commented Sep 28, 2021 •

edited

Loading

codecov bot commented Sep 29, 2021 •

edited

Loading

nl0 left a comment

nl0 Oct 11, 2021

nl0 Oct 11, 2021

fiskus Oct 11, 2021

fiskus Oct 11, 2021

fiskus Oct 11, 2021

nl0 Oct 11, 2021

nl0 Oct 11, 2021

nl0 Oct 11, 2021

fiskus Oct 11, 2021

nl0 Oct 11, 2021

nl0 left a comment •

edited

Loading

nl0 left a comment •

edited

Loading

fiskus commented Oct 14, 2021

nl0 commented Oct 14, 2021

fiskus commented Oct 14, 2021

Two Schemas for workflows/config.yaml, package validation, default package name #2364

Two Schemas for workflows/config.yaml, package validation, default package name #2364

Conversation

fiskus commented Sep 28, 2021 • edited Loading

codecov bot commented Sep 29, 2021 • edited Loading

Codecov Report

nl0 left a 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

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

nl0 left a comment • edited Loading

Choose a reason for hiding this comment

nl0 left a comment • edited Loading

Choose a reason for hiding this comment

Manual testing report

Catalog issues

Backend issues (posting here just FYI)

fiskus commented Oct 14, 2021

nl0 commented Oct 14, 2021

fiskus commented Oct 14, 2021

Two Schemas for `workflows/config.yaml`, package validation, default package name #2364

Two Schemas for `workflows/config.yaml`, package validation, default package name #2364

fiskus commented Sep 28, 2021 •

edited

Loading

codecov bot commented Sep 29, 2021 •

edited

Loading

nl0 left a comment •

edited

Loading

nl0 left a comment •

edited

Loading