-
Notifications
You must be signed in to change notification settings - Fork 41
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
P1: CRUD usage examples moved to appropriate pages #618
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-java ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more suggestions, they're all pretty minor but lmk if you want me to take another look
@katcharov This is ready for tech review. The .java files have been relocated and a few of them merged together. They have all been run locally. |
This ready for tech review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review (Retrieve section)
To retrieve a single document, you can append the ``first()`` method to your | ||
``find()`` operation. You can use the ``sort()`` operation before selecting the first | ||
document to help choose the correct file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this discuss any other methods, apart from first()?
To retrieve a single document, you can use the first() method with a find() method. To select a particular document based on ordering, use the sort() method prior to applying first().
- "append" is odd.
- Let's avoid the term "correct", except to identify actual correctness issues. This suggests first does not behave correctly unless sort is supplied, but it is often fine to select an arbitrary first item.
- I think "file" is intended to be a synonym for "document" but this should be avoided since we do not deal with files, and we should refer to documents as such.
- Somewhat technical, but the sort is part of the find operation.
find()
is a method.
If we are counting on using first
, and it is worth suggesting sort, it is probably worth suggesting limit due to this.
There are also a few unusual or imprecise phrasings in the paragraphs above:
Use the find operation to retrieve a subset of your existing data in MongoDB. You can specify what data to return including which documents to retrieve, in what order to retrieve them, and how many to retrieve.
- "what data to return including which documents to retrieve" suggests that there might be something else besides documents
- "subset" and "existing" seem unnecessary: "Use the find operation to retrieve data from MongoDB"?
To perform a find operation, call the
find()
method on an instance of aMongoCollection
.
- Somewhat technical, but the operation is not performed (sent to the server and executed there) until a method like "first" (or iterator, etc.) is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made some changes here. The only thing I wasn't sure what to do about is your last point:
To perform a find operation...
If someone executes the following code:
collection.find(filter)
Does the server execute a find operation? I understand there a distinction between a method and an operation, but the method is the vehicle by which a user causes a find operation to be performed, correct? Like are we just talking about semantics, or is this something that the reader would find confusing about how this written?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate you checking to make sure this is clear.
collection.find(filter)
... Does the server execute a find operation?
No, this actually happens only after a method like first
(or: executeExplain
, iterator
, cursor
, forEach
, ...) is invoked. The preceding methods, like sort, build something like an BSON document representing the operation. Those final methods eventually send BSON to the server, where the operation is executed. Calling the find
method on an instance of a collection will not initiate a search on the server. (Not that we need to document all of this, but the docs should not imply otherwise.)
(I do realize that some of this goes beyond what was added in this PR, so it is fine if it is addressed in another ticket.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for the clarification. I've made some changes.
source/includes/crud/Find.java
Outdated
@@ -36,12 +38,20 @@ public static void main( String[] args ) { | |||
.sort(Sorts.descending("title")).iterator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments could be more specific, // Projects "title" and "imdb" fields, excludes "_id".
, or Retrieves documents that have a runtime length of under 15 minutes, sorted in reverse-alphabetical order
etc. This should probably also apply to "System.out" outputs.
It seems unusual to sort in reverse-alphabetical order - perhaps this is confusing, and suggests descending is alphabetical?
source/includes/crud/Find.java
Outdated
} finally { | ||
cursor.close(); | ||
System.out.println("Number of documents found with find(): " + cursor.available() + "\n"); | ||
cursor.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using try-with-resources.
source/includes/crud/Find.java
Outdated
} | ||
} finally { | ||
cursor.close(); | ||
System.out.println("Number of documents found with find(): " + cursor.available() + "\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not make sense to sort if all we need is the total number of documents. If we really need the number, we should use a $count stage.
It seems that we should demonstrate cursor iteration - this is much more common and important than available or first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked this eg
source/includes/crud/Find.java
Outdated
// Retrieves the first matching document, applying a projection and a descending sort to the results | ||
Document doc = collection.find(eq("title", "The Room")) | ||
.projection(projectionFields) | ||
.sort(Sorts.descending("imdb.rating")) | ||
.first(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unusual to find the best-rated movie with a particular name (or other identifier) - usually we would return all movies, or include some other criteria such as year.
A more realistic query might be to find the best-rated ("first") movie under 15 minutes.
source/includes/crud/Insert.java
Outdated
// Prints a message if any exceptions occur during the operation | ||
} catch (MongoException me) { | ||
System.err.println("Unable to insert due to an error: " + me); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment might be too obvious or literal (by literal, a severe case would be saying "catches a MongoException" or "calls println"). If it really is needed, I would put it after the catch above the println, since it's the println that prints.
source/includes/crud/Find.java
Outdated
System.out.print("10 movies under 15 minutes: "); | ||
docs.forEach(doc -> System.out.print(doc.get("title") + ", ")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.out.print("10 movies under 15 minutes: "); | |
docs.forEach(doc -> System.out.print(doc.get("title") + ", ")); | |
System.out.println("10 movies under 15 minutes: "); | |
docs.forEach(doc -> System.out.println("- " + doc.get("title"))); | |
System.out.println(); |
Avoids trailing comma issue, and the lack of trailing newline, and moves the newline "spacer" to the end of the list.
source/includes/crud/Find.java
Outdated
.projection(projectionFields) | ||
.sort(Sorts.descending("title")).iterator(); | ||
.sort(Sorts.ascending("title")) | ||
.limit(10); | ||
|
||
// Prints the results of the find operation as JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Prints the results of the find operation as JSON |
This is no longer json.
source/includes/crud/Find.java
Outdated
if (doc == null) { | ||
System.out.println("No results found."); | ||
} else { | ||
System.out.println("Document found with find().first(): " + doc.toJson()); | ||
System.out.println("\n\nThe highest rated movie under 15 minutes: " + doc.toJson()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.out.println("\n\nThe highest rated movie under 15 minutes: " + doc.toJson()); | |
System.out.println("The highest rated movie under 15 minutes: " + doc.toJson().get("title")); |
For consistency with changes to preceding example (no longer prints json).
source/includes/crud/Find.java
Outdated
.first(); | ||
|
||
// Prints a message if there are no result documents, or prints the result document as JSON | ||
// Prints result document as JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Prints result document as JSON |
- Calls the ``find()`` method to retrieve multiple documents that have a ``runtime`` value less than ``15``, applying a projection and sort to the results | ||
- Calls the ``find()`` and ``first()`` methods to retrieve a document that has a ``title`` value of ``"The Room"``, applying a projection and sort before returning the first match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer accurate
source/includes/crud/Delete.java
Outdated
System.out.println("deleteOne() document count: " + result.getDeletedCount()); | ||
|
||
// Prints a message if any exceptions occur during the operation | ||
} catch (MongoException me) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This comment is optional/informational.)
Note that even the find operation can fail with an exception (it does not have these try-catch blocks).
If you feel these operation-level try-catch blocks clutter things up now that the files are merged, it is fine to remove them (also fine to keep). They should of course be kept if there is some reason, for example in this section, there is this suggestion to use try-catch blocks for a particular purpose:
Use a try-catch block to get an acknowledgment for successfully processed documents before the error occurs:
As an aside, that message phrasing could be improved:
Use a try-catch block to learn which documents were successfully processed before the error occured:
source/includes/crud/Delete.java
Outdated
|
||
// Prints the number of deleted documents | ||
System.out.println("Deleted document count: " + result.getDeletedCount()); | ||
System.out.println("deleteMany() document count: " + result.getDeletedCount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not obvious what a "deleteMany document count" is.
Usually the target audience of output messages (vs log messages, or system outs intended for debugging purposes) is the end-user running the program, so they do not make reference to specific methods or other implementation details.
Compare with the output messages from the Insert page:
InsertOneResult result = collection.insertOne(doc1);
System.out.println("Inserted a document with the following id: " + ...);
...
System.out.println("Inserted documents with the following ids: " +
Here, the plural disambiguates which method was used.
In any case, it seems the style should be consistent across the pages and examples.
source/includes/crud/Update.java
Outdated
UpdateOptions options = new UpdateOptions().upsert(true); | ||
|
||
|
||
Document updateOneQuery = new Document().append("title", "Cool Runnings 2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UpdateOptions options = new UpdateOptions().upsert(true); | |
Document updateOneQuery = new Document().append("title", "Cool Runnings 2"); | |
UpdateOptions options = new UpdateOptions().upsert(true); | |
Document updateOneQuery = new Document().append("title", "Cool Runnings 2"); |
just excess whitespace
source/includes/crud/Delete.java
Outdated
@@ -1,8 +1,8 @@ | |||
// Deletes multiple documents from a collection by using the Java driver | |||
// Deletes a document from a collection by using the Java driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file now includes a deleteMany, so the old incorrect comment is now correct. Might be worth doublechecking these comments across files.
827b3b3
to
afb49e7
Compare
Pull Request Info
PR Reviewing Guidelines
JIRA - https://jira.mongodb.org/browse/DOCSP-47267
Staging Links
Self-Review Checklist