-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(jobs): ValidateAction #1421
Conversation
This partially reverts commit b218952. - Tightened bounds on DtoType for JobAction - Validate DTOs
DTO validation is in the controller, so the interceptor isn't needed.
Also tests and example
What does everyone think of the use of JSONPath? Pro: Simple syntax for simple reference, while keeping enough power for more complex structures like arrays A more powerful option would be to validate against a JSON Schema. This would allow type checking. However I think it would be much more verbose. |
I think the JSONpath solution is appropriate for now. |
Good catch, @sofyalaski. Why does this even compile? |
Ah, it compiles because JobClass is structurally compatible with CreateJobDto. Working on a fix now. |
- Revert the previous commit, which was incorrect and incomplete - Make valiateDTO and performActions generic so they can be shared between endpoints - Separate out checkConfigVersion, which is specific to statusUpdate - Move `getJobTypeConfiguration` calls up so this is only called once
This implementation has a couple problems. First, the JSONPath solution doesn't perform any type checking. It would be nice to do something like
This looks fine for simple types (string, number, list of literal values) but could quickly grow in complexity until it reaches a full type system. One solution would be to supply a JSON Schema to validate the DTO (which provides full type support). However including JSON schemas in the config file seems overly complex for operators. Is there a happy medium? Second, the validate method currently acts against the DTO body. Half of the checks in v3 are actually against database entities referenced in the DTO or Job itself. For instance, "archive" jobs should check that all datasets associated with the job have "datasetLifecycle.archivable". Or "publish" jobs with file listings should check that all files exist, requiring checking associated DataBlocks from the dataset. Should these be added to a generic ValidateAction or be a more specific check? We are leaning towards handling those as special cases in the jobs controller. |
- Add new test for validate jobs to check for required parameters in both POST and PATCH operations - Remove some unused TestData which didn't match the current Job DTOs - Add 'validate' job to the test jobconfig.json
- rename top-level array to 'request'. This leaves open the possibility to validate other things beside the DTO - now configuration takes the format of `"JSONPath": JSON-Schema`. The schema part is validated with Avj. Note that JSON Schemas can be very concise for simple datatypes, so this doesn't require deep knowledge of how to write JSONSchema. - Update unit & mocha tests
- remove 'any' via typeguard - `npm run lint:fix` - remove duplicate (and outdated) docstring
(previous merge commit lost a parent)
I changed how this works somewhat. Now the ValidateAction config looks like:
The schema gets validated by avj, so you theoretically could use the full power of a schema. However combining it with the path allows most use cases to be covered by short, easily readable one-liners. Examples:
I spelled the top-level |
@@ -694,62 +694,6 @@ const TestData = { | |||
], | |||
}, | |||
|
|||
ArchiveJob: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These weren't used by any tests. OK to delete them?
"jobParams.datasetIds[*]": { | ||
"type": "object", | ||
"required": ["pid","files"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the best example, as it's a bit complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to try and also cover #1120 in this PR?
Agreed to merge #1440, then remove |
bc8755b
to
761bac5
Compare
Description
Add ValidateAction to check Job DTO formatting
Motivation
When creating a job clients can provide an untyped
jobParams
object in the DTO, or ajobResultObject
when updating. These were previously not validated.Fixes:
This PR adds a new
validate
job action which can test for the presence of particular attributes in the DTO. Currently the attributes are specified using a JSONPath and are only verified to be non-null. If needed, future implementations could add JSONSchema support to do deeper validation.For example, the following
jobConfig.json
section would configure thepublic
job to require at least one datasetId:Changes:
validate
actionTests included
Documentation