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

Second solution for IndexError problem in QuerySet #3277

Closed
wants to merge 2 commits into from

Conversation

artemrizhov
Copy link
Contributor

This is a fix for problem described in ticket #23555 https://code.djangoproject.com/ticket/23555

I've sent a pull request with a fix already #3274
But if somebody thinks the code becomes too complicated and prefers to use try/catch despite the problem then I propose this solution with special exception class. Now you can use this safely anywhere:

try:
    return qs[0]
except QuerySetIndexError:
    return None

@tchaumeny
Copy link
Contributor

The test you're providing doesn't show a flaw in the current implementation, as you have to create that flaw in the test itself. See https://code.djangoproject.com/ticket/23555#comment:3 which provides examples of such IndexError in cPython itself.

@artemrizhov
Copy link
Contributor Author

@tchaumeny The test that I've provided does show the problem. It demostrates that external errors are not handled right way. Please see my comment on the ticket for more explanation https://code.djangoproject.com/ticket/23555#comment:4, and also please read the comments (there is a lot of) on the tests code and on the model used for tests, and also please read the comments on first pull request #3274.
Also please note that this is alternate solution for the case the maintainers think such approach is better. Imho, first solution demonstrates the probalem much better (see #3274).

@timgraham
Copy link
Member

The first solution seems simpler, although I'm not entirely sold on it. Someone more familiar with the ORM may provide more feedback.

@timgraham timgraham closed this Sep 30, 2014
@artemrizhov
Copy link
Contributor Author

The first solution seems simpler

Yes, it does. However please note that it does not provide a way for safe use of try/except with queryset's IndexError. On the current stage this just should be avoided.

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