From 508e626b494105d01184ff6e704cda0ad6e9400c Mon Sep 17 00:00:00 2001 From: Ambrus Simon Date: Fri, 26 Jan 2018 17:38:32 +0100 Subject: [PATCH] Use gear id instead of name (alg) in rules --- api/jobs/gears.py | 16 ++--- api/jobs/handlers.py | 14 ++-- api/jobs/rules.py | 22 ++++--- swagger/examples/input/rule-new.json | 2 +- swagger/examples/output/rule-list.json | 6 +- swagger/examples/rules_list.json | 2 +- swagger/schemas/definitions/rule.json | 4 +- swagger/schemas/input/rule-new.json | 2 +- tests/integration_tests/python/test_jobs.py | 5 +- tests/integration_tests/python/test_rules.py | 68 +++++++++----------- tests/unit_tests/python/test_rules.py | 6 +- 11 files changed, 62 insertions(+), 85 deletions(-) diff --git a/api/jobs/gears.py b/api/jobs/gears.py index d1f37c663..ea564d074 100644 --- a/api/jobs/gears.py +++ b/api/jobs/gears.py @@ -8,7 +8,6 @@ import datetime from jsonschema import Draft4Validator, ValidationError import gears as gear_tools -import pymongo from .. import config from .jobs import Job @@ -39,17 +38,10 @@ def get_gears(): return map(lambda x: x['original'], cursor) def get_gear(_id): - return config.db.gears.find_one({'_id': bson.ObjectId(_id)}) - -def get_gear_by_name(name): - - # Find a gear from the list by name - gear_doc = list(config.db.gears.find({'gear.name': name}).sort('created', pymongo.DESCENDING)) - - if len(gear_doc) == 0 : - raise APINotFoundException('Unknown gear ' + name) - - return gear_doc[0] + gear = config.db.gears.find_one({'_id': bson.ObjectId(_id)}) + if gear is None: + raise APINotFoundException('Cannot find gear {}'.format(_id)) + return gear def get_invocation_schema(gear): return gear_tools.derive_invocation_schema(gear['gear']) diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index c45883d8d..0f7c0d7a3 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -24,7 +24,7 @@ from ..web.errors import APIPermissionException, APINotFoundException, InputValidationException from ..web.request import AccessType -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, check_for_gear_insertion from .jobs import Job, JobTicket, Logs from .batch import check_state, update from .queue import Queue @@ -147,10 +147,7 @@ def post(self, cid): validate_data(payload, 'rule-new.json', 'input', 'POST', optional=True) validate_regexes(payload) - try: - get_gear_by_name(payload['alg']) - except APINotFoundException: - self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(payload['alg'])) + get_gear(payload['gear_id']) payload['project_id'] = cid @@ -200,11 +197,8 @@ 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('alg'): - try: - get_gear_by_name(updates['alg']) - except APINotFoundException: - self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(updates['alg'])) + if updates.get('gear_id'): + get_gear(updates['gear_id']) doc.update(updates) config.db.project_rules.replace_one({'_id': bson.ObjectId(rid)}, doc) diff --git a/api/jobs/rules.py b/api/jobs/rules.py index b111baf59..bb9c97b73 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -138,16 +138,16 @@ def eval_rule(rule, file_, container): return True -def queue_job_legacy(algorithm_id, input_): +def queue_job_legacy(gear_id, input_): """ Tie together logic used from the no-manifest, single-file era. Takes a single FileReference instead of a map. """ - gear = gears.get_gear_by_name(algorithm_id) + gear = gears.get_gear(gear_id) if len(gear['gear']['inputs']) != 1: - raise Exception("Legacy gear enqueue attempt of " + algorithm_id + " failed: must have exactly 1 input in manifest") + raise Exception("Legacy gear enqueue attempt of " + gear_id + " failed: must have exactly 1 input in manifest") input_name = gear['gear']['inputs'].keys()[0] @@ -155,7 +155,8 @@ def queue_job_legacy(algorithm_id, input_): input_name: input_ } - job = Job(str(gear['_id']), inputs, tags=['auto', algorithm_id]) + gear_tag = gear['gear']['name'] + '-' + gear_id + job = Job(str(gear['_id']), inputs, tags=['auto', gear_tag]) return job def find_type_in_container(container, type_): @@ -184,22 +185,23 @@ def create_potential_jobs(db, container, container_type, file_): if 'from_failed_job' not in file_ and eval_rule(rule, file_, container): - alg_name = rule['alg'] + gear_id = rule['gear_id'] + gear = gears.get_gear(gear_id) + gear_tag = gear['gear']['name'] + '-' + gear_id if rule.get('match') is None: input_ = FileReference(type=container_type, id=str(container['_id']), name=file_['name']) - job = queue_job_legacy(alg_name, input_) + job = queue_job_legacy(gear_id, input_) else: inputs = { } for input_name, match_type in rule['match'].iteritems(): match = find_type_in_container(container, match_type) if match is None: - raise Exception("No type " + match_type + " found for alg rule " + alg_name + " that should have been satisfied") + raise Exception("No type " + match_type + " found for alg rule " + gear_tag + " that should have been satisfied") inputs[input_name] = FileReference(type=container_type, id=str(container['_id']), name=match['name']) - gear = gears.get_gear_by_name(alg_name) - job = Job(str(gear['_id']), inputs, tags=['auto', alg_name]) + job = Job(str(gear['_id']), inputs, tags=['auto', gear_tag]) potential_jobs.append({ 'job': job, @@ -248,7 +250,7 @@ def create_jobs(db, container_before, container_after, container_type): job_map = pj['job'].map() Queue.enqueue_job(job_map, origin) - spawned_jobs.append(pj['rule']['alg']) + spawned_jobs.append(pj['rule']['gear_id']) return spawned_jobs diff --git a/swagger/examples/input/rule-new.json b/swagger/examples/input/rule-new.json index f730cd4c8..65781a1e9 100644 --- a/swagger/examples/input/rule-new.json +++ b/swagger/examples/input/rule-new.json @@ -1,7 +1,7 @@ { "_id": "5a12f2923306be0016179f47", "project_id": "57e452791cff88b85f9f9c97", - "alg": "dcm2niix", + "gear_id": "580925ce9e512c57dc8a103c", "name": "dcm2niix", "all": [ { diff --git a/swagger/examples/output/rule-list.json b/swagger/examples/output/rule-list.json index eb6126a22..5d4db0e4a 100644 --- a/swagger/examples/output/rule-list.json +++ b/swagger/examples/output/rule-list.json @@ -1,6 +1,6 @@ [ { - "alg": "dcm2niix", + "gear_id": "580925ce9e512c57dc8a103b", "all": [ { "regex": true, @@ -17,7 +17,7 @@ "any": [] }, { - "alg": "dicom-mr-classifier", + "gear_id": "580925ce9e512c57dc8a103c", "all": [ { "type": "file.type", @@ -34,7 +34,7 @@ "any": [] }, { - "alg": "mriqc", + "gear_id": "580925ce9e512c57dc8a103d", "all": [ { "type": "file.type", diff --git a/swagger/examples/rules_list.json b/swagger/examples/rules_list.json index e35205841..b9e45e9fe 100755 --- a/swagger/examples/rules_list.json +++ b/swagger/examples/rules_list.json @@ -1,6 +1,6 @@ [ { - "alg" : "dicom_mr_classifier", + "gear_id": "580925ce9e512c57dc8a103c", "all" : [ [ "file.type", diff --git a/swagger/schemas/definitions/rule.json b/swagger/schemas/definitions/rule.json index d8fca6377..a73d37f93 100644 --- a/swagger/schemas/definitions/rule.json +++ b/swagger/schemas/definitions/rule.json @@ -29,7 +29,7 @@ "properties": { "_id": { "type": "string" }, "project_id": { "type": "string" }, - "alg": { "type": "string" }, + "gear_id": { "type": "string" }, "name": { "type": "string" }, "any": { "$ref": "#/definitions/rule-items" }, "all": { "$ref": "#/definitions/rule-items" }, @@ -43,7 +43,7 @@ "type": "object", "properties": { "_id": { "type": "string" }, - "alg": { "type": "string" }, + "gear_id": { "type": "string" }, "name": { "type": "string" }, "any": { "$ref": "#/definitions/rule-items" }, "all": { "$ref": "#/definitions/rule-items" }, diff --git a/swagger/schemas/input/rule-new.json b/swagger/schemas/input/rule-new.json index c456eaec4..aefa1c2ae 100644 --- a/swagger/schemas/input/rule-new.json +++ b/swagger/schemas/input/rule-new.json @@ -3,5 +3,5 @@ "title": "Rule", "type": "object", "allOf": [{"$ref": "../definitions/rule.json#/definitions/rule-input"}], - "required": ["alg", "name", "any", "all"] + "required": ["gear_id", "name", "any", "all"] } diff --git a/tests/integration_tests/python/test_jobs.py b/tests/integration_tests/python/test_jobs.py index ee020eda9..e1b09acad 100644 --- a/tests/integration_tests/python/test_jobs.py +++ b/tests/integration_tests/python/test_jobs.py @@ -60,7 +60,7 @@ def test_jobs(data_builder, default_payload, as_public, as_user, as_admin, as_ro job0 = copy.deepcopy(job_data) job0['gear_id'] = '000000000000000000000000' r = as_admin.post('/jobs/add', json=job0) - assert r.status_code == 400 + assert r.status_code == 404 # add job with explicit destination r = as_admin.post('/jobs/add', json=job_data) @@ -358,7 +358,6 @@ def test_failed_job_output(data_builder, default_payload, as_user, as_admin, as_ } gear = data_builder.create_gear(gear=gear_doc) gear2 = data_builder.create_gear() - gear2_name = as_admin.get('/gears/' + gear2).json()['gear']['name'] project = data_builder.create_project() session = data_builder.create_session() acquisition = data_builder.create_acquisition() @@ -366,7 +365,7 @@ def test_failed_job_output(data_builder, default_payload, as_user, as_admin, as_ # create rule for text files r = as_admin.post('/projects/' + project + '/rules', json={ - 'alg': gear2_name, + 'gear_id': gear2, 'name': 'text-trigger', 'any': [], 'all': [{'type': 'file.type', 'value': 'text'}] diff --git a/tests/integration_tests/python/test_rules.py b/tests/integration_tests/python/test_rules.py index 0616e3311..1981c539f 100644 --- a/tests/integration_tests/python/test_rules.py +++ b/tests/integration_tests/python/test_rules.py @@ -1,12 +1,9 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): - gear_name = randstr() - 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 = data_builder.create_gear(gear={'version': '0.0.1'}) + gear_2 = data_builder.create_gear(gear={'version': '0.0.1'}) rule = { - 'alg': gear_name, + 'gear_id': gear, 'name': 'csv-job-trigger-rule', 'any': [], 'all': [ @@ -38,7 +35,7 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): # attempt to add site rule with invalid regex invalid_pattern = '^(?non-image$).+' r = as_admin.post('/site/rules', json={ - 'alg': gear_name, + 'gear_id': gear, 'name': 'invalid-regex-rule', 'any': [], 'all': [ @@ -80,11 +77,11 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): # get specific site rule r = as_admin.get('/site/rules/' + rule_id) assert r.ok - assert r.json()['alg'] == gear_name + assert r.json()['gear_id'] == gear # PUT - update = {'alg': gear_2_name} + update = {'gear_id': gear_2} # attempt to modify site rule without admin r = as_user.put('/site/rules/' + rule_id, json=update) @@ -112,7 +109,7 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): assert r.ok r = as_admin.get('/site/rules/' + rule_id) assert r.ok - assert r.json()['alg'] == gear_2_name + assert r.json()['gear_id'] == gear_2 # DELETE @@ -133,11 +130,9 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): def test_site_rules_copied_to_new_projects(randstr, data_builder, file_form, as_admin, as_root): - gear_1_name = randstr() - gear_1 = data_builder.create_gear(gear={'name': gear_1_name, 'version': '0.0.1'}) - + gear_1 = data_builder.create_gear(gear={'version': '0.0.1'}) rule_1 = { - 'alg': gear_1_name, + 'gear_id': gear_1, 'name': 'csv-job-trigger-rule', 'any': [], 'all': [ @@ -145,11 +140,9 @@ def test_site_rules_copied_to_new_projects(randstr, data_builder, file_form, as_ ] } - 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={'version': '0.0.1'}) rule_2 = { - 'alg': gear_2_name, + 'gear_id': gear_2, 'name': 'text-job-trigger-rule', 'any': [], 'all': [ @@ -231,9 +224,7 @@ def test_site_rules_copied_to_new_projects(randstr, data_builder, file_form, as_ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, api_db): # create versioned gear to cover code selecting latest gear - 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 = data_builder.create_gear(gear={'version': '0.0.1'}) project = data_builder.create_project() bad_payload = {'test': 'rules'} @@ -273,7 +264,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.status_code == 403 rule_json = { - 'alg': 'non-existent-gear-name', + 'gear_id': '000000000000000000000000', 'name': 'csv-job-trigger-rule', 'any': [], 'all': [ @@ -284,11 +275,11 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # 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 r.status_code == 404 - # add project rule w/ proper gear alg + # add project rule w/ proper gear id # NOTE this is a legacy rule - rule_json['alg'] = gear_name + rule_json['gear_id'] = gear r = as_admin.post('/projects/' + project + '/rules', json=rule_json) assert r.ok rule = r.json()['_id'] @@ -296,7 +287,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # get project rules (verify rule was added) r = as_admin.get('/projects/' + project + '/rules') assert r.ok - assert r.json()[0]['alg'] == gear_name + assert r.json()[0]['gear_id'] == gear # try to get single project rule using non-existent rule id r = as_admin.get('/projects/' + project + '/rules/000000000000000000000000') @@ -311,12 +302,12 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.status_code == 404 # try to update rule w/ read-only project perms - r = with_user.session.put('/projects/' + project + '/rules/' + rule, json={'alg': gear_name}) + 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 alg - r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'alg': 'not-a-real-gear'}) - assert r.status_code == 400 + # try to update rule to with invalid gear id + r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'gear_id': '000000000000000000000000'}) + assert r.status_code == 404 # update name of rule rule_name = 'improved-csv-trigger-rule' @@ -333,7 +324,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok # test that job was created via rule - gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] + 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' @@ -358,7 +349,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # add valid container.has- project rule # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json={ - 'alg': gear_name, + 'gear_id': gear, 'name': 'txt-job-trigger-rule-with-measurement', 'any': [ {'type': 'container.has-measurement', 'value': 'functional'}, @@ -376,7 +367,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok # test that job was not created via rule - gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] + gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] assert len(gear_jobs) == 1 # still 1 from before # update test2.csv's metadata to include a valid measurement to spawn job @@ -404,7 +395,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok # test that only one job was created via rule - gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] + 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' @@ -416,7 +407,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # add regex rule # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json={ - 'alg': gear_name, + 'gear_id': gear, 'name': 'file-measurement-regex', 'any': [], 'all': [ @@ -431,7 +422,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok # test that job was created via regex rule - gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] + gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear})] assert len(gear_jobs) == 3 # delete rule @@ -443,11 +434,10 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a def test_disabled_rules(randstr, data_builder, api_db, as_admin, file_form): # Create gear, project and *disabled* rule triggering on any csv (once enabled) - gear_name = randstr() - gear = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.1'}) + gear = data_builder.create_gear(gear={'version': '0.0.1'}) project = data_builder.create_project() r = as_admin.post('/projects/' + project + '/rules', json={ - 'alg': gear_name, + 'gear_id': gear, 'name': 'csv-job-trigger-rule', 'any': [], 'all': [{'type': 'file.type', 'value': 'tabular data'}], diff --git a/tests/unit_tests/python/test_rules.py b/tests/unit_tests/python/test_rules.py index 8dca05532..e8e5402eb 100644 --- a/tests/unit_tests/python/test_rules.py +++ b/tests/unit_tests/python/test_rules.py @@ -165,7 +165,7 @@ def test_eval_rule_any(): }, ], 'all': [], - 'alg': 'dcm2nii', + 'gear_id': '000000000000000000000000', } file_ = {'name': 'hello.dcm', 'type': 'a'} @@ -200,7 +200,7 @@ def test_eval_rule_all(): 'value': '*.dcm', }, ], - 'alg': 'dcm2nii', + 'gear_id': '000000000000000000000000', } file_ = {'name': 'hello.dcm', 'type': 'a'} @@ -243,7 +243,7 @@ def test_eval_rule_any_all(): 'value': '*.dcm', }, ], - 'alg': 'dcm2nii', + 'gear_id': '000000000000000000000000', } file_ = {'name': 'hello.dcm', 'type': 'a'}