Adding get_by_ref for ParametersViewController #5509
Conversation
|
|
f93652b to
d01e886
Compare
d01e886 to
669a6a0
Compare
669a6a0 to
9a70741
Compare
9a70741<
8000
/code> to
b69694f
Compare
6a1f956 to
9a2efa9
Compare
There was a problem hiding this comment.
Thanks for the contirbution.
It looks good to me, just a few minor comments but please also add an entry to CHANGELOG.rst.
I wouldn't mind another TSC member to check as well, as I'm not sure if there are other consequences for changing the outer method to lookup on id or ref.
Also might be worth a mention of the use-case in the Conversation for the need for the get_one to also get by reference.
| def _get_action_by_ref(ref): | ||
| try: | ||
| action_db = Action.get_by_ref(ref) | ||
| if not action_db: |
There was a problem hiding this comment.
Looking at the other methods, then we don't usually check the value of the return value from get_by_id etc, just return it. So for consistency I would do the same and return None rather than throw an exception to be consistent with others.
There was a problem hiding this comment.
Thing is I wanted to throw a meaningful exception in case it was not found in DB same there is for get_by_id.
When I perform a lookup for non existing id I get:
{
"faultstring": "Database lookup for id=\"61b9fa409a66b66d529d8345\" resulted in exception. Unable to find the ActionDB instance. {'id': '61b9fa409a66b66d529d8345'}"
}
@amanda11 Seems you are right, I assume it should behave the same for Action.get_by_ref:
st2/st2common/st2common/models/db/__init__.py
Line 521 in 76feb92
There was a problem hiding this comment.
@amanda11 did a re-check and seems I need to handle that exception as when I perform a lookup for a non exesting action ref I get:
{
"faultstring": "Internal Server Error"
}
Full trace from st2api:
2021-12-28 09:36:27,898 ERROR [-] Failed to call controller function "get_one" for operation "st2api.controllers.v1.action_view
s:parameters_view_controller.get_one": 'NoneType' object has no attribute 'get_uid'
Traceback (most recent call last):
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2common/router.py", line 516, in __call__
resp = func(**kw)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2api/controllers/v1/action_views.py", line 84, in get_one
return self._get_one(action_id, requester_user=requester_user)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2api/controllers/v1/action_views.py", line 104, in _get_one
permission_type=permission_type,
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2rbac_backend/utils.py", line 123, in assert_user_has_resource_db_per
mission
permission_type=permission_type)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2rbac_backend/utils.py", line 282, in user_has_resource_db_permission
permission_type=permission_type)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2rbac_backend/resolvers.py", line 415, in user_has_resource_db_permis
sion
action_uid = resource_db.get_uid()
AttributeError: 'NoneType' object has no attribute 'get_uid'
2021-12-28 09:36:27,898 ERROR [-] API call failed: 'NoneType' object has no attribute 'get_uid'
Traceback (most recent call last):
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2common/middleware/error_handling.py", line 49, in __call__
return self.app(environ, start_response)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2common/middleware/streaming.py", line 48, in __call__
return self.app(environ, start_response)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2common/router.py", line 599, in as_wsgi
resp = self(req)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2common/router.py", line 524, in __call__
raise e
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2common/router.py", line 516, in __call__
resp = func(**kw)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2api/controllers/v1/action_views.py", line 84, in get_one
return self._get_one(action_id, requester_user=requester_user)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2api/controllers/v1/action_views.py", line 104, in _get_one
permission_type=permission_type,
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2rbac_backend/utils.py", line 123, in assert_user_has_resource_db_per
mission
permission_type=permission_type)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2rbac_backend/utils.py", line 282, in user_has_resource_db_permission
permission_type=permission_type)
File "/opt/stackstorm/st2/lib/python3.6/site-packages/st2rbac_backend/resolvers.py", line 415, in user_has_resource_db_permis
sion
action_uid = resource_db.get_uid()
AttributeError: 'NoneType' object has no attribute 'get_uid' (_exception_class='AttributeError',_exception_message="'NoneType'
object has no attribute 'get_uid'",_exception_data={})
Seems it is not raising the exception when the action reference is not found
| @@ -81,7 +94,10 @@ def _get_one(action_id, requester_user): | |||
| Handle: | |||
| GET /actions/views/parameters/1 | |||
| """ | |||
| action_db = LookupUtils._get_action_by_id(action_id) | |||
| if ResourceReference.is_resource_reference(action_id): | |||
There was a problem hiding this comment.
This method has changed so that it isn't looking up by action_id, but rather now action_id or action_ref. So we should change the pydoc to make that clear.
94560f4 to
02fc6cb
Compare
02fc6cb to
56b41f9
Compare
Done: |
56b41f9 to
e28d2d6
Compare
1af9c3e to
4fbdbee
Compare
CHANGELOG.rst
Outdated
| Added | ||
| ~~~~~ | ||
|
|
||
| * Added st2 API get action parameters by ref. |
There was a problem hiding this comment.
Please add reference to the pull request, e.g. #5509 (see the other formats in the file). That way its easy to track PRs from changelog.
Also each new entry doesn't need a header "added", just add to the existing "Added" section already in place for the current "in development" section.
| raise ValueError('Referenced action "%s" doesnt exist' % (ref)) | ||
| return action_db | ||
| except Exception as e: | ||
| msg = 'Database lookup for ref="%s" resulted in exception. %s' % (ref, e) |
There was a problem hiding this comment.
It seems a little odd to raise an exception in this method only to re-catch it 2 lines down. But equally you need to handle generic and the None being returned, so not sure I could think of a cleaner way.
There was a problem hiding this comment.
LGTM overall 👍
To make sure the https://api.stackstorm.com/api/v1/actions/#/parameters_view_controller.get_one will be updated, we'll also need to include in this PR the new Openapi spec with adjusted endpoint
st2/st2common/st2common/openapi.yaml
Lines 1 to 5 in 38012c9
4fbdbee to
91d0d48
Compare
I have a UI where I'm using the GET action parameters by action ref and then creating an execution (with the POST execution) so I need to use the action ref from the previous call (GET action parameters). |
91d0d48 to
944e7cd
Compare
I have adjusted: But can't seem to succeed in regenarating it: And ofcourse the CI is failing for that: |
@DavidMeu If you un-do your changes on your local system, and you run the regenerate command does it work? Just wondering if the older version can't be re-generated either.... |
Actually the problem is that the Makefile isn't coping because you have a ( in your path name. The error is about the "(" in the pathname. |
8b846ac to
9f89e78
Compare
9f89e78 to
15e7bd9
Compare
15e7bd9 to
0a1ab72
Compare
Adding ability to get action parameters by reference.