fix: return clean 404 instead of crashing on POST without a trailing slash - #1807
Merged
Conversation
…slash
RequestTarget('') (an exact resource-path match with nothing left to
parse, i.e. no trailing slash) left both `id` and `isCollection`
undefined instead of the well-defined null/false Resources.getMatch()
produces for every other match shape. Any dispatch or authorization
code that assumed one of the two was always set (rather than reading
a truly `undefined` value) could throw an unhandled TypeError instead
of a clean HTTP error.
- RequestTarget: give the "matched but no trailing slash" case its
own well-defined state (id=null, isCollection=false) instead of
leaving both undefined.
- Resource.ts: reject that state for POST up front with a clear 404,
before any post()/allowCreate() override runs against it.
- Table.ts: update the GET "describe" check to the new null-based
signal so `GET /resource` (no trailing slash) still returns table
metadata instead of regressing to 404.
Refs #678
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cross-model review flagged the Table.ts describe-check change as a risk to collection reads; verified it's a false positive (isSearchTarget already routes isCollection=true targets to search() before the describe check runs) and added an explicit regression test for it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request resolves an issue where POSTing to a resource path without a trailing slash resulted in an unhandled TypeError. It ensures that RequestTarget properly defines id and isCollection as null and false respectively when there is no trailing slash, and updates Resource.ts to throw a clean 404 error for such POST requests. Additionally, Table.ts is updated to use a loose nullish check for target.id, and new unit tests are introduced. The review feedback suggests using node:assert/strict instead of node:assert in the new test file to align with strict equality standards.
Contributor
|
Reviewed; no blockers found. |
…st() The clean-404 guard added for harper#678 fired for every Resource subclass's static POST dispatch, including ones with their own instance post() override (e.g. the redirector template's Redirect.post(), which does a bulk CSV/JSON import via POST to its collection root with no trailing slash — a legitimate, currently-supported convention). That regressed all 6 CI runtime legs on integrationTests/components/redirector.test.ts. Root cause: the guard didn't need to be this broad. It exists to give a purpose -built error message ahead of the base Resource.prototype.post()'s fallback to missingMethod() (which already throws a clean, if generic, 405 for this same null-id/non-collection state -- confirmed by reverting the whole harper#678 fix and re-running post-trailing-slash.test.js: no crash, just a 405 instead of the desired 404). A resource with its own post() override is trusted to handle a null-id/non-collection target itself; it may not consult id/isCollection at all. Now only throws when this.prototype.post === Resource.prototype.post, i.e. the resource has no custom post() to hand the request to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per gemini-code-assist review on #1807: strict-mode assertions align with the current testing standard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POSTing to a Resource path missing its required trailing slash (or hitting it with a trailing slash plus a nonexistent path segment) threw an unhandled
TypeError(Cannot read properties of undefined/null (reading 'query')) instead of a clean HTTP error. The trailing-slash requirement itself is correct, documented, intended behavior — this only fixes the crash.Root cause:
Resources.getMatch()returns an emptyrelativeURL('') when a URL matches a resource's exact base path with no trailing slash.new RequestTarget('')then hit an earlyreturnmeant only for the truly argument-less internal constructor call (new RequestTarget()), leaving bothidandisCollectionundefined— a state distinct from (and easy to confuse with) the well-formed collection target (id: null, isCollection: true) that a real trailing slash produces. Any dispatch/permission code downstream that assumed one of the two was always set could crash on theundefinedvalue.Changes
resources/RequestTarget.ts: the "matched, nothing left to parse, no trailing slash" case now gets its own well-defined state (id: null, isCollection: false) instead of leaving bothundefined.resources/Resource.ts: the sharedtransactional()dispatch wrapper rejects that state for POST up front with a clear404, before any (possibly custom)post()/allowCreate()override ever sees the half-resolved target.resources/Table.ts: updated the GET "describe" check (GET /Tablewith no trailing slash returns table metadata) fromtarget.id === undefinedtotarget.id == nullto match the new invariant — this was already existing, intentional behavior that needed to stay working.unitTests/resources/post-trailing-slash.test.js: new coverage for theRequestTargetinvariant plus both failure shapes from the issue, and regression tests confirming well-formed POST/GET (collection and non-collection) are unaffected.Investigation note
I could not reproduce the exact original crash on current
mainwith a bare table or with the real customer-facing pattern this issue names (super.post(data)dropping the target argument, fromtemplate-redirector-3.0.1.tgz'sVersionclass) —missingMethod()'s existing 405 already catches those cases cleanly. The underlyingRequestTargetstate bug is real and independently confirmed (verified theundefined-vs-well-formed asymmetry directly), and is the most plausible root cause given the issue's exact "undefined" vs "null" error-shape distinction between the two malformed-path variants. This fix closes that gap at the dispatch layer so it can't crash any current or future resource implementation, rather than relying onmissingMethod's incidental safety net.Review notes
Ran a cross-model review (Gemini via
agy, standard mode). It raised three concerns, all verified as false positives against the actual code (Gemini only sees the diff, not full file context):Table.tsdescribe-check change breaks collection reads — disproven:isSearchTarget()already routesisCollection: truetargets tosearch()before the describe check runs (now covered by an explicit regression test).400 Invalid primary keyvia existingcheckValidIdvalidation, unaffected by this change.options.methodcould be uppercase, bypassing the new guard — disproven:options.methodis a hardcoded lowercase literal in each static verb's own declaration (e.g.method: 'post'), not derived from the request's raw HTTP method casing.Refs #678
🤖 Generated with Claude Code