PYTHON-3454 Specifying a generic type for a collection does not correctly enforce type safety when inserting data #1081

juliusgeo · 2022-10-18T21:18:32Z

No description provided.

juliusgeo · 2022-10-20T18:11:10Z

(python3.8) ➜  mongo-python-driver git:(PYTHON-3454) mypy --install-types --non-interactive test/test_mypy.py
Success: no issues found in 1 source file

Verified to work in Python 3.8+.

blink1073 · 2022-10-20T19:46:50Z

doc/examples/type_hints.rst


-You can use :py:class:`~typing.TypedDict` (Python 3.8+) when using a well-defined schema for the data in a :class:`~pymongo.collection.Collection`:
+You can use :py:class:`~typing_extensions.TypedDict` (Python 3.8+) when using a well-defined schema for the data in a :class:`~pymongo.collection.Collection`.
+Note that all `schema_validation`_ for inserts and updates is done on the server. This is due to the fact that these methods automatically add


I think you need to remove the underscore to match the link below "schema validation"

Also wrap lines at 80 or 100 chars.

doc/examples/type_hints.rst

blink1073 · 2022-10-20T19:51:00Z

pymongo/collection.py

        )

    @_csot.apply
    def insert_many(


We should add a mypy test for insert_many too.

And bulkWrite InsertOne if possible?

ShaneHarvey · 2022-10-20T19:13:56Z

test/utils.py

    that are configured on the client.
    """
-    hello = client.admin.command(HelloCompat.LEGACY_CMD)
+    hello: Any = client.admin.command(HelloCompat.LEGACY_CMD)


Can we use dict instead?

ShaneHarvey · 2022-10-20T20:17:22Z

doc/examples/type_hints.rst


-You can use :py:class:`~typing.TypedDict` (Python 3.8+) when using a well-defined schema for the data in a :class:`~pymongo.collection.Collection`:
+You can use :py:class:`~typing_extensions.TypedDict` (Python 3.8+) when using a well-defined schema for the data in a :class:`~pymongo.collection.Collection`.
+Note that all `schema_validation`_ for inserts and updates is done on the server. This is due to the fact that these methods automatically add


Also wrap lines at 80 or 100 chars.

ShaneHarvey · 2022-10-20T20:21:33Z

test/test_mypy.py


 try:
-    from typing import TypedDict  # type: ignore[attr-defined]
+    from typing_extensions import NotRequired, TypedDict


Doesn't this mean these tests will be skipped even when TypedDict exists because typing_extensions is not installed in CI?

I added it to CI.

ShaneHarvey · 2022-10-20T20:22:09Z

pymongo/collection.py

        )

    @_csot.apply
    def insert_many(


And bulkWrite InsertOne if possible?

ShaneHarvey · 2022-10-20T20:22:42Z

test/test_mypy.py

        assert retreived["year"] == 1
        assert retreived["name"] == "a"

+    # TODO: mypy --install-types --non-interactive test/test_mypy.py


Remove the todo?

ShaneHarvey · 2022-10-20T20:23:58Z

test/test_mypy.py

+        assert type(out["_id"]) == ObjectId
+
+    @only_type_check
+    def test_typeddict_document_type_empty(self) -> None:


Instead of empty what about MovieWithoutId?

ShaneHarvey · 2022-10-20T20:27:53Z

doc/examples/type_hints.rst

-You can use :py:class:`~typing.TypedDict` (Python 3.8+) when using a well-defined schema for the data in a :class:`~pymongo.collection.Collection`:
+You can use :py:class:`~typing_extensions.TypedDict` (Python 3.8+) when using a well-defined schema for the data in a :class:`~pymongo.collection.Collection`.
+Note that all `schema_validation`_ for inserts and updates is done on the server. This is due to the fact that these methods automatically add
+an "_id" field. In the example below the "_id" field is marked by the :py:class:`~typing_extensions.NotRequired` notation to allow it to be accessed when reading


This is due to the fact that these methods automatically add an "_id" field.

Schema validation has nothing to do with drivers adding the _id field.

ShaneHarvey · 2022-10-21T22:38:50Z

test/test_mypy.py

+            (coll.insert_one, mov),
+            (coll.bulk_write, [InsertOne(mov)]),
+        ]:
+            meth(arg)  # type:ignore[operator]


How can we put a type ignore here? insert_many/insert_one/bulk_write working without raising a mypy error is the main purpose of this bug fix. I think we're loosing sight of what's important here because the PR is muddled by tests for find_one/_id/NotRequired. Can you open a new ticket for the NotRequired stuff? To fix the bug in PYTHON-3454 I believe we only need to mention schema validation should be configured on the server and add a test like this to confirm the bug is fixed:

def test_typeddict_document_type_insertion(self) -> None: coll: Collection[Movie] = self.db.test mov = Movie(name="THX-1138", year=1971) coll.insert_one(mov) coll.insert_many([mov]) coll.bulk_write([InsertOne(mov)])

I'm finding it hard to separate out the NotRequired stuff in a way that makes sense (in the documentation). I have modified the tests to be similar to what you have, but with the NotRequired stuff as well.

Let's remove everything about _id, NotRequired, typing_extensions, and all the other tests besides the one above. All that is extra stuff that's only tangentially related to the issue. We already have tests that find_one correctly yields the collection generic type: https://github.com/mongodb/mongo-python-driver/blob/4.3.2/test/test_mypy.py#L299-L305

I'm confused how it is tangentially related to the issue when the actual issue at hand is not just whether it accepts a TypedDict, but whether it is also being type-checked, no? The title specifically mentions inserting data, but also mentions enforcing type safety, which is why I included that in this PR.

Also that test does check that the generic type is yielded, it does not also test that the correct MyPy errors occur during the various different ways you can specify _id.

Oh, that is strange. Wouldn't that be an argument in favor of clarifying the _id field in this PR? The fact that the user will have to explicitly specify the _id if they want it to be type-checked. I'm not sure how this would not be very confusing without a full explanation of what is going on.

In the original example the TypeDict did not have an explicit _id field.

Yes, but I view that as a deficiency in the original example. The _id field will always be inserted, and there are three ways that the user can use that with type-checking:

Movie - would require changing our examples to pass in an explicit _id field.

MovieWithoutId - the _id field is just ignored and you cannot type check the _id field (what our previous example was doing).

ImplicitMovie - would not require passing in an _id, but would allow type-checking.

I don't think that the user just wanted the error to go away, but also wanted it to be type-checked. This would leave us with Movie and ImplicitMovie, one of which would require the user to always pass in an _id, and the other would have the expected behavior but requires NotRequired. Both of these options require an explanation to the user, though, because it depends on how they are using the TypedDict.

Not including the _id is still a valid choice, if it is not relevant to your application.

It is a valid choice, but it has caveats to it and I'm afraid users will be confused if they expect to be able to type check the _id field just as any other field

blink1073

LGTM!

blink1073 · 2022-10-28T19:49:01Z

test/test_mypy.py

+        coll.insert_one(bad_movie)
+        coll.insert_many([bad_mov])  # type:ignore[list-item]
+        coll.insert_many(
+            [{"name": "THX-1138", "year": "WRONG TYPE"}]


The auto-formatter got us here, you need to move the ignore comment

blink1073 · 2022-10-28T19:55:12Z

There's also some unused imports causing the lint to fail.

ShaneHarvey · 2022-10-28T20:38:41Z

pymongo/operations.py

    __slots__ = ("_doc",)

-    def __init__(self, document: _DocumentIn) -> None:
+    def __init__(self, document: Union[_DocumentIn, _DocumentType]) -> None:


Can we defer this change to PYTHON-3493?

ShaneHarvey · 2022-10-28T20:44:47Z

doc/examples/type_hints.rst

 .. doctest::

-  >>> from typing import TypedDict
+  >>> from typing_extensions import TypedDict


Let's remove typing_extensions.

ShaneHarvey · 2022-10-28T20:44:56Z

.github/workflows/test-python.yml

      - name: Install dependencies
        run: |
-          python -m pip install -U pip mypy
+          python -m pip install -U pip mypy typing_extensions


Let's remove typing_extensions.

ShaneHarvey · 2022-10-28T20:54:22Z

test/test_mypy.py

-
    # Not available in Python 3.7
+    from typing_extensions import TypedDict
+


Let's revert these changes and go back to using typing.

We will need to keep using typing_extensions in this file, or else split off a different file to run mypy on Python 3.7. The reason it was working before is that we were inadvertently ignoring errors when running this file.

The reason it was working before is that we were inadvertently ignoring errors when running this file.

Can you explain? How was this fixed?

We added a new invocation in GitHub Actions in this PR to run this file without disabling several error checks.

I see thanks.

ShaneHarvey · 2022-10-31T17:46:25Z

test/test_mypy.py

 import unittest
 from typing import TYPE_CHECKING, Any, Dict, Iterable, Iterator, List

-try:


We need to keep this in a try/except so that python test/test_mypy.py (or python3.7 setup.py test -s test.test_mypy) works even when typing_extensions is not installed. It's fine for the typing checking job to depend on typing_extensions, but not the test suite itself.

juliusgeo added 7 commits October 11, 2022 12:03

initial commit

92188dd

Merge remote-tracking branch 'upstream/master' into PYTHON-3454

ab64c72

add documentation

ea41432

add mypy test

6e21760

fix test

2a86839

fix the tests

d4ef6b8

fix documentation, etc

3a063b8

juliusgeo marked this pull request as ready for review October 20, 2022 18:11

juliusgeo requested review from ShaneHarvey and blink1073 as code owners October 20, 2022 18:11

blink1073 requested changes Oct 20, 2022

View reviewed changes

ShaneHarvey reviewed Oct 20, 2022

View reviewed changes

fix tests so they test all the methods

8322387

juliusgeo requested review from ShaneHarvey and blink1073 October 21, 2022 22:22

ShaneHarvey requested changes Oct 21, 2022

View reviewed changes

juliusgeo added 4 commits October 25, 2022 10:09

simplify tests

f9ba0c7

not sure why this is not working

1bb46a5

update tests

8139724

Merge remote-tracking branch 'upstream/master' into PYTHON-3454

4c2c265

juliusgeo force-pushed the PYTHON-3454 branch from 7331570 to 4c2c265 Compare October 28, 2022 19:23

clean up docs

91f3666

blink1073 approved these changes Oct 28, 2022

View reviewed changes

blink1073 reviewed Oct 28, 2022

View reviewed changes

fix autoformatter 'fix'

6cfe733

juliusgeo requested a review from blink1073 October 28, 2022 19:50

remove unused imports

671eea7

blink1073 approved these changes Oct 28, 2022

View reviewed changes

one final unused import

3ab850e

ShaneHarvey requested changes Oct 28, 2022

View reviewed changes

juliusgeo added 4 commits October 28, 2022 13:57

remove typing_extensions, defer bulk write change

7c1dab2

remove typing_extensions, defer bulk write change

3869d8d

one final reference to typing_extensions

327fe90

my ignores are wrong?

2d58a21

juliusgeo requested a review from ShaneHarvey October 28, 2022 21:32

add back typing_extensions

75e0dc5

ShaneHarvey requested changes Oct 31, 2022

View reviewed changes

add back in try except

e0d1d98

juliusgeo requested a review from ShaneHarvey October 31, 2022 17:49

ShaneHarvey approved these changes Oct 31, 2022

View reviewed changes

juliusgeo merged commit 9083821 into mongodb:master Nov 1, 2022


		# Not available in Python 3.7
		from typing_extensions import TypedDict

PYTHON-3454 Specifying a generic type for a collection does not correctly enforce type safety when inserting data #1081

PYTHON-3454 Specifying a generic type for a collection does not correctly enforce type safety when inserting data #1081

Uh oh!

Conversation

juliusgeo commented Oct 18, 2022

Uh oh!

juliusgeo commented Oct 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusgeo Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusgeo Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blink1073 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blink1073 commented Oct 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusgeo Oct 25, 2022 •

edited

Loading

juliusgeo Oct 27, 2022 •

edited

Loading

ShaneHarvey Oct 31, 2022 •

edited

Loading