From 71a813835e0c1c880bf017f5dd46fc519b43cc8c Mon Sep 17 00:00:00 2001 From: Ambrus Simon Date: Fri, 26 Jan 2018 17:39:28 +0100 Subject: [PATCH] Add config to rules --- api/jobs/gears.py | 10 +-- api/jobs/handlers.py | 7 +- api/jobs/queue.py | 4 +- api/jobs/rules.py | 3 + raml/schemas/definitions/rule.json | 2 + tests/integration_tests/python/test_rules.py | 69 ++++++++++++++++---- 6 files changed, 72 insertions(+), 23 deletions(-) diff --git a/api/jobs/gears.py b/api/jobs/gears.py index 123bb5e87..3c8f09358 100644 --- a/api/jobs/gears.py +++ b/api/jobs/gears.py @@ -5,6 +5,7 @@ from __future__ import absolute_import import bson.objectid +import copy import datetime from jsonschema import Draft4Validator, ValidationError import gears as gear_tools @@ -97,13 +98,13 @@ def suggest_for_files(gear, files): return suggested_files def validate_gear_config(gear, config_): - if len(gear.get('manifest', {}).get('config', {})) > 0: - invocation = gear_tools.derive_invocation_schema(gear['manifest']) + if len(gear.get('gear', {}).get('config', {})) > 0: + invocation = gear_tools.derive_invocation_schema(gear['gear']) ci = gear_tools.isolate_config_invocation(invocation) validator = Draft4Validator(ci) try: - validator.validate(config_) + validator.validate(fill_gear_default_values(gear, config_)) except ValidationError as err: key = None if len(err.relative_path) > 0: @@ -121,8 +122,7 @@ def fill_gear_default_values(gear, config_): Given a gear and a config map, fill any missing keys using defaults from the gear's config """ - if config_ is None: - config_ = {} + config_ = copy.deepcopy(config_) or {} for k,v in gear['gear'].get('config', {}).iteritems(): if 'default' in v: diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index e64061a32..ce6854943 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -181,7 +181,7 @@ def post(self, cid): validate_data(doc, 'rule-new.json', 'input', 'POST', optional=True) validate_regexes(doc) - get_gear(doc['gear_id']) + validate_gear_config(get_gear(doc['gear_id']), doc.get('config')) doc['project_id'] = cid @@ -231,8 +231,9 @@ def put(self, cid, rid): updates = self.request.json validate_data(updates, 'rule-update.json', 'input', 'POST', optional=True) validate_regexes(updates) - if updates.get('gear_id'): - get_gear(updates['gear_id']) + gear_id = updates.get('gear_id', doc['gear_id']) + config_ = updates.get('config', doc.get('config')) + validate_gear_config(get_gear(gear_id), config_) doc.update(updates) config.db.project_rules.replace_one({'_id': bson.ObjectId(rid)}, doc) diff --git a/api/jobs/queue.py b/api/jobs/queue.py index 4d2ab8ce9..8223b4f26 100644 --- a/api/jobs/queue.py +++ b/api/jobs/queue.py @@ -158,7 +158,7 @@ def enqueue_job(job_map, origin, perm_check_uid=None): if gear.get('gear', {}).get('custom', {}).get('flywheel', {}).get('invalid', False): raise InputValidationException('Gear marked as invalid, will not run!') - config_ = fill_gear_default_values(gear, job_map.get('config', {})) + config_ = job_map.get('config', {}) validate_gear_config(gear, config_) # Translate maps to FileReferences @@ -197,7 +197,7 @@ def enqueue_job(job_map, origin, perm_check_uid=None): # Config options are stored on the job object under the "config" key config_ = { - 'config': config_, + 'config': fill_gear_default_values(gear, config_), 'inputs': { }, 'destination': { 'type': destination.type, diff --git a/api/jobs/rules.py b/api/jobs/rules.py index bb9c97b73..12c9dc7f8 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -203,6 +203,9 @@ def create_potential_jobs(db, container, container_type, file_): job = Job(str(gear['_id']), inputs, tags=['auto', gear_tag]) + if 'config' in rule: + job.config = rule['config'] + potential_jobs.append({ 'job': job, 'rule': rule diff --git a/raml/schemas/definitions/rule.json b/raml/schemas/definitions/rule.json index 69d13a674..94caecec0 100644 --- a/raml/schemas/definitions/rule.json +++ b/raml/schemas/definitions/rule.json @@ -31,6 +31,7 @@ "project_id": { "type": "string" }, "gear_id": { "type": "string" }, "name": { "type": "string" }, + "config": { "type": "object" }, "any": { "$ref": "#/definitions/rule-items" }, "all": { "$ref": "#/definitions/rule-items" }, "disabled": { "type": "boolean" } @@ -44,6 +45,7 @@ "_id": { "type": "string" }, "gear_id": { "type": "string" }, "name": { "type": "string" }, + "config": { "type": "object" }, "any": { "$ref": "#/definitions/rule-items" }, "all": { "$ref": "#/definitions/rule-items" }, "disabled": { "type": "boolean" } diff --git a/tests/integration_tests/python/test_rules.py b/tests/integration_tests/python/test_rules.py index 1981c539f..82e136c0c 100644 --- a/tests/integration_tests/python/test_rules.py +++ b/tests/integration_tests/python/test_rules.py @@ -222,9 +222,10 @@ 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 - gear = data_builder.create_gear(gear={'version': '0.0.1'}) + gear_config = {'param': {'type': 'string', 'pattern': '^default|custom$', 'default': 'default'}} + gear = data_builder.create_gear(gear={'version': '0.0.1', 'config': gear_config}) project = data_builder.create_project() bad_payload = {'test': 'rules'} @@ -267,19 +268,42 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a 'gear_id': '000000000000000000000000', '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/ non-existent gear # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json=rule_json) assert r.status_code == 404 - # add project rule w/ proper gear id + # try to add project rule w/ invalid config # NOTE this is a legacy rule rule_json['gear_id'] = gear + rule_json['config'] = {'param': 'invalid'} + r = as_admin.post('/projects/' + project + '/rules', json=rule_json) + assert r.status_code == 422 + assert r.json()['reason'] == 'config did not match manifest' + del rule_json['config'] + + # add project rule w/ proper gear id + # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json=rule_json) assert r.ok rule = r.json()['_id'] @@ -305,10 +329,15 @@ 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={'gear_id': gear}) assert r.status_code == 403 - # try to update rule to with invalid gear id + # try to update rule with invalid gear id r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'gear_id': '000000000000000000000000'}) assert r.status_code == 404 + # try to update rule with invalid gear config + r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'config': {'param': 'invalid'}}) + assert r.status_code == 422 + assert r.json()['reason'] == 'config did not match manifest' + # update name of rule rule_name = 'improved-csv-trigger-rule' r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'name': rule_name}) @@ -323,11 +352,25 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a r = as_admin.post('/projects/' + project + '/files', files=file_form('test2.csv')) assert r.ok - # test that job was created via rule + # test that job was created via rule and uses gear default config gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] assert len(gear_jobs) == 1 assert len(gear_jobs[0]['inputs']) == 1 assert gear_jobs[0]['inputs'][0]['name'] == 'test2.csv' + assert gear_jobs[0]['config']['config'] == {'param': 'default'} + + # update rule to have a custom config + r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'config': {'param': 'custom'}}) + assert r.ok + + # upload another file that matches rule + r = as_admin.post('/projects/' + project + '/files', files=file_form('test3.csv')) + assert r.ok + + # test that job was created via rule and custom config + gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] + assert len(gear_jobs) == 2 + assert gear_jobs[1]['config']['config'] == {'param': 'custom'} # try to delete rule of non-existent project r = as_admin.delete('/projects/000000000000000000000000/rules/000000000000000000000000') @@ -368,7 +411,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # test that job was not created via rule gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] - assert len(gear_jobs) == 1 # still 1 from before + assert len(gear_jobs) == 2 # still 2 from before # update test2.csv's metadata to include a valid measurement to spawn job metadata = { @@ -396,9 +439,9 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # test that only one job was created via rule gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] - assert len(gear_jobs) == 2 - assert len(gear_jobs[1]['inputs']) == 1 - assert gear_jobs[1]['inputs'][0]['name'] == 'test3.txt' + assert len(gear_jobs) == 3 + assert len(gear_jobs[2]['inputs']) == 1 + assert gear_jobs[2]['inputs'][0]['name'] == 'test3.txt' # delete rule r = as_admin.delete('/projects/' + project + '/rules/' + rule2) @@ -423,7 +466,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # test that job was created via regex rule gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] - assert len(gear_jobs) == 3 + assert len(gear_jobs) == 4 # delete rule r = as_admin.delete('/projects/' + project + '/rules/' + rule3)