Properly validate RequestPriority parameter #9994
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Tests went fine! |
There was a problem hiding this comment.
@amaltaro I made one comment in the code. Please take a look at it. Thanks!
| reqPrioDefin = StdBase.getWorkloadCreateArgs()['RequestPriority'] | ||
| try: | ||
| # Web GUI sends it as a string | ||
| reqArgs['RequestPriority'] = int(reqArgs['RequestPriority']) |
There was a problem hiding this comment.
Wouldn't that be more clear if one just does the following here:
if not isinstance(reqArgs['RequestPriority'], reqPrioDefin['type']):
raise InvalidSpecParameterValue(msg)
We need to check for the type only and not for the possibility to cast the object to a given type, because we never do so down the code. And we will also avoid hardcoding a int type check in the middle of the code while we have the object type defined from StdBase. I think we should take it from the upper level object otherwise we risk to have objects that silently transform from one type to another, which in my opinion is a bug.
There was a problem hiding this comment.
Good catch. I meant to write:
reqArgs['RequestPriority'] = reqPrioDefin['type'](reqArgs['RequestPriority'])
About simply checking the data type, it would be easier if we were already running this software in python3. Since we are still in py2, any check for string is just another complication in the near future.
|
Jenkins results:
|
amaltaro
force-pushed
the
fix-9992
branch
2 times, most recently
from
edaa43f to
8a5d174
Compare
improve error message cast using type from stdbase use type check remove blank space fix message
fix unit tests
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
@todor-ivanov sorry for bugging you again here. I didn't manage to reproduce the string type on my VM, neither via script, nor via Gui. So I made the change to simply check for the data type. Can you please have another look? Thanks |
|
Thanks! |
Fixes #9992
Status
ready
Description
Make sure the
RequestPriorityspec parameter is properly validated, regardless of which state the workflow is.For clarity: it must be of the
integertype and be in the range of[0, 1e6]Is it backward compatible (if not, which system it affects?)
Yes (well, we need to chase those requests with float value, otherwise an ACDC might fail to be created...)
Related PRs
none
External dependencies / deployment changes
none