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

PYTHON-3493 Bulk Write InsertOne Should Be Parameter Of Collection Type #1106

Merged
merged 31 commits into from
Nov 10, 2022

Conversation

juliusgeo
Copy link
Contributor

No description provided.

pymongo/operations.py Outdated Show resolved Hide resolved
pymongo/operations.py Outdated Show resolved Hide resolved
pymongo/operations.py Outdated Show resolved Hide resolved
pymongo/operations.py Outdated Show resolved Hide resolved
@blink1073
Copy link
Member

  File "/home/runner/work/mongo-python-driver/mongo-python-driver/pymongo/operations.py", line 22, in <module>
    from pymongo.typings import RawBSONDocument, _CollationIn, _DocumentType, _Pipeline
ImportError: cannot import name 'RawBSONDocument' from 'pymongo.typings' (/home/runner/work/mongo-python-driver/mongo-python-driver/pymongo/typings.py)

That import is only during type checking, you'll have to import from the raw_bson module directly.

pymongo/collection.py Outdated Show resolved Hide resolved
@blink1073
Copy link
Member

It looks like typing_extensions is still not installed in EG:

[2022/11/03 13:33:05.363] ERROR: name 'Movie' is not defined (NameError)
 [2022/11/03 13:33:05.363] Traceback (most recent call last):
[2022/11/03 13:33:05.363]   File "/data/mci/ad80d9537e77ee1c525bef8a857fbff0/src/test/test_mypy.py", line 121, in test_bulk_write
[2022/11/03 13:33:05.363]     requests: List[InsertOne[Movie]] = [InsertOne(Movie(name="American Graffiti", year=1973))]
[2022/11/03 13:33:05.363] NameError: name 'Movie' is not defined

test/test_session.py Outdated Show resolved Hide resolved
test/test_server_selection.py Outdated Show resolved Hide resolved
@juliusgeo juliusgeo marked this pull request as ready for review November 7, 2022 17:01
pymongo/typings.py Show resolved Hide resolved
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

>>> assert result["_id"]

This same typing scheme works for all of the insert methods (`insert_one`, `insert_many`, and `bulk_write`). For `bulk_write`,
both `InsertOne/Many` and `ReplaceOne/Many` operators are generic.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no such thing as InsertMany or ReplaceMany.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can you make these rst links to the methods and classes (:meth:...).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliusgeo ReplaceMany should be removed, there is no ReplaceMany operator


This same typing scheme works for all of the insert methods (:meth:`~pymongo.collection.Collection.insert_one`,
:meth:`~pymongo.collection.Collection.insert_many`, and :meth:`~pymongo.collection.Collection.bulk_write`).
For `bulk_write`, :py:class:`~pymongo.operations.InsertOne`, :py:class:`~pymongo.operations.ReplaceOne`, and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:py:class: -> :class:

This same typing scheme works for all of the insert methods (:meth:`~pymongo.collection.Collection.insert_one`,
:meth:`~pymongo.collection.Collection.insert_many`, and :meth:`~pymongo.collection.Collection.bulk_write`).
For `bulk_write`, :class:`~pymongo.operations.InsertOne`, :class:`~pymongo.operations.ReplaceOne`, and
:class:`~pymongo.operations.UpdateMany` operators are generic.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateMany is not generic.

Copy link
Contributor Author

@juliusgeo juliusgeo Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is made generic in this PR. Line 378 of operations.py. The name of the class is elided from the GitHub diff view. That would mean I need to add tests, though. I will do that.

@@ -374,7 +375,7 @@ class UpdateMany(_UpdateOp):
def __init__(
self,
filter: Mapping[str, Any],
update: Union[Mapping[str, Any], _Pipeline],
update: Union[_DocumentType, RawBSONDocument, _Pipeline],
Copy link
Member

@ShaneHarvey ShaneHarvey Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, the update doc for UpdateOne and UdateMany shouldn't accept the _DocumentType. The update is a mapping like this {"$inc": ...}, not a full doucment.

Copy link
Contributor Author

@juliusgeo juliusgeo Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Those were added in without tests, hence why that wasn't noticed. I will remove those changes.

@@ -201,7 +201,7 @@ def test_list_collection_names_filter(self):
db.capped.insert_one({})
db.non_capped.insert_one({})
self.addCleanup(client.drop_database, db.name)

filter: dict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a type hint for mypy so that this error does not occur:

(python3.11) ➜  mongo-python-driver git:(PYTHON-3493)           mypy --install-types --non-interactive --disable-error-code var-annotated --disable-error-code attr-defined --disable-error-code union-attr --disable-error-code assignment --disable-error-code no-redef --disable-error-code index --allow-redefinition --allow-untyped-globals --exclude "test/mypy_fails/*.*" test

test/test_database.py: note: In member "test_list_collection_names_filter" of class "TestDatabase":
test/test_database.py:208: error: Argument "filter" to "list_collection_names" of "Database" has incompatible type "object"; expected "Optional[Mapping[str, Any]]"  [arg-type]
                names = db.list_collection_names(filter=filter)
                                                        ^~~~~~
Found 1 error in 1 file (checked 121 source files)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. I thought it was a copy/past error. Down below there's already filter: Any. Can we define this once? perhaps filter: Union[None, dict] would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that works.

@@ -347,7 +347,7 @@ def test_bulk_write_no_results(self):

def test_bulk_write_invalid_arguments(self):
# The requests argument must be a list.
generator = (InsertOne({}) for _ in range(10))
generator = (InsertOne[dict]({}) for _ in range(10))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these changes required? This seems annoyingly verbose for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required because otherwise mypy complains:

(python3.11) ➜  mongo-python-driver git:(PYTHON-3493) ✗           mypy --install-types --non-interactive --disable-error-code var-annotated --disable-error-code attr-defined --disable-error-code union-attr --disable-error-code assignment --disable-error-code no-redef --disable-error-code index --allow-redefinition --allow-untyped-globals --exclude "test/mypy_fails/*.*" test

test/test_bulk.py: note: In member "test_numerous_inserts" of class "TestBulk":
test/test_bulk.py:299: error: Argument 1 to "InsertOne" has incompatible type "Dict[<nothing>, <nothing>]"; expected "RawBSONDocument"  [arg-type]
            requests = [InsertOne({}) for _ in range(n_docs)]
                                  ^~
Found 1 error in 1 file (checked 121 source files)

I'm not sure why it doesn't resolve to the bound of the TypeVar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reason we need a type annotation on MongoClient

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, it's just empty/untyped dictionary. A non-empty dict works without a manual type annotation:

InsertOne({}) #  Argument 1 to "InsertOne" has incompatible type "Dict[<nothing>, <nothing>]"; expected "RawBSONDocument"
InsertOne({"a": 1}). # This works

It's a shame the error message is so opaque.

For `bulk_write` both :class:`~pymongo.operations.InsertOne` and :class:`~pymongo.operations.ReplaceOne` operators are generic.

.. doctest::

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs the python version guard

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of, please schedule the docs and doctest evergreen tasks when changing things like this.

@juliusgeo juliusgeo merged commit 92e6150 into mongodb:master Nov 10, 2022
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