Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/all variables injected when function has no variables #348

Conversation

ohasi
Copy link
Contributor

@ohasi ohasi commented Jul 8, 2023

Fix bug where function decorated as task receives all variables that are available to the task causing an unexpected keyword argument error. Referenced in #337.

The bug originates from the zeebe grpc API for ActivateJobs. If no variables are passed to fetchVariable , it fetches all available variables.

Changes

  • Add a include_variables flag to private function _create_job_from_raw_job. If flag is set to False, retrieved variables will be ignored.
    The function is called in one place, which now calls it with bool(variables_to_fetch) as the flag's value. None or an empty list will cause no variables to be injected to the Job object, so they will never end up getting passed to the function.

API Updates

New Features (required)

  • Support for functions with no arguments as task functions

Checklist

  • Unit tests
  • Documentation

References

ActivateJobs API

Fixes #337

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2023

CLA assistant check
All committers have signed the CLA.

@JonatanMartens
Copy link
Collaborator

Looks good, but please add a simple unit test so that this bug doesn't reproduce.

@ohasi
Copy link
Contributor Author

ohasi commented Jul 22, 2023

Looks good, but please add a simple unit test so that this bug doesn't reproduce.

Done.
Something to have in mind that someone mentioned to me - this pr will remove the ability to intentionally get all parameters by setting the variables_to_fetch to [] or None.
I don't think that's a problem or intended functionality, but just making sure

@@ -71,7 +72,7 @@ def task(
def task_wrapper(task_function: Callable):
config = TaskConfig(
task_type,
exception_handler,
exception_handler or self._default_exception_handler,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add a small unit test for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it in #349

def _build_dict_from_list(keys: List[str], *values):
if not values:
return dict(zip(keys, keys))
assert len(keys) == len(values)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add this imo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed :)

tests/unit/utils/random_utils.py Outdated Show resolved Hide resolved
ohasi and others added 3 commits February 25, 2024 19:06
Co-authored-by: Jonatan Martens <40060128+JonatanMartens@users.noreply.github.com>
@ohasi
Copy link
Contributor Author

ohasi commented Feb 25, 2024

Waiting for re-review :)

Copy link
Contributor Author

@ohasi ohasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for re-review :)

def _build_dict_from_list(keys: List[str], *values):
if not values:
return dict(zip(keys, keys))
assert len(keys) == len(values)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed :)

@@ -71,7 +72,7 @@ def task(
def task_wrapper(task_function: Callable):
config = TaskConfig(
task_type,
exception_handler,
exception_handler or self._default_exception_handler,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it in #349

@dimastbk
Copy link
Collaborator

dimastbk commented Jun 3, 2024

fixed in #404

@dimastbk dimastbk closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All variables from process are injected into a task function that takes in no parameters
4 participants