From f0e95ce5b267ce91f98d9c8d35d9b4c07b9bea17 Mon Sep 17 00:00:00 2001 From: Ambrus Simon Date: Fri, 20 Oct 2017 14:58:47 +0200 Subject: [PATCH] Add gear config to rules and improve rule input validation --- api/config.py | 2 +- api/jobs/handlers.py | 5 +- raml/resources/projects.raml | 3 + raml/schemas/definitions/rule.json | 77 ++++++++++++++++----- raml/schemas/input/rule-add.json | 62 ----------------- raml/schemas/input/rule-new.json | 7 ++ raml/schemas/input/rule-update.json | 60 +--------------- raml/schemas/output/rule-list.json | 5 ++ raml/schemas/output/rules-list.json | 7 -- test/integration_tests/python/test_rules.py | 44 ++++++++++-- 10 files changed, 117 insertions(+), 155 deletions(-) delete mode 100644 raml/schemas/input/rule-add.json create mode 100644 raml/schemas/input/rule-new.json create mode 100644 raml/schemas/output/rule-list.json delete mode 100644 raml/schemas/output/rules-list.json diff --git a/api/config.py b/api/config.py index 7bcfc5b6b..87946b31b 100644 --- a/api/config.py +++ b/api/config.py @@ -170,7 +170,7 @@ def apply_env_variables(config): 'project.json', 'project-template.json', 'project-update.json', - 'rule-add.json', + 'rule-new.json', 'rule-update.json', 'session.json', 'session-update.json', diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index 1f324b4bf..4fa373a88 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -164,13 +164,14 @@ def post(self, cid): doc = self.request.json - validate_data(doc, 'rule-add.json', 'input', 'POST', optional=True) + validate_data(doc, 'rule-new.json', 'input', 'POST', optional=True) try: - get_gear_by_name(doc['alg']) + gear = get_gear_by_name(doc['alg']) except APINotFoundException: self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(doc['alg'])) doc['project_id'] = cid + doc.setdefault('config', gear['gear']['config']) result = config.db.project_rules.insert_one(doc) return { '_id': result.inserted_id } diff --git a/raml/resources/projects.raml b/raml/resources/projects.raml index af9aafcc6..054a92547 100644 --- a/raml/resources/projects.raml +++ b/raml/resources/projects.raml @@ -81,6 +81,9 @@ post: get: responses: 200: + body: + application/json: + schema: !include ../schemas/output/rule-list.json /analyses: type: analyses-list /{AnalysisId}: diff --git a/raml/schemas/definitions/rule.json b/raml/schemas/definitions/rule.json index 64f8553f1..c92774ae5 100644 --- a/raml/schemas/definitions/rule.json +++ b/raml/schemas/definitions/rule.json @@ -1,22 +1,63 @@ { "$schema": "http://json-schema.org/draft-04/schema#", - "type": "object", - "properties": { - "alg": { - "type": "string" - }, - "all":{ - "type":"array", - "items":{ - "type":"array", - "items":{ - "type":"string" - } + "definitions": { + "_id": { "type": "string" }, + "project_id": { "type": "string" }, + "alg": { "type": "string" }, + "name": { "type": "string" }, + + "config": { + "type": "object", + "additionalProperties": { "$ref": "http://json-schema.org/draft-04/schema" } + }, + + "rule-items": { + "type": "array", + "items": { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "file.type", + "file.name", + "file.measurements", + "container.has-type", + "container.has-measurement" + ] + }, + "value": { "type": "string" } + }, + "required": [ "type", "value" ], + "additionalProperties": false + } + }, + + + "rule-input": { + "type": "object", + "properties": { + "_id": { "$ref": "#/definitions/_id" }, + "project_id": { "$ref": "#/definitions/project_id" }, + "alg": { "$ref": "#/definitions/alg" }, + "name": { "$ref": "#/definitions/name" }, + "config": { "$ref": "#/definitions/config" }, + "any": { "$ref": "#/definitions/rule-items" }, + "all": { "$ref": "#/definitions/rule-items" } + }, + "additionalProperties": false + }, + + "rule-output": { + "type": "object", + "properties": { + "_id": { "$ref": "#/definitions/_id" }, + "alg": { "$ref": "#/definitions/alg" }, + "name": { "$ref": "#/definitions/name" }, + "config": { "$ref": "#/definitions/config" }, + "any": { "$ref": "#/definitions/rule-items" }, + "all": { "$ref": "#/definitions/rule-items" } + } } - } - }, - "required": [ - "alg", - "all" - ] + } } diff --git a/raml/schemas/input/rule-add.json b/raml/schemas/input/rule-add.json deleted file mode 100644 index bf41dde0b..000000000 --- a/raml/schemas/input/rule-add.json +++ /dev/null @@ -1,62 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-04/schema#", - "title": "rule", - "type": "object", - "properties": { - "alg": { - "type": "string" - }, - "name": { - "type": "string" - }, - "any": { - "type": "array", - "items": { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "file.type", - "file.name", - "file.measurements", - "container.has-type", - "container.has-measurement" - ] - }, - "value": { - "type": "string" - } - } - } - }, - "all": { - "type": "array", - "items": { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "file.type", - "file.name", - "file.measurements", - "container.has-type", - "container.has-measurement" - ] - }, - "value": { - "type": "string" - } - } - } - } - }, - "required": [ - "alg", - "all", - "any", - "name" - ], - "additionalProperties": false -} diff --git a/raml/schemas/input/rule-new.json b/raml/schemas/input/rule-new.json new file mode 100644 index 000000000..c456eaec4 --- /dev/null +++ b/raml/schemas/input/rule-new.json @@ -0,0 +1,7 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "title": "Rule", + "type": "object", + "allOf": [{"$ref": "../definitions/rule.json#/definitions/rule-input"}], + "required": ["alg", "name", "any", "all"] +} diff --git a/raml/schemas/input/rule-update.json b/raml/schemas/input/rule-update.json index 2db5a2267..31ffc6d60 100644 --- a/raml/schemas/input/rule-update.json +++ b/raml/schemas/input/rule-update.json @@ -1,62 +1,6 @@ { "$schema": "http://json-schema.org/draft-04/schema#", - "title": "rule", + "title": "Rule", "type": "object", - "properties": { - "_id": { - "type": "string" - }, - "project_id": { - "type": "string" - }, - "alg": { - "type": "string" - }, - "name": { - "type": "string" - }, - "any": { - "type": "array", - "items": { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "file.type", - "file.name", - "file.measurements", - "container.has-type", - "container.has-measurement" - ] - }, - "value": { - "type": "string" - } - } - } - }, - "all": { - "type": "array", - "items": { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "file.type", - "file.name", - "file.measurements", - "container.has-type", - "container.has-measurement" - ] - }, - "value": { - "type": "string" - } - } - } - } - }, - "additionalProperties": false + "allOf": [{"$ref": "../definitions/rule.json#/definitions/rule-input"}] } diff --git a/raml/schemas/output/rule-list.json b/raml/schemas/output/rule-list.json new file mode 100644 index 000000000..c97e91234 --- /dev/null +++ b/raml/schemas/output/rule-list.json @@ -0,0 +1,5 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "type": "array", + "items": { "$ref":"../definitions/rule.json#/definitions/rule-output" } +} diff --git a/raml/schemas/output/rules-list.json b/raml/schemas/output/rules-list.json deleted file mode 100644 index 22c31bcbf..000000000 --- a/raml/schemas/output/rules-list.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-04/schema#", - "type": "array", - "items": { - "$ref":"../definitions/rule.json#" - } -} diff --git a/test/integration_tests/python/test_rules.py b/test/integration_tests/python/test_rules.py index cecbb0ec5..75ebb79e8 100644 --- a/test/integration_tests/python/test_rules.py +++ b/test/integration_tests/python/test_rules.py @@ -207,11 +207,12 @@ def test_site_rules_copied_to_new_projects(randstr, data_builder, file_form, as_ data_builder.delete_group(group, recursive=True) -def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, api_db): +def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, api_db): # create versioned gear to cover code selecting latest gear + # add gear config to latest gear to check rules inheriting it gear_name = randstr() gear_1 = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.1'}) - gear_2 = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.2'}) + gear_2 = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.2', 'config': {'param': {'type': 'string'}}}) project = data_builder.create_project() bad_payload = {'test': 'rules'} @@ -254,15 +255,39 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a 'alg': 'non-existent-gear-name', 'name': 'csv-job-trigger-rule', 'any': [], - 'all': [ - {'type': 'file.type', 'value': 'tabular data'}, - ] + 'all': [], } + # try to add project rule w/ invalid rule-item (invalid type) + # NOTE this is a legacy rule + rule_json['all'] = [{'type': 'invalid', 'value': 'test'}] + r = as_admin.post('/projects/' + project + '/rules', json=rule_json) + assert r.status_code == 400 + assert "'invalid' is not one of" in r.json()['message'] + + # try to add project rule w/ invalid rule-item (missing value) + # NOTE this is a legacy rule + rule_json['all'] = [{'type': 'file.name'}] + r = as_admin.post('/projects/' + project + '/rules', json=rule_json) + assert r.status_code == 400 + assert "'value' is a required property" in r.json()['message'] + + # set valid rule-item + rule_json['all'] = [{'type': 'file.type', 'value': 'tabular data'}] + + # try to add project rule w/ invalid gear config + # NOTE this is a legacy rule + rule_json['config'] = {'foo': 'bar'} + r = as_admin.post('/projects/' + project + '/rules', json=rule_json) + assert r.status_code == 400 + assert "'bar' is not of type u'object'" in r.json()['message'] + del rule_json['config'] + # try to add project rule w/ non-existent gear # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json=rule_json) assert r.status_code == 400 + assert "Cannot find gear" in r.json()['message'] # add project rule w/ proper gear alg # NOTE this is a legacy rule @@ -271,10 +296,11 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok rule = r.json()['_id'] - # get project rules (verify rule was added) + # get project rules (verify rule was added and uses default gear config) r = as_admin.get('/projects/' + project + '/rules') assert r.ok assert r.json()[0]['alg'] == gear_name + assert r.json()[0]['config'] == {'param': {'type': 'string'}} # try to get single project rule using non-existent rule id r = as_admin.get('/projects/' + project + '/rules/000000000000000000000000') @@ -292,10 +318,14 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a r = with_user.session.put('/projects/' + project + '/rules/' + rule, json={'alg': gear_name}) assert r.status_code == 403 - # try to update rule to with invalid gear alg + # try to update rule with invalid gear alg r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'alg': 'not-a-real-gear'}) assert r.status_code == 400 + # try to update rule with invalid gear config + r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'config': {'foo': 'bar'}}) + assert r.status_code == 400 + # update name of rule rule_name = 'improved-csv-trigger-rule' r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'name': rule_name})