Reviewing Spring code

Code review is where a Spring codebase's quality is actually maintained — by another engineer (or you, reviewing your own diff) asking the questions this course has taught, before the change ships. This lesson is a Spring-specific review checklist, ordered by how much damage each issue causes. It is the whole course compressed into what to look for in a pull request.

The mindset

Review catches expensive problems cheaply — a security hole, a lost update, an N+1 that will crawl on production data — before they reach users. Review the change, not the person; ask questions ("what happens if two requests hit this at once?") rather than deliver verdicts. And on this course, where merging deploys to real users, you are often your own reviewer — so run this checklist on your own diff before opening the PR, not only on others'.

The security pass — first, because it is most dangerous

Security bugs cause the worst outcomes, so look for them first (the security module):

• Object-level authorisation (IDOR). Does an endpoint fetching by id check the record belongs to the caller, or does a role check alone let any authenticated user read anyone's data? Scope queries to the principal; return 404 for records they should not know exist. The most damaging logic bug — always check it.
• Input validated? @Valid on request bodies; no trusting client-supplied ids/roles for authorisation (use the authenticated principal).
• Secrets out of the code? No credentials or JWT secret committed; config from the environment.
• Entities not exposed? DTOs at the boundary, so sensitive fields do not leak.
• CSRF handled correctly for the auth model; no stack traces returned to clients.

A single missed object-level check or committed secret is worse than any number of style issues — hence this pass comes first.

The data-integrity and concurrency pass — second, hardest to undo

Bugs that corrupt data are next (the concurrency and data modules):

• Transactions. Are multi-write operations @Transactional? Is the rollback rule right (checked exceptions need rollbackFor)? Is @Transactional on a service method, not self-invoked?
• The lost update. Any read-modify-write on data concurrent requests touch, without @Version (optimistic locking) or a pessimistic lock? That is silent corruption under load.
• Constraints. Are invariants enforced by database constraints (unique, check), not just application checks that a race can bypass?

Data corruption often cannot be fixed after the fact — scrutinise anything that writes.

The performance pass — third, fails silently at scale

The issues that pass a demo and die on production data (the JPA modules):

• N+1 queries. Any loop (or serialisation) crossing a lazy relationship without a fetch join / @EntityGraph? Ask for the query count. This is the most common performance bug.
• Whole entities where a projection would do, on read-heavy endpoints.
• Missing pagination on a list endpoint that could return unbounded rows.
• Missing indexes on filtered/sorted columns on tables that will grow.

These are invisible with ten rows, so they must be reasoned about — "what does this do with 100,000 parcels?" is the reviewer's job.

The design and correctness pass

Then the everyday quality questions (the layering and best-practices lessons):

• Layering. Is business logic in the service (not the controller or repository)? Is the controller thin? Are queries on the repository, not scattered?
• Constructor injection, not field injection; final dependencies.
• DTOs at the boundary; validation on inputs.
• Failure cases handled and tested — not just the happy path (invalid input, missing object, unauthorised); are there tests, and do they cover failures?
• Exceptions thrown meaningfully and not swallowed; logging with context where a production failure would otherwise be silent.
• Names that say what things are; no needless complexity (a hand-rolled query where a derived method fits; a Service class where a function would do — the patterns module).

A practical review checklist

In priority order, to run against a Spring PR:

1. Security — object-level auth (IDOR), input validated, secrets externalised, entities not exposed, CSRF/error-leak handled.
2. Data & concurrency — transactions correct, no lost-update race (@Version/locks), invariants as DB constraints.
3. Performance — no N+1, projections for reads, pagination, indexes.
4. Layering & DI — logic in the service, thin controllers, constructor injection.
5. API & correctness — DTOs, validation, right status codes, failure cases handled and tested.
6. Clarity — good names, no over-engineering, exceptions/logging honest.
7. Config & migrations — no secrets committed, Flyway migration included for schema changes, ddl-auto=validate in production.

Run them in order and do not fuss over clarity on a PR that fails the security pass — fix the dangerous things first. Every item traces back to a lesson; being able to apply them to a real diff — your own or a colleague's — is what "worth hiring" means, and it is the point the whole best-practices module builds to.

Check your work

Mindset. Review the change not the person, ask questions; you are often your own reviewer here, so run the checklist on your own diff before the PR.

Priority order. Security first (IDOR, validation, secrets, entity exposure, CSRF/leaks), then data & concurrency (transactions, lost-update/@Version, DB constraints), then performance (N+1, projections, pagination, indexes), then layering/DI, correctness & tests, clarity, config/migrations.

The headline checks. Object-level authorisation (the most damaging logic bug), the lost update, N+1, business logic in the service, DTOs at the boundary, failures tested.

Fix dangerous first. Do not polish clarity on a PR that fails security; work top-down.

Practice

1. Run the seven-item checklist against a recent Spring change (yours or a colleague's); write one comment per issue found.
2. Review a diff specifically for IDOR: for every fetch-by-id, check it is scoped to the caller.
3. Review for N+1: find every loop/serialisation crossing a relationship and ask whether it is fetch-joined.
4. Find business logic in a controller during review and suggest where it should move.
5. Check a multi-write operation is @Transactional with the correct rollback behaviour and no self-invocation.
6. Review your own next change before opening the PR and catch at least one issue you would have shipped.

Official documentation

• OWASP — API Security Top 10 — The security items, including IDOR.
• Spring — Data access optimization — The performance items.
• Spring — Core technologies (DI, beans) — The layering and injection items.

Next: Spring already encodes the patterns.