Skip to content

DOCSP-46321: CRUD operations #144

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

Closed

Conversation

norareidy
Copy link
Collaborator

@norareidy norareidy commented Jan 10, 2025

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-46321
Staging - https://deploy-preview-144--docs-pymongo.netlify.app/interact-data/crud/

Note: this PR will move to the docs-django repo.

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link

netlify bot commented Jan 10, 2025

Deploy Preview for docs-pymongo ready!

Name Link
🔨 Latest commit 60cc381
🔍 Latest deploy log https://app.netlify.com/sites/docs-pymongo/deploys/679132f65201d4000880d89b
😎 Deploy Preview https://deploy-preview-144--docs-pymongo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

genres = ArrayField(models.CharField(max_length=100), blank=True)
objects = MongoManager()

class Meta:

Choose a reason for hiding this comment

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

@timgraham Do we need managed=False here too?

Choose a reason for hiding this comment

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

Well, yes, there is a lot of feedback from #132 that's not yet incorporated here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated


.. code-block:: bash

python3 manage.py shell

Choose a reason for hiding this comment

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

@norareidy Can you confirm you want to keep python3 (vs. python) for MongoDB-wide compliance reasons? Just want to make sure we decide definitively somewhere in writing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

python is fine, I updated the Getting Started guide as well

snooty.toml Outdated
@@ -25,6 +25,7 @@ sharedinclude_root = "https://raw.githubusercontent.com/10gen/docs-shared/main/"
driver-short = "PyMongo"
driver-long = "PyMongo, the MongoDB synchronous Python driver,"
driver-async = "PyMongo Async"
django-odm = "MongoDB Backend for Django"

Choose a reason for hiding this comment

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

@norareidy Also here can you confirm we want to use "MongoDB Backend for Django" instead of "Django MongoDB Backend" ? Thank you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to "Django MongoDB Backend" in all PRs

Copy link

@aclark4life aclark4life left a comment

Choose a reason for hiding this comment

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

LGTM pending minor issues resolution, which may or may not require additional edits.

@norareidy norareidy requested a review from timgraham January 14, 2025 20:43
.. tip::

To learn more about Django's ``QuerySet`` API, see
`QuerySet API reference <https://docs.djangoproject.com/en/5.1/ref/models/querysets/>`__

Choose a reason for hiding this comment

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

How about using intersphinx for linking to Django docs? The objects file: http://docs.djangoproject.com/en/5.0/_objects/ I think this would be especially helpful for making sure links stay updated when switching from one Django version to the next.

Choose a reason for hiding this comment

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

Looks like there is some precedent for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking into this!

Choose a reason for hiding this comment

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

@norareidy were we able to get resolve the issue with sphinx?

released = models.DateTimeField("release date", null=True, blank=True)
awards = EmbeddedModelField(Award)
genres = ArrayField(models.CharField(max_length=100), null=True, blank=True)
objects = MongoManager()

Choose a reason for hiding this comment

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

no need for this

Choose a reason for hiding this comment

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

No need for MongoManager or all 4 lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think MongoManager

Example
~~~~~~~

The following example calls the ``create()`` method on your ``Movie`` objects

Choose a reason for hiding this comment

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

The phrasing "create() method on your Movie objects" is used several times. This isn't precisely correct and could give the impression of something like "Movie.create(). "Movie.objects" is a manager and "create" is a queryset method. Rather than get into all of that, you might just say something like, "The following example uses the create() method to insert a document..."

CRUD operations. To update documents in your collection, call the
``QuerySet`` operation methods on your model objects that represent the collection.
Then, {+django-odm+} runs the operations on your collection documents.
CRUD operations. To run these operations, call ``QuerySet`` methods

Choose a reason for hiding this comment

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

operations, ^you can call

``QuerySet`` operation methods on your model objects that represent the collection.
Then, {+django-odm+} runs the operations on your collection documents.
CRUD operations. To run these operations, call ``QuerySet`` methods
your model's ``Manager``. The ``Manager`` class handles database

Choose a reason for hiding this comment

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

methods ^on your

To insert a document into a collection, call the ``create()`` method
on an instance of your model's ``Manager`` class. Pass the new document's
fields and values as arguments to the ``create()`` method.

Choose a reason for hiding this comment

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

To insert a document into a collection, call the create() method on your model's Manager class. Pass the new document's field names and field values as arguments to the create() method.

and save the object as a collection document in one method call.
To view an example that creates an object then saves it to the
database by calling ``save()``, see `create() <https://docs.djangoproject.com/en/5.1/ref/models/querysets/#create>`__
in the Django documentation.

Choose a reason for hiding this comment

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

I don't know if you want to mention this shortcut here, but I think you can also do this:

movie = Movie(title="Poor Things", runtime=141)

Presumably this works because instantiation of the class calls the manager's create method or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like you still need to call save() after to actually insert it into the database


To retrieve documents from your collection, call the ``filter()`` method on an
instance of your model's ``Manager`` class. Pass a query filter, or criteria
that specifies which documents to retrieve, as an argument to the ``filter()`` method.

Choose a reason for hiding this comment

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

To retrieve documents from your collection, call the filter() method on your model's Manager class.


To learn how to create a Django application that uses the ``Movie``
model and the Python interactive shell to interact with MongoDB documents,
visit the :ref:`django-get-started` tutorial.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: link broken until #132 is merged

Copy link

@aclark4life aclark4life left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

a few small things!

Comment on lines 28 to 29
CRUD operations. To run these operations, you can call ``QuerySet`` methods
on your model's ``Manager``. The ``Manager`` class handles database
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CRUD operations. To run these operations, you can call ``QuerySet`` methods
on your model's ``Manager``. The ``Manager`` class handles database
CRUD operations. To run these operations, call ``QuerySet`` methods
on your model's ``Manager`` instance? object?. The ``Manager`` class handles database

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aclark4life you suggested I remove "instance" - is it more accurate to replace with "object" or just keep as is?

Copy link

Choose a reason for hiding this comment

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

I like Manager object.

Choose a reason for hiding this comment

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

Manager is a class with QuerySet methods mixed in: https://github.com/django/django/blob/stable/5.1.x/django/db/models/manager.py#L176.

The default Manager is called objects … and it's an attribute of the model class… but yeah, looking at Django's documentation a manager typically referred to as just "manager". It may help to use "manager" instead of Manager since we're referring to the model's manager and not the Manager class e.g.: https://docs.djangoproject.com/en/5.1/topics/db/queries/#retrieving-objects

Lastly, I find instance and object confusing for various reasons. I would say if you see Django using those terms go for it, else stick close to how Django describers managers as much as possible.

Choose a reason for hiding this comment

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

Correcting myself: this is how Django says it:

"To retrieve objects from your database, construct a QuerySet via a Manager on your model class."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay thank you! I will use "model's manager" as much as possible

CRUD operations. To run these operations, you can call ``QuerySet`` methods
on your model's ``Manager``. The ``Manager`` class handles database
operations and allows you to interact with your MongoDB data by referencing
Django models. By default, Django adds a ``Manager`` named ``objects``
Copy link
Contributor

Choose a reason for hiding this comment

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

S: consider making just "Django" a source constant as its a word with a high possibility of typos, IMO

Comment on lines 36 to 39
- :ref:`create() <django-crud-insert>`
- :ref:`filter() and get() <django-crud-read>`
- :ref:`update() <django-crud-modify>`
- :ref:`delete() <django-crud-delete>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- :ref:`create() <django-crud-insert>`
- :ref:`filter() and get() <django-crud-read>`
- :ref:`update() <django-crud-modify>`
- :ref:`delete() <django-crud-delete>`
- :ref:`create() <django-crud-insert>`: Insert new data
- :ref:`filter() and get() <django-crud-read>`: Retrieve specific documents... etc
- :ref:`update() <django-crud-modify>`
- :ref:`delete() <django-crud-delete>`


.. tip::

To learn more about Django's ``QuerySet`` API, see
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To learn more about Django's ``QuerySet`` API, see
To learn more about Django's ``QuerySet`` API, see the

Comment on lines 123 to 124
The ``create()`` method allows you to create a new ``Movie`` object
and save the object as a collection document in one method call.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The ``create()`` method allows you to create a new ``Movie`` object
and save the object as a collection document in one method call.
The ``create()`` method allows you to create a new ``Movie`` object
and save the object to MongoDB in one method call.

Comment on lines 125 to 127
To view an example that creates an object then saves it to the
database by calling ``save()``, see `create() <https://docs.djangoproject.com/en/5.1/ref/models/querysets/#create>`__
in the Django documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: why not include this example here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added


If your query matches no documents or multiple documents, the ``get()``
method generates an error. To retrieve one document from a query
that might match multiple, chain the ``first()`` method to ``filter()``.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: you can still link to the specify a query guide in the TODO, but adding a one line example here of chaining first() wouldnt be a bad idea either!

Comment on lines 219 to 220
Movie.objects.filter(title="High Fidelity").update(plot=
"Rob, a record store owner and compulsive list maker, recounts his top five breakups, including the one in progress.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: fix indentation?

@norareidy norareidy requested a review from rustagir January 17, 2025 15:53
Copy link

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

Another great read. Provided some additional feedback. Let me know what you think

Comment on lines +71 to +73
class Meta:
db_table = "movies"
managed = False
Copy link

Choose a reason for hiding this comment

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

Could you add a helpful tooltip or note to explain the purpose of these two meta configurations. A blurb along the lines of:
"""
In Django, defining a Meta class within a Model allows you to configure the Meta options for the model. In this example, we use db_table = "movies" to change the collection referenced in the database to look for movies, and we set managed = False to stop database creation, modification, or deletion during migration commands. To read more, check out Django's Meta Options documentation.
"""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since I use this sample data on almost every page, what do you think of linking to the Models guide (which explains Meta and str()) instead of repeating the explanation every time?

Copy link

Choose a reason for hiding this comment

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

Sounds good to me

Comment on lines +75 to +76
def __str__(self):
return self.title
Copy link

Choose a reason for hiding this comment

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

Same here. Another helpful tooltip:
"""
In Django, you can override the str method of a model to define how its instances are represented as strings.
"""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same comment as above

Copy link

Choose a reason for hiding this comment

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

Yep. Works for me

<MongoQuerySet [<Movie: The Bumblebee Flies Anyway>, <Movie: Angels of the Universe>,
<Movie: First Person Plural>, <Movie: Just, Melvin: Just Evil>, <Movie: Sound and Fury>,
<Movie: Peppermint Candy>]>

Copy link

Choose a reason for hiding this comment

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

I'm thinking we should add a tooltip to link to the lookup operations documentation in Django.

cc: @aclark4life @timgraham

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean add a link to the Django documentation for each method at the end of the sections? I can do that!

Comment on lines 28 to 29
CRUD operations. To run these operations, you can call ``QuerySet`` methods
on your model's ``Manager``. The ``Manager`` class handles database
Copy link

Choose a reason for hiding this comment

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

I like Manager object.

Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

lgtm!

@timgraham
Copy link

Are there instructions on building the docs locally so we can debug intersphinx? I got this far:

$ ./build.sh 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2039  100  2039    0     0   9396      0 --:--:-- --:--:-- --:--:--  9396
make: *** No rule to make target 'examples'.  Stop.
=======================================================================================================================================================================
========================================================================== Running parser... ==========================================================================
INFO:snooty.main:Snooty 0.18.11 starting
INFO:snooty.parser:cache: 0 hits and 101 misses
INFO:snooty.gizaparser.domain:Parsing steps YAML
INFO:snooty.gizaparser.domain:Parsing extracts YAML
INFO:snooty.gizaparser.domain:Parsing release YAML
WARNING(fundamentals/collations.txt:0ish): Page not included in any toctree and not marked :orphan:
WARNING(fundamentals/type-hints.txt:0ish): Page not included in any toctree and not marked :orphan:
2 diagnostics; 74 pages; 3 assets
========================================================================== Parser complete ============================================================================
=======================================================================================================================================================================
Error: Failed to replace env in config: ${NPM_BASE_64_AUTH}
    at /usr/local/lib/node_modules/npm/lib/config/core.js:415:13
    at String.replace (<anonymous>)
    at envReplace (/usr/local/lib/node_modules/npm/lib/config/core.js:411:12)
    at parseField (/usr/local/lib/node_modules/npm/lib/config/core.js:389:7)
    at /usr/local/lib/node_modules/npm/lib/config/core.js:330:24
    at Array.forEach (<anonymous>)
    at Conf.add (/usr/local/lib/node_modules/npm/lib/config/core.js:328:23)
    at ConfigChain.addString (/usr/local/lib/node_modules/npm/node_modules/config-chain/index.js:244:8)
    at Conf.<anonymous> (/usr/local/lib/node_modules/npm/lib/config/core.js:316:10)
    at /usr/local/lib/node_modules/npm/node_modules/graceful-fs/graceful-fs.js:90:16
/usr/local/lib/node_modules/npm/lib/npm.js:59
      throw new Error('npm.load() required')
      ^

Error: npm.load() required
    at Object.get (/usr/local/lib/node_modules/npm/lib/npm.js:59:13)
    at process.errorHandler (/usr/local/lib/node_modules/npm/lib/utils/error-handler.js:205:32)
    at emitOne (events.js:125:13)
    at process.emit (events.js:221:7)
    at process._fatalException (bootstrap_node.js:382:26)

Comment on lines +71 to +73
class Meta:
db_table = "movies"
managed = False
Copy link

Choose a reason for hiding this comment

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

Sounds good to me

Comment on lines +75 to +76
def __str__(self):
return self.title
Copy link

Choose a reason for hiding this comment

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

Yep. Works for me

@norareidy
Copy link
Collaborator Author

Moved to the Django repo: mongodb/docs-django#11

Copy link

@R-shubham R-shubham left a comment

Choose a reason for hiding this comment

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

LGTM

.. tip::

To learn more about Django's ``QuerySet`` API, see
`QuerySet API reference <https://docs.djangoproject.com/en/5.1/ref/models/querysets/>`__

Choose a reason for hiding this comment

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

@norareidy were we able to get resolve the issue with sphinx?

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.

6 participants