Refactor the orquesta st2kv function #4692
Conversation
m4dcoder
requested review from
jinpingh and
bigmstone
and removed request for
jinpingh
| @@ -25,10 +25,12 @@ | |||
| LOG = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| def st2kv_(context, key, decrypt=False): | |||
| def st2kv_(context, key, **kwargs): | |||
There was a problem hiding this comment.
Can we explicitly declare keyword arguments here? Not a fan of **kwargs since it doesn't make it immediately obvious which arguments a function takes.
I assume you did that to being able to distinguish if a default value is specified or not. Another way to handle that would be to set a default value of this keyword argument to some special value.
There was a problem hiding this comment.
Yes. I did this to distinguish whether default value is actually provided and not to be confused if someone wants default to be None. Use of kwargs is more elegant in this case IMO. But if this is what's blocking, then I can assign some bogus value to default. I'll assign it KAMI_DONT_WANT_KWARGS. :P
There was a problem hiding this comment.
@Kami Looks like I won't be able to change to an explicit arg for default. When I set the default to some constant value, there may be a bug in YAQL that tries to make the parameter strongly typed. When I try to use the function in YAQL expression, YAQL returns error saying it cannot recognize the signature. If I set default=None, there's no problem. I also try this with Jinja and there's no problem setting default="SOME_CONSTANTS". So at this point, you'll have to live with using kwargs here.
| @@ -107,6 +109,11 @@ def get_by_scope_and_name(cls, scope, name): | |||
| :rtype: :class:`KeyValuePairDB` or ``None`` | |||
| """ | |||
| query_result = cls.impl.query(scope=scope, name=name) | |||
|
|
|||
| if not query_result: | |||
There was a problem hiding this comment.
Wonder how this change affects other code like datastore service, etc. which uses this function.
Also looking at the code below, we used to return None if value didn't exist so just need to make sure all the code which uses it has also been updated and it has no negative side affects.
EDIT: I see you updated the code to handle this exception.
There was a problem hiding this comment.
Yep, it's handled in the LookUp classes.
| ) | ||
| # Decrypt in deserialize_key_value cannot handle NoneType. | ||
| if kvp.value is None: | ||
| return kvp.value |
There was a problem hiding this comment.
Wonder in which scenarios we could end up here since we only support strings for datastore values aka when None can be set / returned for a datastore value attribute.
There was a problem hiding this comment.
That may be true from normal usage. But nothing is preventing a None value from being written into the KeyValuePairDB record.
|
@Kami TBH, the keyvalue/datastore utils/services are very inconsistent. Most of our services modules are just functions. But the keyvalue service have separate classes in them. And then we have datastore service which is confusing. The functions in the keyvalue util should probably be in the service module. They would be more consistent with functions of other services. And then low level DB query should return error when record not found. The utils/service can abstract that and decide whether to throw exception. |
m4dcoder
force-pushed
the
fix-orquesta-st2kv
branch
from
8991d11
to
8464364
Compare
Currently, if the key-value pair in the StackStorm datastore has empty string or NoneType as value, the orquesta st2kv function will return the error that key does not exist. This patch fixes the issue and return the empty string or None value.
Make the parameters for the st2kv function into kwargs to prepare for adding the parameter for default value. The parameter for default value is optional and would have been assigned None by default. The function would have no way to determine when a key does not exists whether to return a default value of None. By making the parameters into kwargs, the function can see if the default key is in the kwargs to determine what default value to return if a key does not exists. The default value can support returning a value of NoneType without being confused whether a default value is set or not.
Shorten the variable name to give more room for code without extending code to separate line.
Add a parameter named "default" for the orquesta st2kv function to return the default value if the key does not exists in the StackStorm datastore. Add unit and integration tests for the feature.
Refactor the try block and only return default value if the StackStormDBObjectNotFoundError is raised.
Make sure both YAQL and Jinja variants for the orquesta st2kv usage are tested.
Fix a minor lint error in the orquesta st2kv module.
m4dcoder
force-pushed
the
fix-orquesta-st2kv
branch
from
fed12fd
to
beb9346
Compare
Fix the orquesta st2kv to return empty string and null values correctly and allow st2kv function to return a supplied default value for nonexistent key. Closes #4678