MongoDB Peer-Programming: Reviewing a Pull Request

How this round actually works

Here is the shape candidates tend to report:

• Timing: plan for roughly 60 minutes of review, after a 10 to 15 minute introduction (introductions, a little setup, and getting oriented). It is shorter than a build-a-feature round, so the pace and prioritisation of your feedback matter.
• The interviewer shares a small but real service, often in Java. MongoDB runs a lot of backend services and owns the official MongoDB Java driver, so a Java service over a MongoDB collection is a natural choice. You clone it, or they share their screen.
• There is one open pull request. It compiles. It probably even works for the happy path. Your job is to review it before it merges.
• For the first few minutes you read: the project, then the PR. Narrate as you go. They want to hear your first impressions of unfamiliar code.
• Then you walk the PR the way you would on GitHub: "here is something I would flag, here is why, here is what I would suggest instead." They push back, ask "what happens if...", and discuss. That back-and-forth is the interview.
• If you finish with time to spare, they often ask you to apply one of your fixes, or to add a small feature on top. More on that at the end.

The codebase you're handed

Take a breath and read before you react. This is a tiny Spring Boot service over a MongoDB movies collection (picture the sample_mflix dataset). Click through it:

1package com.example.catalog;
2 
3import com.mongodb.client.MongoCollection;
4import org.bson.Document;
5import org.bson.types.ObjectId;
6 
7import java.util.Optional;
8 
9import static com.mongodb.client.model.Filters.eq;
10 
11public class MovieService {
12 
13    private final MongoCollection<Document> movies;
14 
15    public MovieService(MongoCollection<Document> movies) {
16        this.movies = movies;
17    }
18 
19    public Optional<Movie> findById(String id) {
20        Document doc = movies.find(eq("_id", new ObjectId(id))).first();
21        if (doc == null) {
22            return Optional.empty();
23        }
24        return Optional.of(toMovie(doc));
25    }
26 
27    static Movie toMovie(Document doc) {
28        Movie movie = new Movie();
29        movie.setId(doc.getObjectId("_id").toHexString());
30        movie.setTitle(doc.getString("title"));
31        movie.setYear(doc.getInteger("year", 0));
32        movie.setRating(doc.getDouble("rating"));
33        return movie;
34    }
35}

How to orient yourself in about three minutes, out loud:

• Find the entry point and the shape. CatalogApplication is a stock Spring Boot app, MovieController exposes /movies, MovieService talks to a MongoCollection<Document> through the MongoDB Java driver, and Movie is the model. That is the whole app.
• Read the existing MovieService closely, because it sets the house style. Notice the patterns the team already uses: Filters.eq from the driver's builders (not hand-built Document queries), Optional for "might not be there," and a shared toMovie(Document) mapper. Strong reviewers anchor on the existing conventions, because the most useful feedback is often "there is already a pattern for this in the file."
• Glance at the test. MovieServiceTest actually asserts on the mapper. That tells you the team expects tests to assert, not just run.

Then say the one-liner: "So it is a thin Spring Boot service: a controller, a service that wraps a MongoDB collection with the Java driver, a model, and a unit test. The house style is driver builders, Optional, and a shared mapper." Now you are ready to review.

The pull request

Here is the change you have been asked to review. The author's description:

PR #142: Add movie title search

Adds a GET /movies/search endpoint so the frontend can search movies by title, paginated ten at a time. Tested locally against my Atlas cluster, works great. 🎬

The diff is below: new lines are highlighted, and the three changed files carry a dot in the tree.

1package com.example.catalog;
2 
3import com.mongodb.client.FindIterable;
4import com.mongodb.client.MongoCollection;
5import com.mongodb.client.MongoCursor;
6import org.bson.Document;
7import org.bson.types.ObjectId;
8 
9import java.util.ArrayList;
10import java.util.List;
11import java.util.Optional;
12 
13import static com.mongodb.client.model.Filters.eq;
14 
15public class MovieService {
16 
17    private final MongoCollection<Document> movies;
18 
19    public MovieService(MongoCollection<Document> movies) {
20        this.movies = movies;
21    }
22 
23    public Optional<Movie> findById(String id) {
24        Document doc = movies.find(eq("_id", new ObjectId(id))).first();
25        if (doc == null) {
26            return Optional.empty();
27        }
28        return Optional.of(toMovie(doc));
29    }
30 
31    // Added in this PR: search movies by title, 10 per page.
32    public List<Movie> searchByTitle(String title, int page) {
33        try {
34            Document query = new Document("title",
35                    new Document("$regex", ".*" + title + ".*"));
36 
37            FindIterable<Document> found = movies.find(query);
38 
39            List<Movie> results = new ArrayList<Movie>();
40            MongoCursor<Document> cursor = found.iterator();
41            while (cursor.hasNext()) {
42                Document doc = cursor.next();
43                Movie m = new Movie();
44                m.setId(doc.getString("_id"));
45                m.setTitle(doc.getString("title"));
46                m.setYear(doc.getInteger("year"));
47                m.setRating(doc.getDouble("rating"));
48                results.add(m);
49            }
50 
51            int from = page * 10;
52            int to = from + 10;
53            if (to > results.size()) {
54                to = results.size();
55            }
56            return results.subList(from, to);
57        } catch (Exception e) {
58            e.printStackTrace();
59            return null;
60        }
61    }
62 
63    static Movie toMovie(Document doc) {
64        Movie movie = new Movie();
65        movie.setId(doc.getObjectId("_id").toHexString());
66        movie.setTitle(doc.getString("title"));
67        movie.setYear(doc.getInteger("year", 0));
68        movie.setRating(doc.getDouble("rating"));
69        return movie;
70    }
71}

Before you comment on any single line, do this:

1. Read the description for intent. The author wants title search with pagination. Reasonable feature. Now you know what "correct" should look like.
2. Build the mental model. The new method builds a regex query on title, pulls every match through a cursor, maps each document to a Movie, then slices out a page. Four steps. Hold them in your head.
3. Run it in your mind on a realistic input. Picture searching "the" against a collection of 100,000 movies. What actually happens? That single question surfaces the most important issue in the whole PR.

How to run the review: lenses, in priority order

Strong reviewers do not read top to bottom and comment on whatever catches their eye. They pass over the diff several times, each pass through a different lens, most important first:

1. Correctness. Does it do the right thing, including on empty results, missing fields, and odd inputs?
2. Data safety and security. Untrusted input reaching the query, injection, resource leaks.
3. Performance and scale. How does this behave on a large collection? This is the MongoDB-flavoured lens, and the one they care about most.
4. API and design. Does it fit the existing patterns and the REST contract?
5. Tests. Do they actually prove the change works?
6. Style and polish. Naming, constants, idioms. Last, and lightest.

Now let's walk PR #142 through those lenses.

🔴 Blocking: raise these first

1. It loads the whole result set into memory, then paginates

This is the one to lead with. Look at the order of operations:

FindIterable<Document> found = movies.find(query);
// ...map every matching document into `results`...
int from = page * 10;
int to = from + 10;
return results.subList(from, to);

The pagination happens in application memory after every matching document has already been pulled out of MongoDB. On sample_mflix that is harmless. On a real collection, a search for "the" could drag hundreds of thousands of documents across the network and into the JVM heap just to hand back ten of them.

What you would say: "My one blocking comment is the pagination. We are fetching every match into a list and then slicing a page out of it in memory, so the cost scales with how many movies match, not with the page size. I would push skip and limit into the query and let MongoDB do the paging."

The fix:

movies.find(filter)
      .skip(page * PAGE_SIZE)
      .limit(PAGE_SIZE)
      .iterator();

A senior bonus catch, for free: the in-memory version also has a latent crash. results.subList(from, to) throws IndexOutOfBoundsException when from is past the end of the list, so asking for page 5 of a 12-result search blows up. Moving paging into the query makes that bug disappear too, an out-of-range page simply returns an empty list.

2. It builds a regex out of raw user input

Document query = new Document("title",
        new Document("$regex", ".*" + title + ".*"));

There are three problems stacked in that one line, and a strong candidate peels all three:

• The input is not escaped. Whatever the user types is spliced straight into a regular expression. Regex metacharacters change the meaning of the query, and a pathological input can trigger catastrophic backtracking (a ReDoS) that pins a CPU on the server. Wrap the input with Pattern.quote(...).
• It cannot use an index. A leading .* makes this an unanchored substring scan, so MongoDB cannot use a normal index on title and falls back to a collection scan. For real search you want a text index ($text) or Atlas Search, which is MongoDB's purpose-built full-text engine. Worth saying out loud even if you do not implement it.
• It is case-sensitive. Searching "Matrix" will not find "the matrix". Add the case-insensitive option.

What you would say: "This regex is built by string concatenation from user input. I would at least escape it with Pattern.quote and make it case-insensitive. But I would also flag that a leading-wildcard regex can't use an index, so at scale the right tool here is a text index or Atlas Search. Can we talk about whether this needs to be true substring search or whether prefix search would do?"

The fix (pragmatic version):

Bson filter = Filters.regex("title", Pattern.quote(title), "i");

3. It swallows the exception and returns null, and the test hides it

These two are listed together on purpose, because they combine into something worse than either alone.

} catch (Exception e) {
    e.printStackTrace();
    return null;
}

Catching Exception swallows everything, including bugs that should be loud, logs to stderr with printStackTrace instead of a real logger, and then returns null. The controller hands that null straight back, so the endpoint answers 200 OK with an empty body, or a caller downstream hits a NullPointerException far from the real cause.

Now look at the test that shipped with the PR:

@Test
void testSearch() {
    MovieService service = new MovieService(null);
    List<Movie> results = service.searchByTitle("matrix", 0);
    System.out.println(results);
}

It constructs the service with a null collection, so the very first line inside searchByTitle throws a NullPointerException. That exception is swallowed by the catch, the method returns null, and the test prints null and passes. The method is completely broken, and the test is green. That is the single most important thing to point out in the whole review: the swallowed exception and the assertion-free test conspire to make a broken method look healthy.

What you would say: "Two things that I think are actually the same problem. The catch block swallows every exception and returns null, which a search method should never do, it should return an empty list or let the error propagate. And the new test has no assertions and passes a null collection, so it would stay green even if the method always failed. I would have the method return an empty list, drop the catch-all, and make the test assert on real results."

The fix: delete the catch-all (let genuine failures surface, or wrap them with context), and return an empty list, never null:

if (title == null || title.isBlank()) {
    return List.of();
}

🟠 Should-fix: clearly worth changing

4. The cursor is never closed

MongoCursor<Document> cursor = found.iterator();
while (cursor.hasNext()) { /* ... */ }

A MongoCursor holds a server-side cursor and a connection. If anything throws mid-loop, or even on the happy path here, it is never closed, which leaks resources under load. MongoCursor is Closeable, so wrap it in try-with-resources:

try (MongoCursor<Document> cursor = movies.find(filter)./*...*/.iterator()) {
    while (cursor.hasNext()) {
        results.add(toMovie(cursor.next()));
    }
}

5. It reinvents the existing mapper, and reintroduces a bug

The PR maps documents to Movie by hand:

m.setId(doc.getString("_id"));
m.setYear(doc.getInteger("year"));

But MovieService already has a toMovie(Document) helper three methods up, and the hand-rolled copy is subtly wrong: _id is an ObjectId, not a String, so doc.getString("_id") returns null, and getInteger("year") with no default unboxes to a NullPointerException if year is ever missing. The existing toMovie already handles both correctly.

What you would say: "There is already a toMovie helper in this file that does this mapping, and it handles the ObjectId id and a missing year properly. Reusing it removes this duplication and also fixes the _id mapping bug here."

6. Paginating without a sort gives unstable pages

find(...).skip(...).limit(...) with no sort relies on MongoDB's natural order, which is not guaranteed to be stable between calls. The same movie can appear on page 1 and page 2, or be skipped entirely, as data changes. Pagination needs a deterministic sort key:

.sort(Sorts.ascending("title"))

7. No input validation

There is no guard for a null or blank title, or a negative page. A blank title should return an empty list (or everything, but decide deliberately), and a negative page should be clamped. The isBlank() guard above covers the title; Math.max(page, 0) covers the page.

🟡 Polish: mention briefly, do not dwell

These are the kind of notes you raise quickly and explicitly mark as minor, so the author knows they are not blocking:

• Magic number. 10 is hard-coded and appears twice. Pull it into a private static final int PAGE_SIZE.
• Diamond operator. new ArrayList<Movie>() can be new ArrayList<>().
• Endpoint ergonomics. In the controller, @RequestParam int page is required, so a caller who omits page gets a 400. A defaultValue = "0" is friendlier and matches how the rest of the API behaves. The existing getById returns a ResponseEntity; returning a bare List for search is acceptable (an empty list plus 200 is a fine "no results"), but it is worth a sentence acknowledging the inconsistency.

The refactor, all together

Pull the blocking and should-fix items together and the change becomes small, safe, and fast. Here is the version a strong candidate lands on (changed lines highlighted):

1package com.example.catalog;
2 
3import com.mongodb.client.MongoCollection;
4import com.mongodb.client.MongoCursor;
5import com.mongodb.client.model.Filters;
6import com.mongodb.client.model.Sorts;
7import org.bson.Document;
8import org.bson.conversions.Bson;
9import org.bson.types.ObjectId;
10 
11import java.util.ArrayList;
12import java.util.List;
13import java.util.Optional;
14import java.util.regex.Pattern;
15 
16import static com.mongodb.client.model.Filters.eq;
17 
18public class MovieService {
19 
20    private static final int PAGE_SIZE = 20;
21 
22    private final MongoCollection<Document> movies;
23 
24    public MovieService(MongoCollection<Document> movies) {
25        this.movies = movies;
26    }
27 
28    public Optional<Movie> findById(String id) {
29        Document doc = movies.find(eq("_id", new ObjectId(id))).first();
30        if (doc == null) {
31            return Optional.empty();
32        }
33        return Optional.of(toMovie(doc));
34    }
35 
36    public List<Movie> searchByTitle(String title, int page) {
37        if (title == null || title.isBlank()) {
38            return List.of();
39        }
40        int skip = Math.max(page, 0) * PAGE_SIZE;
41 
42        Bson filter = Filters.regex("title", Pattern.quote(title), "i");
43 
44        List<Movie> results = new ArrayList<>();
45        try (MongoCursor<Document> cursor = movies.find(filter)
46                .sort(Sorts.ascending("title"))
47                .skip(skip)
48                .limit(PAGE_SIZE)
49                .iterator()) {
50            while (cursor.hasNext()) {
51                results.add(toMovie(cursor.next()));
52            }
53        }
54        return results;
55    }
56 
57    static Movie toMovie(Document doc) {
58        Movie movie = new Movie();
59        movie.setId(doc.getObjectId("_id").toHexString());
60        movie.setTitle(doc.getString("title"));
61        movie.setYear(doc.getInteger("year", 0));
62        movie.setRating(doc.getDouble("rating"));
63        return movie;
64    }
65}

Notice what the refactor does and does not do. It changes only what the PR touched: findById and the existing toMovie are left exactly as they were. That restraint is itself a senior signal. Reviewers who rewrite the whole file lose trust, the discipline is to fix the change under review, not to redecorate.

What each change bought you:

• Paging in the query (skip + limit): cost now scales with page size, not collection size, and the out-of-range crash is gone.
• Filters.regex(..., Pattern.quote(title), "i"): input is escaped, search is case-insensitive, and you have flagged the index conversation for later.
• try-with-resources: the cursor is always closed.
• Reusing toMovie: less code, and the _id and year bugs vanish.
• A real test that asserts on the validation path, instead of one that prints and always passes.

Delivering the review like a teammate, not a gatekeeper

The fixes are half the score. How you deliver them is the other half, and it is where this round really separates people. A few habits that read as senior:

• Lead with a summary and something genuine you liked. "The feature works and the wiring is clean" costs you nothing and buys a lot of goodwill.
• Order by severity, and label it. Say which comments are blocking, which are should-fix, and which are nits, so the author knows where to spend their attention.
• Frame as questions and suggestions, not commands. "What do you think about pushing the paging into the query?" lands far better than "this is wrong." You are reviewing the code, not the person.
• Explain the why, with the blast radius. "This matters because on a big collection it pulls everything into memory" teaches; "don't do this" does not.
• Offer to pair. "Happy to jump in and apply the paging fix together if that is useful" signals you are a collaborator.

If there's time: the small feature add-on

Candidates who handle the review well are often asked, with the time that remains, to apply one of the fixes for real or to add a small feature on top. This is a bonus on top of the review and refactor, not the main event, but it is common enough to rehearse. A typical ask here would be: "Now let users filter the search by a minimum rating."

It is a small, natural extension of the method you just cleaned up. Add a parameter, add one more clause to the filter with the driver's builders, and (a nice touch) sort by rating so the best matches come first:

public List<Movie> searchByTitle(String title, int page, double minRating) {
    if (title == null || title.isBlank()) {
        return List.of();
    }
    int skip = Math.max(page, 0) * PAGE_SIZE;

    Bson filter = Filters.and(
            Filters.regex("title", Pattern.quote(title), "i"),
            Filters.gte("rating", minRating));

    List<Movie> results = new ArrayList<>();
    try (MongoCursor<Document> cursor = movies.find(filter)
            .sort(Sorts.descending("rating"))
            .skip(skip)
            .limit(PAGE_SIZE)
            .iterator()) {
        while (cursor.hasNext()) {
            results.add(toMovie(cursor.next()));
        }
    }
    return results;
}

And expose it on the endpoint with a sensible default so existing callers keep working:

@GetMapping("/search")
public List<Movie> search(
        @RequestParam("q") String query,
        @RequestParam(name = "page", defaultValue = "0") int page,
        @RequestParam(name = "minRating", defaultValue = "0") double minRating) {
    return movieService.searchByTitle(query, page, minRating);
}

Talk through the trade-off as you go: "Filters.and keeps this readable, and now that we filter and sort on rating, this is the point where I would add a compound index on title and rating so the query and the sort can both be served by the index." That single sentence ties the feature back to the performance lens and shows you think about MongoDB, not just Java.

What earns a Strong Hire

Your game plan, start to finish

1. Orient (about 3 minutes). Entry point, the one service, the model, the test. Say the one-liner.
2. Read the PR for intent, build the four-step mental model, and run it in your head on a big collection.
3. Give your headline, then review worst-first: correctness and safety, then performance, then design and tests, then nits.
4. Discuss, do not lecture. Pause after each blocking item. Frame as questions. Praise what is good.
5. If time allows, apply a fix or add the small feature, and tie it back to indexing.

This round is a direct read on the teammate MongoDB wants: someone who reads unfamiliar code quickly, knows what actually breaks at scale, fixes it cleanly in the language and the database, and makes their teammates better in the process. Review a few PRs like this one out loud until the worst-first instinct is automatic, and you will walk in ready. 🚀