rest: add share_workflow endpoint #538
Conversation
Adds a new endpoint to share a workflow with a user. Closes reanahub/reana-client#680
DaanRosendal
force-pushed
the
feature/share-add
branch
from
5aaee87
to
84ce2ed
Compare
| from sqlalchemy import and_, nullslast, or_ | ||
| from sqlalchemy.orm import aliased | ||
| from webargs import fields | ||
| from webargs.flaskparser import use_args, use_kwargs |
There was a problem hiding this comment.
These imports should be placed before importing REANA packages, see https://github.com/reanahub/reana/wiki/Tips-for-Python#imports
There was a problem hiding this comment.
Done.
The change has been applied to this superseding PR: #552
| @blueprint.route("/workflows/<workflow_id_or_name>/share", methods=["POST"]) | ||
| @use_kwargs( | ||
| { | ||
| "user_id": fields.Str(required=True), |
There was a problem hiding this comment.
Let's change the name of the parameter user_id to user, to be consistent with the other endpoints. It needs to be changed here, in the OpenAPI spec and in the function body
There was a problem hiding this comment.
Done.
The change has been applied to this superseding PR: #552
| "user_email_to_share_with": fields.Str(required=True), | ||
| "message": fields.Str(required=False), | ||
| "valid_until": fields.Str(required=False), |
There was a problem hiding this comment.
I would personally put these parameters (user_email_to_share_with, message and valid_until) in the request body instead of sending them as query parameters, for two reasons:
messagecan be very long, and having long URLs can cause troubles- this is a POST request, and the details of the "resource" (in this case, the sharing request) that needs to be created are usually passed as part of the body
What do you think?
There was a problem hiding this comment.
I agree with you. Thanks for the suggestion.
The change has been applied to this superseding PR: #552
| errors: | ||
| type: array | ||
| items: | ||
| type: string |
There was a problem hiding this comment.
Please correct me if I am wrong, but I think we never return in any other endpoint an array of errors. If this is not needed, let's stick to the usual way of just returning a message, to be consistent with the rest.
What do you think? This needs to be changed also in the specification of the other return codes.
There was a problem hiding this comment.
You are right, nicely spotted.
The change has been applied to this superseding PR: #552
| schema: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| examples: | ||
| application/json: | ||
| { | ||
| "errors": ["User is not allowed to share the workflow."] | ||
| } |
There was a problem hiding this comment.
Schema and examples do not match
There was a problem hiding this comment.
The 403 has been removed, see this comment: #538 (comment)
| @use_kwargs( | ||
| { | ||
| "user_id": fields.Str(required=True), | ||
| "user_email_to_share_with": fields.Str(required=True), |
There was a problem hiding this comment.
We could even use fields.Email here, but it's not strictly necessary as we are only using it for lookups (and we are storing it only if an user with that email exists): https://marshmallow.readthedocs.io/en/stable/marshmallow.fields.html#marshmallow.fields.Email
There was a problem hiding this comment.
Idem. Having refactored user_email_to_share_with to be in the JSON request body, is this still possible?
| raise ValueError("Unable to share a workflow with yourself.") | ||
|
|
||
| user_to_share_with = ( | ||
| Session.query(User).filter(User.email == user_email_to_share_with).first() |
There was a problem hiding this comment.
minor: we already have an unique index on email so it does not really matter, but to be pedantic this should be one_or_none() and not first(), to make sure there is only one user returned by the query.
Not really needed to change it here, but I personally like it more as it helps to find wrong SQL queries where multiple rows are returned while we expected only one of them. A common mistake is to forget one filtering condition and you are suddenly returning the wrong data, or even worse applying actions to the wrong user/workflow! (this is not the case here, though)
There was a problem hiding this comment.
I changed it to be one_or_none()
The change has been applied to this superseding PR: #552
|
|
||
| if valid_until: | ||
| try: | ||
| datetime.date.fromisoformat(valid_until) |
There was a problem hiding this comment.
Parsing the date should not be needed here if we define it as fields.Date, see the previous related comment
There was a problem hiding this comment.
Idem. Having refactored valid_until to be in the JSON request body, this check should stay if there is no way to use the fields.Date field.
| if datetime.date.fromisoformat(valid_until) < datetime.date.today(): | ||
| raise ValueError("The 'valid_until' date cannot be in the past.") | ||
|
|
||
| if message and len(message) > 5000: |
There was a problem hiding this comment.
A couple of comments:
- we can validate the length of the string already inside
use_kwargs, like this (not tested):
"message": fields.Str(required=False, validate=validate.Length(max=5000))- I think we should put this value in
config.py(but no need to read it from an env variable) to avoid having5000hardcoded in the code
What do you think?
There was a problem hiding this comment.
Idem. Having refactored message to be in the JSON request body, is this still possible?
I did create a config value MAX_WORKFLOW_SHARING_MESSAGE_LENGTH. The change has been applied to this superseding PR: #552
| existing_share = ( | ||
| Session.query(UserWorkflow) | ||
| .filter_by(user_id=user_to_share_with.id_, workflow_id=workflow.id_) | ||
| .first() | ||
| ) | ||
|
|
||
| if existing_share: | ||
| return ( | ||
| jsonify( | ||
| { | ||
| "message": f"{workflow.get_full_workflow_name()} is already shared with {user_email_to_share_with}." | ||
| } | ||
| ), | ||
| 409, | ||
| ) | ||
|
|
||
| Session.add( | ||
| UserWorkflow( | ||
| user_id=user_to_share_with.id_, | ||
| workflow_id=workflow.id_, | ||
| message=message, | ||
| valid_until=valid_until, | ||
| ) | ||
| ) | ||
| Session.commit() |
There was a problem hiding this comment.
There is a potential race condition here, as this can happen:
existing_shareis fetched from the database, but it does not exist soNoneis returned- another (concurrent) API request creates the workflow share
- we try to create a new share, but
Session.add/Session.commitwill fail as the share already exists
Note that in this case nothing bad happens, as the database will simply return an integrity error (given that there will be multiple rows with the same primary keys). However, we could prevent this issue by simply inserting the new row without checking first if it exists:
try:
Session.add(UserWorkflow(...))
Session.commit()
except IntegrityError:
Session.rollback()
return jsonify(...), 409Also see https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
There was a problem hiding this comment.
Nicely spotted! 👍
The change has been applied to this superseding PR: #552
| "message": "Malformed request.", | ||
| "errors": ["Missing data for required field."] | ||
| } | ||
| 403: |
There was a problem hiding this comment.
I think the 403 error is never returned?
There was a problem hiding this comment.
That is correct, nicely spotted. I removed it. In the future once this is implemented (reanahub/reana-server#663) it could be readded.
The change has been applied to this superseding PR: #552
Adds a new endpoint to share a workflow with a user.
Closes reanahub/reana-client#680