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

1090 fix(m2m): Return empty list if there are no m2m entries #1089

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

nVitius
Copy link
Contributor

@nVitius nVitius commented Sep 30, 2024

I ran into an issue where running getm2m without any existing m2m entries will throw an error that I believe is unintended.
image

@sinisaos
Copy link
Member

sinisaos commented Oct 1, 2024

@nVitius Thank you for your PR. I think this error is intentional and tells us that the list of values ​​cannot be empty. I think you should rather handle ValueError in your code directly using try/except. Sorry if I'm wrong.

@nVitius
Copy link
Contributor Author

nVitius commented Oct 1, 2024

Seems unintuitive to throw an error instead of returning an empty list...
Also, the error isn't very obvious, as it's coming from the is_in call within the get_m2m method.

To clarify, the error is indicating that is_in didn't receive any id's to check. Not that there are no entries.

@dantownsend
Copy link
Member

I'm just looking at this now.

@dantownsend
Copy link
Member

Yeah, I think this is a bug.

@sinisaos You make a good point - if someone calls is_in directly with no values, then I think raising an error is OK. But for this particular query, the user isn't calling is_in directly, so we should handle this edge case and return an empty list.

@dantownsend dantownsend changed the title fix(m2m): Return empty list if there are no m2m entries 1090 fix(m2m): Return empty list if there are no m2m entries Oct 1, 2024
@dantownsend
Copy link
Member

Resolves #1090

@sinisaos
Copy link
Member

sinisaos commented Oct 1, 2024

Seems unintuitive to throw an error instead of returning an empty list... Also, the error isn't very obvious, as it's coming from the is_in call within the get_m2m method.

To clarify, the error is indicating that is_in didn't receive any id's to check. Not that there are no entries.

@nVitius Sorry for my mistake. Thanks for the clarification.

@dantownsend
Copy link
Member

@sinisaos No need to apologise

@dantownsend
Copy link
Member

OK, I'm going to throw up a quick test, then will get this merged in. Thanks both 👍

@dantownsend
Copy link
Member

@nVitius Thanks a lot for the fix - it will be in the next release 👍

@dantownsend dantownsend merged commit 5173a39 into piccolo-orm:master Oct 1, 2024
46 checks passed
@nVitius nVitius deleted the patch-1 branch October 1, 2024 08:45
@dantownsend
Copy link
Member

The fix is in piccolo==1.19.1.

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.

3 participants