Skip to content

Get rid of ValuesQuerySet #608

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.

Sign up for GitHub

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

Jump to bottom
Closed
sobolevn opened this issue May 1, 2021 · 9 comments
Closed

Get rid of ValuesQuerySet #608

sobolevn opened this issue May 1, 2021 · 9 comments
Labels
bug Something isn't working

Comments

@sobolevn
Copy link
Member

sobolevn commented May 1, 2021

Problems with QuerySet / ValuesQuerySet separation:

  • There are a lot of places, where we use QuerySet annotation, so ValuesQuerySet is not allowed there.
  • We cannot simply change it, because _BaseQuerySet is not enough for small portion of cases
  • T type var has models.Model bound, which stops us from annotating thing with, for example, QuerySet[SomeTypedDict], which is valid when we use .values and SomeTypedDict represents the final result of this call
@sobolevn sobolevn added the bug Something isn't working label May 1, 2021
@sobolevn sobolevn mentioned this issue May 1, 2021
Closed
@sobolevn
Copy link
Member Author

sobolevn commented May 1, 2021

Related: 6522f30

@syastrov
Copy link
Contributor

syastrov commented May 8, 2021

But should a QuerySet where .values was called still have the original Model in its type?

It might be relevant if I were to call .filter after calling .values to know which type of Model is being queried so that the type-checker can generate an error if I filtered on an invalid field.

If Django hadn't allowed users to "flaunt their individualism", this would be a lot simpler :)

https://docs.djangoproject.com/en/3.2/ref/models/querysets/#django.db.models.query.QuerySet.values

The people who made Django prefer to put all the SQL-affecting methods first, followed (optionally) by any output-affecting methods (such as values()), but it doesn’t really matter. This is your chance to really flaunt your individualism.

Also, another weird edge-case is that you can call .bulk_create after calling .values. There, knowledge of the Model is also useful for type-checking.

However, we can just decide to go after simplicity rather than absolute correctness, and accept that these uncommon patterns of use are likely to result in less type-checking or false positives.

@sobolevn
Copy link
Member Author

sobolevn commented May 8, 2021 •
edited
Loading

I thing that we can try to add the second type argument dynamically to QuerySet instance when we need to (basically after .values / similar calls). This way, we would still have pretty user-facing API like QuerySet[MyModel] and all the features of ValuesQuerySet.

Later, we can use TypeArgTuple to represent this natively using variadic generics.

@syastrov
Copy link
Contributor

That would be nice. Would you check the behavior in other type-checkers if you haven't provided all the type parameters?
It would be pretty nasty if you always got an error there.

I'm not sure I see how using variadic generics will help? It seems like we have some fixed number of type parameters, but we should allow some defaults for them if not provided (except that is not currently possible).

@sobolevn sobolevn mentioned this issue Jun 25, 2021
Closed
@syastrov
Copy link
Contributor

syastrov commented Jun 27, 2021 •
edited
Loading

I am afraid that making 2 generic parameters to QuerySet and hacking it in the mypy plugin to default the second parameter to the first one will make it even less likely that django-stubs will work in other type-checkers than mypy.
(I actually implemented this before mkurnikov changed QuerySet back to take one type param :)).

Are there any other options? I'll try to list some:

  1. Start up a discussion / PEP about making default type parameters on typing-sig.
    Pros: Fixing the root of the problem and working in all type checkers. We can remove ValuesQuerySet
    Cons: Likely to take a lot of time and effort, and will not necessarily be accepted

  2. Add second parameter to QuerySet, and don't implement this hackery about defaulting the second type parameter in the plugin.
    Pros: It's explicit/standard and works across type checkers. Doesn't require hacks. We can remove ValuesQuerySet
    Cons: A lot of people would find it annoying to change their code, but it's a one-time fix, assuming we settle on this. To prevent people from writing QuerySet[MyModel, MyModel] all over the place, we can provide an alias in django_stubs_ext. e.g. ModelQuerySet = QuerySet[_T, _T]. Then it's just ModelQuerySet[MyModel].

  3. Rename it to _ValuesQuerySet, since it is a private implementation detail, and leave it as is.
    Pros: Simple change, and hopefully won't break anyone's code.
    Cons: We now don't have a way to type-annotate the querysets returning e.g. tuples/dicts in user-code, _ValuesQuerySet is private/doesn't exist at runtime. We could provide a public alias to this though in django_stubs_ext, e.g. ValuesQuerySet = _ValuesQuerySet[_T, _Row].

I think I vote for (3). What do you think @sobolevn?

There are a lot of places, where we use QuerySet annotation, so ValuesQuerySet is not allowed there.

It's a subclass, so I don't know why it wouldn't be allowed. Do you have any examples?

Edit: Ah, I see, we have to change the current hierarchy by having ValuesQuerySet inherit directly from QuerySet and removing _BaseQuerySet.

T type var has models.Model bound, which stops us from annotating thing with, for example, QuerySet[SomeTypedDict], which is valid when we use .values and SomeTypedDict represents the final result of this call

I think we should use 2 generic parameters to allow this, so if you didn't care which model it was about, you could use the annotation: QuerySet[Any, SomeTypedDict]

@syastrov
Copy link
Contributor

I've given idea (3) a shot: #654
I had to add a type-ignore on the _ValuesQuerySet class declaration or I got an "Cannot create a consistent method resolution order (MRO)" error (I believe due to inheriting from Container).

@sobolevn
Copy link
Member Author

Yes, I agree that this seems like the best solution.

@sobolevn
Copy link
Member Author

Start up a discussion / PEP about making default type parameters on typing-sig.

I have already seen this discussed, but I cannot find the link.

@MichaelAquilina MichaelAquilina mentioned this issue Jul 3, 2021
Merged
syastrov added a commit to syastrov/django-stubs that referenced this issue Jul 7, 2021
@syastrov
Make QuerySet[_T] an external alias to _QuerySet[_T, _T].
7108a0c 
This currently has the drawback that error messages display the internal type _QuerySet, with both type arguments.

See also discussion on typeddjango#661 and typeddjango#608.

Fixes typeddjango#635: QuerySet methods on Managers (like .all()) now return QuerySets rather than Managers.

Address code review by @sobolevn.
sobolevn pushed a commit that referenced this issue Jul 23, 2021
@syastrov
QuerySet.annotate improvements (#398)
cfd69c0 
* QuerySet.annotate returns self-type. Attribute access falls back to Any.

- QuerySets that have an annotated model do not report errors during .filter() when called with invalid fields.
- QuerySets that have an annotated model return ordinary dict rather than TypedDict for .values()
- QuerySets that have an annotated model return Any rather than typed Tuple for .values_list()

* Fix .annotate so it reuses existing annotated types. Fixes error in typechecking Django testsuite.

* Fix self-typecheck error

* Fix flake8

* Fix case of .values/.values_list before .annotate.

* Extra ignores for Django 2.2 tests (false positives due to tests assuming QuerySet.first() won't return None)

Fix mypy self-check.

* More tests + more precise typing in case annotate called before values_list.

Cleanup tests.

* Test and fix annotate in combination with values/values_list with no params.

* Remove line that does nothing :)

* Formatting fixes

* Address code review

* Fix quoting in tests after mypy changed things

* Use Final

* Use typing_extensions.Final

* Fixes after ValuesQuerySet -> _ValuesQuerySet refactor. Still not passing tests yet.

* Fix inheritance of _ValuesQuerySet and remove unneeded type ignores.

This allows the test
"annotate_values_or_values_list_before_or_after_annotate_broadens_type"
to pass.

* Make it possible to annotate user code with "annotated models", using PEP 583 Annotated type.

* Add docs

* Make QuerySet[_T] an external alias to _QuerySet[_T, _T].

This currently has the drawback that error messages display the internal type _QuerySet, with both type arguments.

See also discussion on #661 and #608.

Fixes #635: QuerySet methods on Managers (like .all()) now return QuerySets rather than Managers.

Address code review by @sobolevn.

* Support passing TypedDicts to WithAnnotations

* Add an example of an error to README regarding WithAnnotations + TypedDict.

* Fix runtime behavior of ValuesQuerySet alias (you can't extend Any, for example).

Fix some edge case with from_queryset after QuerySet changed to be an
alias to _QuerySet. Can't make a minimal test case as this only occurred
on a large internal codebase.

* Fix issue when using from_queryset in some cases when having an argument with a type annotation on the QuerySet.

The mypy docstring on anal_type says not to call defer() after it.
@sobolevn
Copy link
Member Author

sobolevn commented Sep 4, 2021

We can consider this done! Thanks to @syastrov 🎉

@sobolevn sobolevn closed this as completed Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sobolevn @syastrov