PUT /executions/{executionIdentifier} #36
Conversation
| return func(model=body, *args, **kwargs) | ||
| else: | ||
| return ErrorCodeAndMessageMarshaller( | ||
| INVALID_MODEL_PROVIDED), 400 | ||
|
|
||
| model, errors = schema.load(body) | ||
| if (errors): | ||
| model, errors = schema.load(body, partial=partial) |
There was a problem hiding this comment.
How that's really nice!
| @get_db_session | ||
| @unmarshal_request(ExecutionSchema(), partial=True) | ||
| @marshal_response() | ||
| def put(self, model, execution_identifier, user, db_session): |
There was a problem hiding this comment.
Please replace put with PATCH. We will also change this in the OpenAPI 3.0 descriptor to reflect the changes discussed with @glatard. I tested it and patch works with flask-restful.
There was a problem hiding this comment.
I didn't want to do this before the change was approved in https://github.com/fli-iam/CARMIN. The issue has been created (CARMIN-org#86), but I don't know of a pull request that was merged. @glatard should we wait before a PR is merged before implementing it as PATCH?
| name: str, | ||
| pipeline_identifier: str, | ||
| input_values: object, | ||
| name: str = None, |
There was a problem hiding this comment.
At first I didn't want to allow none for these values as they are illegal and should never be None, but as we always do the transfer via the Schema and it performs the validation this is ok. I will change my call to this constructor in the POST.
There was a problem hiding this comment.
This was my opinion as well. I don't think it causes any issues, and remains safe.
| response = test_config.test_client.put( | ||
| '/executions/{}'.format(execution_id), | ||
| headers={"apiKey": standard_user().api_key}, | ||
| data=json.dumps(PATCH_VALID_EXECUTION)) |
There was a problem hiding this comment.
For uniformity, we should use json_request_data utils function. I just realized it isn't used in Executions either. It does the same thing, except it was placed under a standard utils function. However, do you feel like it may be a bit overkill to have such utils function? I did it back when I wrote our first unit tests to insure uniformity, but rethinking about it, as there isn't any fancy parameter used for the moment, we could simply call json.dumps. What are you thoughts on that @louis-ver?
There was a problem hiding this comment.
I don't see the need for the json_request_data function. It is less explicit and does not really provide an advantage.
However, I think that load_json_data is useful, since the actual json function is a little verbose and not really helpful.
I say we remove json_request_data, but keep load_json_data
| "apiKey": standard_user().api_key | ||
| }) | ||
| execution = load_json_data(response) | ||
| assert execution["name"] == PATCH_VALID_EXECUTION2["name"] |
There was a problem hiding this comment.
Please make sure the change took place in the DB. You can find an example in test_register_already_existing_user_folder (register_test.py). This will insure that the DB changes really took place.
There was a problem hiding this comment.
I replaced the GET request with a DB query instead. Didn't know how to do that. Thanks for the head's up!
| "apiKey": standard_user().api_key | ||
| }) | ||
| execution = load_json_data(response) | ||
| assert execution["timeout"] == PATCH_VALID_EXECUTION3["timeout"] |
There was a problem hiding this comment.
Please make sure the change took place in the DB. You can find an example in test_register_already_existing_user_folder (register_test.py). This will insure that the DB changes really took place.
| '/executions/{}'.format(execution_id), | ||
| headers={"apiKey": standard_user().api_key}, | ||
| data=json.dumps(PATCH_INVALID_PARAMETER)) | ||
| assert response.status_code == 400 |
There was a problem hiding this comment.
This 400 is caused by an exception in the DB right? I that case I believe you should add a try catch as done with register, and return a proper ErrorCodeAndMessage.
There was a problem hiding this comment.
No, the 400 is caused in the unmarshaling of the request, inside the @unmarshal_request decorator. The model is invalid, since timeout should be an integer.
* GET /execution/{executionIdentifier}
* PUT /executions/{executionIdentifier} (#36)
* PUT /executions/{executionIdentifier}
* Add tests for PUT /executions/
* Fix bug in execution change logic
* Add more checking to test results
* Refactor for less db calls
* Query DB instead of issuing GET request
* Removing non required params
* Add execution_id to EXECUTION_NOT_FOUND error
No description provided.