From a9dfd466235eacfa9646c63735f52128bef0b7c9 Mon Sep 17 00:00:00 2001 From: Ambrus Simon Date: Wed, 25 Oct 2017 14:30:32 -0500 Subject: [PATCH] Fix + validate custom rule configs --- api/jobs/gears.py | 4 +- api/jobs/handlers.py | 15 ++++-- api/jobs/rules.py | 3 ++ raml/schemas/definitions/rule.json | 6 +-- test/integration_tests/python/test_rules.py | 53 +++++++++++++-------- 5 files changed, 51 insertions(+), 30 deletions(-) diff --git a/api/jobs/gears.py b/api/jobs/gears.py index 5f4d6d635..59d64240d 100644 --- a/api/jobs/gears.py +++ b/api/jobs/gears.py @@ -105,8 +105,8 @@ 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) diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index bbdcb093a..9e0a99187 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -21,7 +21,7 @@ from ..auth.apikeys import JobApiKey -from .gears import validate_gear_config, get_gears, get_gear, get_invocation_schema, remove_gear, upsert_gear, suggest_container, get_gear_by_name, check_for_gear_insertion +from .gears import validate_gear_config, get_gears, get_gear, get_invocation_schema, remove_gear, upsert_gear, suggest_container, get_gear_by_name, check_for_gear_insertion, fill_gear_default_values from .jobs import Job, Logs from .batch import check_state, update from .queue import Queue @@ -182,7 +182,9 @@ def post(self, cid): self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(doc['alg'])) doc['project_id'] = cid - doc.setdefault('config', gear['gear']['config']) + + if 'config' in doc: + validate_gear_config(gear, fill_gear_default_values(gear, doc['config'])) result = config.db.project_rules.insert_one(doc) return { '_id': result.inserted_id } @@ -235,6 +237,11 @@ def put(self, cid, rid): except APINotFoundException: self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(updates['alg'])) + if 'alg' in updates or 'config' in updates: + gear = get_gear_by_name(updates.get('alg', doc['alg'])) + config_ = fill_gear_default_values(gear, updates.get('config', doc.get('config', {}))) + validate_gear_config(gear, config_) + doc.update(updates) config.db.project_rules.replace_one({'_id': bson.ObjectId(rid)}, doc) @@ -274,7 +281,7 @@ def add(self): if not self.superuser_request: uid = self.uid - job = Queue.enqueue_job(payload,self.origin, perm_check_uid=uid) + job = Queue.enqueue_job(payload, self.origin, perm_check_uid=uid) return { '_id': job.id_ } @@ -532,7 +539,7 @@ def post(self): gear = get_gear(gear_id) if gear.get('gear', {}).get('custom', {}).get('flywheel', {}).get('invalid', False): self.abort(400, 'Gear marked as invalid, will not run!') - validate_gear_config(gear, config_) + validate_gear_config(gear, fill_gear_default_values(gear, config_)) container_ids = [] container_type = None diff --git a/api/jobs/rules.py b/api/jobs/rules.py index 3e6e37585..8ba625745 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -199,6 +199,9 @@ def create_potential_jobs(db, container, container_type, file_): gear = gears.get_gear_by_name(alg_name) job = Job(str(gear['_id']), inputs, tags=['auto', alg_name]) + 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 c92774ae5..0b64b6d68 100644 --- a/raml/schemas/definitions/rule.json +++ b/raml/schemas/definitions/rule.json @@ -5,11 +5,7 @@ "project_id": { "type": "string" }, "alg": { "type": "string" }, "name": { "type": "string" }, - - "config": { - "type": "object", - "additionalProperties": { "$ref": "http://json-schema.org/draft-04/schema" } - }, + "config": { "type": "object" }, "rule-items": { "type": "array", diff --git a/test/integration_tests/python/test_rules.py b/test/integration_tests/python/test_rules.py index 75ebb79e8..cd72a2477 100644 --- a/test/integration_tests/python/test_rules.py +++ b/test/integration_tests/python/test_rules.py @@ -3,7 +3,7 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): gear = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.1'}) gear_2_name = randstr() - gear_2 = data_builder.create_gear(gear={'name': gear_2_name, 'version': '0.0.1'}) + gear_2 = data_builder.create_gear(gear={'name': gear_2_name, 'version': '0.0.2'}) rule = { 'alg': gear_name, @@ -211,8 +211,9 @@ def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with # create versioned gear to cover code selecting latest gear # add gear config to latest gear to check rules inheriting it gear_name = randstr() + gear_config = {'param': {'type': 'string', 'pattern': '^default|custom$', 'default': 'default'}} 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', 'config': {'param': {'type': 'string'}}}) + gear_2 = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.2', 'config': gear_config}) project = data_builder.create_project() bad_payload = {'test': 'rules'} @@ -275,32 +276,31 @@ def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with # set valid rule-item rule_json['all'] = [{'type': 'file.type', 'value': 'tabular data'}] - # try to add project rule w/ invalid gear config + # try to add project rule w/ non-existent gear # 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'] + assert "Cannot find gear" in r.json()['message'] - # try to add project rule w/ non-existent gear + # try to add project rule w/ invalid config # NOTE this is a legacy rule + rule_json['alg'] = gear_name + rule_json['config'] = {'param': 'invalid'} r = as_admin.post('/projects/' + project + '/rules', json=rule_json) - assert r.status_code == 400 - assert "Cannot find gear" in r.json()['message'] + assert r.status_code == 422 + assert r.json()['reason'] == 'config did not match manifest' + del rule_json['config'] # add project rule w/ proper gear alg # NOTE this is a legacy rule - rule_json['alg'] = gear_name r = as_admin.post('/projects/' + project + '/rules', json=rule_json) assert r.ok rule = r.json()['_id'] - # get project rules (verify rule was added and uses default gear config) + # get project rules 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') @@ -323,8 +323,9 @@ def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with 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 + 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' @@ -340,11 +341,25 @@ def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with 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_2})] 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_2})] + 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') @@ -385,7 +400,7 @@ def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with # test that job was not created via rule gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] - 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 = { @@ -413,9 +428,9 @@ def test_project_rules(randstr, data_builder, file_form, as_root, as_admin, with # test that only one job was created via rule gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] - 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)