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

isin method should not use narrow isinstance for generating criterion #668

Open
JordanReiter opened this issue Feb 11, 2022 · 1 comment

Comments

@JordanReiter
Copy link

This is the code:

def isin(self, arg: Union[list, tuple, set, "Term"]) -> "ContainsCriterion":
    if isinstance(arg, (list, tuple, set)):
        return ContainsCriterion(self, Tuple(*[self.wrap_constant(value) for value in arg]))
    return ContainsCriterion(self, arg)

You'll notice that if it is anything other than a list, tuple, or set, the code assumes it is already a type that the query builder can handle "natively" without conversion.

If you send it an iterable other than the ones specified above, you will get a failure later on, namely:

AttributeError: <object type> object has no attribute 'nodes_'

In my case I was using:

....where(col.isin(someval.keys()))

And got the error

AttributeError: 'dict_keys' object has no attribute 'nodes_'

But this could just as easily happen with someval.values() or even some custom-built iterable used in the code.

Unless I'm missing something, the desired input for ContainsCriterion would always be a Tuple object so perhaps a better implementation would be:

def isin(self, arg: Union[list, tuple, set, "Term"]) -> "ContainsCriterion":
    if isinstance(arg, Tuple):
        return ContainsCriterion(self, arg)
    return ContainsCriterion(self, Tuple(*[self.wrap_constant(value) for value in arg]))

Just a thought. If Tuple is not always the right input, then some class that implements a base class of Tuple should be used in isinstance. Limiting the inputs to one of three types (common as they may be) leads to unexpected errors such as the one I listed above.

They may even be other methods that use this isinstance check, this is just the only one I found.

@doc-sheet
Copy link

I guess isin() function should also understand *args
i.e. table.column.isin('value1', 'value2')

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

No branches or pull requests

2 participants