Fix subtle bug in listify function + simplify list munging

[jdavcs]

Subtle bug (discovered while upgrading SQLAlchemy syntax.)

The ModelManager._munge_filters method is supposed to "combine two lists into a single list" (as per docstring; which is also the right behavior). However, if one of the arguments is None and the other argument is not a list, it will return the other argument unchanged. This becomes a problem when one tries to iterate over the returned value, which is not an unlikely scenario:

filters = self._munge_filters(my_new_filter, current_filters)  # current_filters is None
stmt = select(Foo).where(*filters)  # this will break

The obvious fix is to replace the existing implementation with a one-liner reusing our existing listify util function on both arguments: return listify(listA) + listify(listB). However, this exposed a bug: if one of the arguments is an object with the special __bool__ method overridden, listify may return an empty list - which is the case with SQLAlchemy filters. A filter is a sql expression (e.g. Foo.bar == condition); so checking if not my_filter will simply apply not to the sql expression and negate it, returning True - and that 's where the code assumes the item as None and returns an empty list. Explicitly checking for None fixes this.

List munging moved to util.

Unit tests included.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[mvdbeek]

This seems like a pretty risky change. Can you move the special casing to munge_filters ? It does change things i.e. when item is 0.

[nsoranzo]

It surely is counter-intuitive that listify(0) and listify(False) return [], but as Marius says we cannot change the behaviour of this function any more.
We probably need a listify_new function that does what you were proposing here, and use that in new code / when refactoring.

[mvdbeek]

I don't think we want more functions in util/__init__.py, which we have to maintain indefinitely as the public api. Apart from that munge_lists is not a good name and we're not re-using this anywhere.

[mvdbeek]

@jmchilton can you elaborate ?

[jdavcs]

I've added a function to managers.base that handles just this use case (unlike the more generalized util.listify). Is this a better approach?

[mvdbeek]

Thank you, I think this looks good.

[nsoranzo]

The "Test Galaxy packages" failure is related.

[jdavcs]

This function is indeed a very special case: listify wouldn't have worked here. The arguments are not SQLAlchemy expressions (as I had naively assumed), but ParsedFilter objects - our own abstraction which is itself a tuple. Caught by our unit tests - good thing we had them!