Skip to content

Some SWF Filter Parameters should not be required. #1019

Closed
JimJty opened this Issue Sep 26, 2012 · 6 comments

3 participants

@JimJty
JimJty commented Sep 26, 2012

When calling boto.swf.layer1.list_closed_workflow_executions, the filters require all parameters. For example if I pass the workflow_name parameter to this function, I am required to pass the workflow_version also. This is inconvenient because the AWS api does not require both (WorkflowTypeFilter).
The same also applies to ExecutionTimeFilter, only one parameter is required out of the two, but boto requires both, which limits the functionality.

reference:
http://docs.amazonwebservices.com/amazonswf/latest/apireference/API_WorkflowTypeFilter.html
http://docs.amazonwebservices.com/amazonswf/latest/apireference/API_ExecutionTimeFilter.html

@oozie
oozie commented Oct 5, 2012

Sorry if I'm missing something obvious, but it seems that list_closed_workflow_executions only requires a domain as a positional argument and accepts everything else as keyword arguments. It does require a pair of timeFilter timestamps for either starting or closing period (unlike list_open_workflow_executions for obvious reasons). Also, it does not complain to me when I only pass workflow_name part of the filter and leave the version out. Is there an error message?

@JimJty
JimJty commented Oct 10, 2012

Yes it doesn't complain, but it doesn't work as it expected. In the code both workflow_name and workflow_version are required. If both aren't passed, workflow_name is ignored.

https://github.com/boto/boto/blob/develop/boto/swf/layer1.py#L1440

if workflow_name and workflow_version:
            data['typeFilter'] = {'name': workflow_name,
                                  'version': workflow_version}

I did this fix:

if workflow_name and workflow_version:
            data['typeFilter'] = {'name': workflow_name,
                                  'version': workflow_version}
elif workflow_name and not workflow_version:
            data['typeFilter'] = {'name': workflow_name}
elif not workflow_name and workflow_version:
            data['typeFilter'] = {'version': workflow_version}

and
https://github.com/boto/boto/blob/develop/boto/swf/layer1.py#L1426

    if start_latest_date and start_oldest_date:
            data['startTimeFilter'] = {'oldestDate': start_oldest_date,
                                       'latestDate': start_latest_date}
    if close_latest_date and close_oldest_date:
            data['closeTimeFilter'] = {'oldestDate': close_oldest_date,
                                       'latestDate': close_latest_date}

I did this fix:

       if start_latest_date and start_oldest_date:
            data['startTimeFilter'] = {'oldestDate': start_oldest_date,
                                       'latestDate': start_latest_date}
        elif start_latest_date and not start_oldest_date:
            data['startTimeFilter'] = {'latestDate': start_latest_date}
        elif not start_latest_date and start_oldest_date:
            data['startTimeFilter'] = {'oldestDate': start_oldest_date}

        if close_latest_date and close_oldest_date:
            data['closeTimeFilter'] = {'oldestDate': close_oldest_date,
                                       'latestDate': close_latest_date}
        elif close_latest_date and not close_oldest_date:
            data['closeTimeFilter'] = {'latestDate': close_latest_date}
        elif not close_latest_date and close_oldest_date:
            data['closeTimeFilter'] = {'oldestDate': close_oldest_date}
@oozie
oozie commented Oct 10, 2012

Ah, I see what you mean. There are two separate issues with the existing code:

  • 'Paternalistic' behavior as observed by JimJty
  • The spaghetti control structure,

It would be good to have Mitch (original author) confirm that the former is just a side effect. And if so, I'd like to propose an alternative solution untangling the spaghetti and simplifying request construction. It goes like this:

  • Each method would take the following form, defining all arguments in a dictionary.
def list_closed_workflow_executions(self, domain,
                                        start_latest_date=None,
                                        start_oldest_date=None,
                                        close_latest_date=None,
                                        close_oldest_date=None,
                                        close_status=None,
                                        tag=None,
                                        workflow_id=None,
                                        workflow_name=None,
                                        workflow_version=None,
                                        maximum_page_size=None,
                                        next_page_token=None,
                                        reverse_order=None):
    """Docstring""""
    data = {
        'domain': domain,
        'startTimeFilter': {'oldestDate': start_oldest_date,
                            'latestDate': start_latest_date},
        'closeTimeFilter': {'oldestDate': close_oldest_date,
                            'latestDate': close_latest_date},
        'closeStatusFilter': {'status': close_status},
        'tagFilter': {'tag': tag},
        'typeFilter': {'name': workflow_name,
                       'version': workflow_version},
        'executionFilter' = {'workflowId': workflow_id}
    }
    return self.make_request('ListClosedWorkflowExecutions', data)
  • self.make_request would be corrected to invoke a recursive class method _normalize_json(data) that clears the dictionary of all None defaults.
@classmethod
def _normalize_json(cls, data):
    for item in data:
        if isinstance(data[item], dict):
            cls._normalize_json(data[item])
        if data[item] in (None, {}):
            del data[item]
    return 
  • The json.dumps(data) part would serialize JSON also within make_request().

This solution would untangle and de-duplicate much of the code making it more maintainable.

garnaat: if you agree to the proposal I'll be happy to take the implementation upon myself. The change would be backwards compatible.

@garnaat
the boto project member
garnaat commented Oct 14, 2012

I'm not sure I'd call the control structure "spaghetti". Macaroni, perhaps, but spaghetti? I don't think so 8^)

Feel free to rework this. Backwards-compatibility is pretty important at this point. Thanks!

@oozie
oozie commented Oct 18, 2012

Yes, I meant macaroni!

@garnaat
the boto project member
garnaat commented Nov 14, 2012

The PR referenced above has been merged.

@garnaat garnaat closed this Nov 14, 2012
@msabramo msabramo pushed a commit to msabramo/boto that referenced this issue Nov 28, 2012
@oozie oozie Fix for #1019 / rework of request building logic. f7c1b29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.