Repository navigation
v2: fixes and hardening from review of #6–#8 - #9
Conversation
Each has a regression test that failed before the fix. - cursor pagination with select() repeated documents and did not terminate: sort keys were left out of the projection, so the next cursor was built from defaults. Sort keys are now always fetched - bulk_save left memory out of sync after a partial failure. Writes in the batch are independent; the ones that succeeded are now brought in sync and the error lists the rest in .failed. Which updates applied is determined by reading the documents back, since the server only reports a total - bulk_write hard deletes skipped OnDelete cascade rules - a cycle of cascading references recursed forever; cascades now carry the set of documents already being deleted - cascades resolved ids with distinct() (16MB limit) and one unbounded $in; ids are now read with find and applied in chunks - saving an untracked instance that has an id (a copy, a deepcopy, a lean read) raised a bare duplicate-key error; it now says the instance is not tracked and what to do - reload() loaded Deferred fields - Query.update() returns an int that also carries .matched - the Location header of a create was a bare id; it is now a URL under the API prefix Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From review of the web layer. - request bodies are read under the size limit on every plug (declared length checked first, then chunk by chunk); previously only the pure ASGI plug stopped at the limit and the others buffered the whole body - DELETE applies the row scope in the delete's own filter, not only in the lookup before it, and honours If-Match - deleting a loaded document checks its revision on hard deletes too - offset pagination refuses pages deeper than Pagination.max_offset (10,000 skipped documents by default) - totals are opt-in per resource: Pagination.total now defaults to False - reads made for a request carry a 10s time limit when neither the registry nor the model sets one - Api(openapi=False) keeps the API description private on every plug - HEAD and OPTIONS are answered by Api.handle and the pure ASGI plug - Resource(explicit_writes=True): a field is writable only when its Policy says so - mongospecs_web.testing.mount_on(plug, api) builds an app on any built-in plug BREAKING: ?total=true needs page=Cursor(total=True) / Offset(total=True). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- parity page: 28 of 35 Mongoose behaviours now confirmed against its documentation; three more deliberate differences recorded - plan amendments A25-A29: the computed decorator, Atlas Search and vector indexes declared out of scope for 0.2 (not testable here), revision-checked hard deletes, the changed web defaults - guides: limits and defaults of the web layer, untracked documents, partial bulk_save failures, registries and explicit managers - CI: MongoDB 7.0 on Python 3.11, 3.13 and 3.15 - publish workflow runs lint, types and the test suite first Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis PR hardens the v2 core and web layers against reproduced review defects: write and cascade operations now reconcile partial outcomes safely, pagination and tracking preserve correctness, HTTP requests and deletes enforce bounded and conditional behavior, and the supported integrations, documentation, and CI receive matching regression coverage and defaults. Sequence diagram for safe bulk save reconciliationsequenceDiagram
participant App
participant Manager
participant MongoDB
participant Documents
App->>Manager: bulk_save(docs)
Manager->>MongoDB: bulk_write(updates, ordered=False)
MongoDB-->>Manager: write result or OperationFailed
alt partial failures or matched count mismatch
Manager->>MongoDB: find(ids)
MongoDB-->>Manager: current documents
Manager->>Manager: compare applied updates
Manager->>Documents: re-track successful documents
Manager-->>App: error.failed = unsuccessful documents
else all writes succeed
Manager->>Documents: re-track all documents
Manager-->>App: written count
end
Sequence diagram for conditional scoped deletionsequenceDiagram
actor Client
participant Api
participant Resource
participant MongoBackend
participant MongoDB
Client->>Api: DELETE /resource/id with If-Match
Api->>Resource: dispatch Delete
Resource->>MongoBackend: delete(id, scope, if_rev)
MongoBackend->>MongoDB: find document by id and scope
MongoDB-->>MongoBackend: loaded document
MongoBackend->>MongoDB: delete with scope and revision filter
alt revision or scope no longer matches
MongoDB-->>MongoBackend: no match
MongoBackend-->>Api: StaleDocumentError or DocumentNotFound
Api-->>Client: error response
else match succeeds
MongoDB-->>MongoBackend: deleted
MongoBackend-->>Api: success
Api-->>Client: 204 response
end
Sequence diagram for cycle-safe cascading deletessequenceDiagram
participant Manager
participant MongoDB
Manager->>Manager: delete_many(filter, seen)
Manager->>MongoDB: find matching ids
MongoDB-->>Manager: ids in chunks
Manager->>Manager: _cascade(ids, seen)
Manager->>Manager: delete_many(cascade filter, _seen=seen)
Manager->>Manager: skip ids already in seen
Manager->>MongoDB: delete_many(chunk ids)
MongoDB-->>Manager: deleted count
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Each has a regression test that failed before the change
(tests/test_review_round2.py in both packages).
core
- a field whose default is None is not stored while it is None, and
setting it to None unsets it; a unique sparse index no longer
collides on stored nulls
- field names that shadow the Document API raise ConfigurationError
- registry.transaction() inside a running transaction raises unless
independent=True (it would commit on its own)
- UpdateCount formats as its int value in str() and f-strings
- filter values are cast by one rule on every backend
- empty Or()/Nor(), non-text prefix()/contains(), sort keys that are
not field paths and server type mismatches raise QueryError
- include() checks its path and strategy when the query is built
- find().on_invalid('raise' | 'skip' | 'raw')
web
- integers beyond 64 bits are a 422 on create and patch, and an
EncodingError is no longer a 500
- Quart: init_app installs BodyLimit, which reads API bodies under the
limit before Quart queues them
- a guard runs once per request however many places list it
- OpenAPI publishes only referenced definitions and no private field
- HEAD on the ASGI plug reports the length of the GET body
BREAKING: optional fields that are None are omitted from stored
documents, so an exists filter now means 'has a value'.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Private fields of embedded (non-document) models were serialised, writable and described in OpenAPI. They are now handled like private fields of the document itself, at any depth and inside lists, dicts, optionals and unions: never in a response (including included documents), a 422 'unknown field' when a client supplies one on create or patch, and absent from every published schema. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Leaving None-valued optional fields out of stored documents stays the default. StoreNone on a field, or none='store' on a model, stores an explicit null instead (OmitNone opts a field back out). Encoding, change tracking, update(), upsert defaults and the bulk paths follow the field's mode; loading never produces a spurious change in either mode. A unique sparse index over a field that stores null is refused with a pointer to a partial index. Also applies the omit rule to embedded pydantic models, which were encoded with their None values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- web GET list/get read stored documents with a projection of the readable fields and serialise them directly when that is certain to equal the model path; anything else falls back to the model - DELETE is one conditional write unless the model needs the document (delete hooks, cascades) - independent includes are loaded concurrently (not inside a session) - per_parent_limit on a reverse relation is applied on the server - tracked documents use one weak reference instead of weakref.finalize - Query._clone copies slots instead of re-running __init__ - routes are looked up by resource before patterns are tried fix(core): cursor pagination no longer skips or stops at documents without a value for a sort key Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Requests beyond the limit are answered at once with 503 and Retry-After instead of queueing for database connections. test: correctness of the optimised paths (tracking, clone, includes, per-parent limits, routing) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…om the stored document Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- docs/guide/production.md: pool and timeout settings, backpressure, read and pagination choices, relations, writes, what needs a replica set; each item marked measured, tested or guidance - docs/benchmarks.md: hot paths before and after the review - bench/results: B9 before (49a32a9) and after (fb543de) - plan: decision per review finding, amendment A36 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An ObjectId, Binary, Timestamp, Regex or Int64 inside a field typed Any (or dict[str, Any], list[Any]) made the JSON conversion fail, and the web layer answered 500: on pydantic for any ObjectId, on msgspec and attrs for the BSON types their serialiser does not know. All three backends now give the same JSON for such values, at any depth. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A PATCH that gave an object for an embedded model replaced the stored object, writing defaults over everything the client did not send, including private, read-only and immutable values it is not allowed to set. Objects are now merged key by key (RFC 7396) in one atomic update; lists and dicts are still replaced whole. - nested keys are held to the embedded model's own Policy - an embedded object holding protected values cannot be set to null, and a list or dict of such objects cannot be replaced through PATCH - Policy(insert=False) is enforced on nested keys at create - the OpenAPI Patch schema describes embedded objects as partial BREAKING: clients that relied on PATCH replacing an embedded object must now send null for the keys they want cleared. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PATCH merging into embedded objects (A37) had no way out for resources
that need the embedded model validated whole, need to write over a
stored null, or use a backend without supports_merge.
- Resource(merge_patch=False): PATCH replaces an embedded object with
the one sent. Values the client cannot send go back to their
defaults; nested keys the policy closes are still rejected.
- Put: PUT /{id} replaces every field the client may update (the sent
value, or the default when left out) and leaves the rest as stored,
in one atomic update with the If-Match check. An embedded object
holding protected values is replaced key by key; one without is
replaced whole. It never creates, and is not in the default ops.
- OpenAPI gains <Model>Put schemas where a resource has the operation.
Recorded as A39 in the plan.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each was reproduced against the replica set before it was fixed; the tests are in test_review_round3.py of both packages. Core - A single-field update on a pydantic model failed a model-level validator that only saw the other fields' defaults. The field's own validators still run; the cross-field verdict is no longer taken. - insert_many and bulk_save run document hooks, as insert and save do. - page() starts its count after its arguments are validated, so a bad cursor no longer leaves an un-awaited coroutine. Web - Request bodies are strict JSON: NaN, Infinity and over-deep nesting are a 400 (NaN was stored; deep nesting was a logged 500). - If-Match on a model without a Revision is a 412, not an unconditional write. If-None-Match accepts lists, weak tags and *. - The ASGI plug drops a request whose client disconnected mid-body. - Api(openapi=False) is honoured by the FastAPI plug. - Guards and hooks must name an operation the resource has. - The concurrency limit is applied before authentication. - A filter or sort key given twice is a 400. Recorded as A40 in the plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Branch pushes and their pull requests both triggered CI, so every job ran twice. mongospecs[all] now includes the otel and sentry extras, and mongospecs-web declares msgspec as the optional encoder it already uses when present. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A new user had reference-quality guides and no way into them. - examples/: a quickstart script, a to-do REST API with its tests, and a compose file for a single-node replica set. They are run by the suite, and the tutorial page and the README show them as they are. - docs/tutorial.md: from MongoDB to a model, an API and its tests. - New guides: choosing a backend, errors (what each means and the HTTP status it becomes), authentication and authorization, testing an API. The last two are executed like the other guide pages. - The replica-set requirement is stated in the README, the landing page and the tutorial instead of only in the production notes. - The package README is checked to be the root README. - The reference section is removed: it was generated from docstrings, and the source is the better reference. mkdocstrings goes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- CHANGELOG.md, CONTRIBUTING.md and SECURITY.md. - A workflow deploys the documentation to GitHub Pages from main. - The publish workflow refuses a release tag that does not match the version in the package it names. - Package metadata links to the documentation and the changelog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
msgspec and attrs do not check types when a model is constructed or assigned to, and pydantic does not check assignments. A wrongly typed value was therefore stored as it was, and the document could not be loaded afterwards. insert, save, insert_many and bulk_save now decode the document's own encoding with the backend's strict rules before sending anything, and raise ValidationError if it does not fit. save checks only when there is a change, and only the changed fields of a document loaded with a projection. The check can be skipped with validate=False on a call or SpecConfig(validate_writes=False) on a model. It cost about 3 µs (msgspec) and 5 µs (pydantic) per seven-field document when measured. Recorded as A41 in the plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the site config to zensical.toml on the new default theme, swap the docs dependency, and deploy through the GitHub Pages actions since zensical has no gh-deploy. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Stacked on #8. A second session reviewed the three v2 PRs and reported five reproduced bugs plus read-only findings. I reproduced each bug with a failing test before fixing it; reproducing them turned up one more (cyclic cascades recursed forever).
Bugs fixed (commit 1)
select()+sort()+ cursorpage()repeated documents and never terminatedLocationafter a create was a bare idApi(which knows where the resource is mounted)bulk_saveleft memory out of sync after a partial failure.failed. The server only reports a total, so on a mismatch the documents are read back and compared with what each update should have writtenbulk_writehard deletes skippedOnDeletecascadescopy.copy()of a tracked document raised a bareDuplicateKeyErrorAlso from the read-only findings: cascades read ids with
findand work in chunks instead ofdistinct()and one unbounded$in;reload()no longer loadsDeferredfields;Query.update()returns anintthat also carries.matched.Web hardening (commit 2)
DELETEapplies the row scope in the delete's own filter and honoursIf-Match. Hard deletes of a loaded document are now revision-checked too.max_offset(10,000 skipped documents by default).Api(openapi=False),HEAD/OPTIONSonApi.handleand the pure ASGI plug, andResource(explicit_writes=True)for default-deny field policy.Breaking:
?total=trueis now opt-in per resource (page=Cursor(total=True)).Follow-ups (commit 3)
docs/parity.md: 28 of 35 Mongoose behaviours are now confirmed against its documentation; 7 are still from memory and marked.PATCH and PUT on embedded objects (
933df43,ecb42e6)PATCHmerges an object given for an embedded model into the stored one, key by key (RFC 7396), in one atomic update. Replacing it wrote defaults over everything the client did not send, including private, read-only and immutable values it is not allowed to set. Lists and dicts are still replaced whole. (A37)Resource(merge_patch=False)turns the merge off for a resource:PATCHreplaces the embedded object with the one sent. That is the way out when the embedded model has cross-field validators, when clients must be able to write over a storednull, or when the backend has nosupports_merge. The cost is the hazard the merge removes: values the client cannot send go back to their defaults. Nested keys the policy closes are rejected either way. (A39)Putoperation:PUT /{id}replaces every field the client may update (the sent value, or the default when left out; a required field must be sent) and leaves the rest as stored, with the sameIf-Matchcheck. An embedded object holding protected values is replaced key by key; one without is replaced whole. It never creates. OpenAPI gains<Model>Putschemas. (A39)Putis not in the defaultops. Guards are keyed by operation, so adding it to the defaults would expose an unguarded write on resources that only guardPatch.Breaking: clients that relied on
PATCHreplacing an embedded object must sendnullfor the keys they want cleared, or the resource setsmerge_patch=False.Third review (
7a4012b,0c5388a)I reviewed the stack as one codebase and reproduced nine defects against the replica set. Each has a test that failed first (
test_review_round3.pyin both packages). Recorded as A40.Model.update()andPATCHon a pydantic model failed a cross-field validator that only saw the other fields' defaultsNaNandInfinityin a body were accepted and stored, then came back asnullIf-Matchon a model without aRevisionwas ignored and the write went throughIf-Match: *passes)Api(openapi=False)did nothing on FastAPI/openapi.jsonand/docsinsert_manyandbulk_saveskipped document hooksinsertandsavedototal=Trueleft an un-awaited coroutineValueErrorwhen theApibuilds its routesAlso:
If-None-Matchaccepts lists, weak tags and*; the concurrency limit is applied before authentication; a filter or sort key given twice is a 400. CI ran every job twice per push (push and pull_request triggers); it now runs once.mongospecs[all]includes theotelandsentryextras.insert_manyrunning hooks departs from Mongoose, whereinsertManydoes not runsavemiddleware. Abefore("insert")hook that silently does not run loses data; the calls are skipped when no hook is registered.Breaking: the 412 on
If-Matchwithout revisions, the 400 on repeated keys and onNaN/Infinity, and theValueErroron unknown guard keys.Writes are validated (
1dac7f6)msgspec and attrs do not check types in the constructor or on assignment, and pydantic does not check assignments, so
Counter(n="many").insert()stored a string and the document could not be loaded afterwards.insert,save,insert_manyandbulk_savenow check the document, in the form it is about to be stored in, and raiseValidationError.savechecks only when something changed, and only the changed fields of a document loaded with a projection.The check can be skipped with
validate=Falseon a call orSpecConfig(validate_writes=False)on a model. It took about 3 µs (msgspec) and 5 µs (pydantic) per seven-field document in an ad hoc measurement; the recorded B4 benchmark predates it and was not re-run. Recorded as A41.Documentation and repository (
c5eba4d,c23ffbc)examples/: a quickstart script, a to-do REST API with its tests, and a compose file for a one-node replica set. They run in the suite; the tutorial and the README include them as they are.CHANGELOG.md,CONTRIBUTING.md,SECURITY.md; a workflow that deploys the docs to GitHub Pages frommain; the publish workflow refuses a tag that does not match the package version.Two things to know before merging: the docs workflow will overwrite the existing
gh-pagesbranch on the first push tomainthat touches docs, andSECURITY.mdpoints at GitHub private vulnerability reporting, which has to be enabled in the repository settings.Not changed, with reasons
registry.manager(Model), which is now documented. The suggested task-scoped "current registry" is not built.Verification
887 tests pass locally (unit, contract, end-to-end, docs examples,
examples/) against MongoDB 8.0 on a replica set and a sharded cluster, with write validation on;ruffandmypy --strictare clean; docs build in strict mode. The example API was also started under uvicorn and called with curl. The chaos test (primary step-down) was not re-run for the later commits. The reviewer tested against MongoDB 6.0.1, which is below this project's floor of 7.0; I did not test on 6.0.Still open: on framework plugs, trailing slashes,
HEADandOPTIONSfollow the framework and differ between plugs (documented indocs/web/plugs.md). The third review did not read relations, indexes, change streams, bulk compilation, the fast-read path or the bench code.🤖 Generated with Claude Code
Summary by Sourcery
Harden v2 database and web APIs against review-identified correctness, consistency, and resource-safety issues while expanding regression coverage and documentation.
New Features:
Nonedefaults, reserved-field validation, consistent query casting, invalid-document read modes, non-nesting transaction protection, and richer update results.Bug Fixes:
LocationURLs, partialbulk_savestate synchronization, hard-delete cascades, untracked-copy save errors, deferred-field reloads, and cyclic cascade recursion.Enhancements:
CI:
Documentation:
Tests: