test(auth): cover incrementOne/set against a mapped counter column by borisno2 · Pull Request #1543 · OpenSaasAU/stack

borisno2 · 2026-09-13T11:22:44Z

Summary

Extends the mapped Auth adapter test harness (packages/auth/tests/adapter-behaviour.test.ts) to remap the rate-limit list's count and lastRequest fields, not just key
Adds a test driving incrementOne's increment branch against the mapped count column, cross-checking the result against a fresh ORM-lane read
Adds a test driving the set branch against the mapped, bigint-typed lastRequest column, asserting the returned value narrows to a JS number the same way the ORM lane does
No production code changes were needed for this specific remap — I verified this is a genuine coverage gap, not a live bug in the case under test: I temporarily made codecFor's column resolution diverge from sqlWhere's (using the field key instead of the raw column) and confirmed the new tests fail with AuthAdapterError: ... exposes no column "count", then reverted. For the mapped columns this PR exercises (count→hit_count, lastRequest→last_seen), the existing outward()/getFieldAttributes machinery round-trips correctly off RETURNING, because better-auth's own getDefaultFieldName reverse lookup (by fieldName) resolves a mapped column name back to the right field key.
- Caveat found in review: that reverse lookup tries a direct hit against the model's own field keys first, so it isn't robust in general — a fields remap whose target string collides with a different field's own default key in the same model (e.g. { count: 'total_count', lastRequest: 'count' }) would misresolve. That's a narrow, pre-existing edge case in unchanged production code, out of scope for this coverage PR; tracked in Auth adapter: a fields remap colliding with another field's default key silently misresolves #1545.

Test plan

incrementOne covered against a mapped column
The set branch covered against a mapped column
Confirmed (via a temporary mutation) the new tests fail when codecFor's column resolution diverges from sqlWhere's
pnpm --filter @opensaas/stack-auth test — 511 passed, 81 skipped
pnpm lint
pnpm manypkg fix (no changes)
pnpm format (no changes)

🤖 Generated with Claude Code

https://claude.ai/code/session_01QyBL7H6hKQW5HzAkzp8FoY

Extends the mapped rate-limit harness to remap `count` and the bigint `lastRequest` column, then exercises incrementOne's increment and set branches against them, cross-checking with the ORM lane's read and asserting the bigint return narrows to a number the same way the ORM lane does. Confirmed (by temporarily diverging codecFor's column resolution) that these tests fail when incrementOne's column resolution diverges from sqlWhere's. Closes #1241 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QyBL7H6hKQW5HzAkzp8FoY

changeset-bot · 2026-09-13T11:22:48Z

🦋 Changeset detected

Latest commit: 15545b8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@opensaas/stack-auth	Patch
@opensaas/stack-cli	Patch
@opensaas/stack-core	Patch
@opensaas/stack-rag	Patch
@opensaas/stack-storage-s3	Patch
@opensaas/stack-storage-vercel	Patch
@opensaas/stack-storage	Patch
@opensaas/stack-tiptap	Patch
@opensaas/stack-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2026-09-13T11:22:48Z

Deployment failed for project stack-docs with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/open-saas?upgradeToPro=build-rate-limit

chatgpt-codex-connector · 2026-09-13T11:22:48Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

borisno2 · 2026-09-13T11:23:40Z

The failing Vercel status is the stack-docs preview deployment hitting the free-tier daily quota (api-deployments-free-per-day, "retry in 24 hours") — unrelated to this diff, which only touches packages/auth/tests/ and .changeset/. No fix to make here; it's a quota that resets on its own. GitHub Actions (test, e2e (postgres), e2e (dev-database), example-gates) are still running — I'll keep watching those.

Generated by Claude Code

borisno2 · 2026-09-13T11:30:07Z

Review (effort: medium)

Scope

Test-only change: extends packages/auth/tests/adapter-behaviour.test.ts to remap the rate-limit list's count/lastRequest (alongside the existing key remap) and adds two incrementOne cases — one driving the increment branch, one driving the set branch against the remapped int8 lastRequest column with an explicit typeof … === 'number' assertion. No production code changed; a patch changeset is included.

What checks out

count/lastRequest are genuine better-auth rateLimit field keys, and lastRequest is declared bigint: true upstream, so the new mappings do exercise a remapped int8 column as claimed.
incrementOne's two branches (increment: {...}, set: undefined and increment: {}, set: {...}) are both driven, matching what Auth adapter: no coverage for codecFor with a mapped increment/set column #1241 and the follow-up comment asked for.
Cross-checking incrementOne's own return against a fresh findOne read is a good touch — it catches a divergence between the RETURNING path and the ORM-lane read path, not just one or the other.
Distinct keys (ip:incr, ip:set) avoid collision with the existing ip:9 case in the same describe/beforeEach-truncated harness.
This closes the literal coverage gap in Auth adapter: no coverage for codecFor with a mapped increment/set column #1241, and the described verification method (temporarily diverging codecFor's column resolution from sqlWhere's and watching the new tests fail with AuthAdapterError) is legitimate evidence that these specific assertions are load-bearing rather than tautological.

On the "no live bug" claim — it needs qualifying, not retracting

Traced outward/isBigInt/resolveField/codecFor in packages/auth/src/adapter/index.ts and sqlWhere/applyOrmWhere in where.ts against the actual @better-auth/core source (get-default-field-name.ts, get-field-attributes.ts, get-field-name.ts, and the incrementOne wiring in factory.ts). Confirmed the mechanism the PR body describes is real: better-auth's own getFieldName/transformInput already rewrite increment's and set's keys to the mapped physical column names before our incrementOne ever sees them, and getDefaultFieldName reverse-resolves a column name back to the internal field key via Object.entries(fields).find(([_, f]) => f.fieldName === field) when a direct hit misses — which is exactly why resolveField/isBigInt/toColumn work correctly on column-keyed data despite being nominally field-key-keyed. So for the specific remap this PR adds (count→hit_count, lastRequest→last_seen), the claim holds and the new tests genuinely prove it.

The reasoning doesn't generalize as broadly as the PR body implies, though. getDefaultFieldName tries a direct hit first — schema[model].fields[field] — before falling back to the reverse lookup:

let f = schema[model]?.fields[field];
if (!f) {
  const result = Object.entries(schema[model]!.fields!).find(([_, f]) => f.fieldName === field);
  ...
}

If an app remaps one field's column to a string that happens to equal a different field's own (unmapped) default key in the same model — e.g. rateLimit: { fields: { count: 'total_count', lastRequest: 'count' } } — then the physical column "count" (now lastRequest's column) hits schema[model].fields['count'] directly and resolves to the original count field instead of lastRequest. resolveField/isBigInt would then check count's (non-bigint) attributes instead of lastRequest's, and incrementOne/outward would silently skip the BigInt widening/narrowing a real int8 column needs — a live, if narrow, bug in the existing (unchanged) production code, not something this PR introduces.

This isn't purely theoretical: nothing in derive-auth-lists.ts or the adapter validates that a fields remap's target string is disjoint from the model's own default field keys — I checked, there's no such check and no docblock/"Known limits" entry calling it out. It's also unexercised by both the old and new tests, since the mapped harness's column names (rate_key, hit_count, last_seen) don't collide with any of rateLimit's own default keys (key, count, lastRequest).

Suggested follow-up (not blocking this PR): file an issue to either (a) add a generate-time check in derive-auth-lists.ts rejecting a fields remap whose target equals another field's own default key in the same model, or (b) hold the field key resolved by sqlWhere's pre-adapter path forward explicitly instead of re-deriving it via getDefaultFieldName's ambiguous direct-hit-first logic. Either way, I'd soften the PR body's "verified this is a genuine coverage gap, not a live bug" to scope it to the specific remap under test — the general claim about getDefaultFieldName being "robust to being given either a field key or a mapped column name" isn't quite true in the presence of this naming collision.

Verdict

Approve on the diff as test-only coverage — it correctly closes #1241 and its acceptance criterion. Recommend filing the follow-up issue above given what tracing the resolution path turned up, and tightening the PR body's claim accordingly.

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01QyBL7H6hKQW5HzAkzp8FoY

Generated by Claude Code

borisno2 · 2026-09-13T11:31:12Z

Thanks for tracing that through — good catch on the direct-hit-first ordering in getDefaultFieldName. Filed #1545 to track the collision hazard, and tightened the PR description's claim to scope it to the specific remap this PR exercises rather than implying general robustness. Not fixing the collision hazard itself in this PR since it's a pre-existing, narrow edge case unrelated to #1241's scope.

Generated by Claude Code

github-actions · 2026-09-13T11:41:21Z