feat(paykit): resolve products by hash instead of latest version#124
Merged
feat(paykit): resolve products by hash instead of latest version#124
Conversation
PostgreSQL JSONB does not preserve key insertion order, so JSON.stringify on a value read back from the DB can differ from the original input. Sort keys before serialising to avoid false-positive provider syncs.
Subscribe and default-plan flows now look up the product version matching the current schema hash rather than always fetching the latest version. This eliminates the failure window between push and deploy where a hash mismatch would reject subscriptions. Adds docs on plan versioning and production push usage. Closes #103
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/paykit/src/customer/customer.service.ts (1)
162-164: Warn when a default plan is skipped.Lines 163-164 silently continue when the current default plan was never pushed for this hash. That leaves new customers without their expected default entitlements and gives operators no signal. A warning with
{ customerId, planId, hash }would make deploy drift much easier to diagnose.Possible improvement
const storedPlan = await getProductByHash(ctx.database, defaultPlan.id, defaultPlan.hash); if (!storedPlan) { + ctx.logger.warn( + { customerId, planId: defaultPlan.id, hash: defaultPlan.hash }, + "skipping default plan: no synced version matches the current schema", + ); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paykit/src/customer/customer.service.ts` around lines 162 - 164, When getProductByHash returns no storedPlan, add a warning log before continuing so operators can see which default plan was skipped; specifically, in the block after const storedPlan = await getProductByHash(ctx.database, defaultPlan.id, defaultPlan.hash); if (!storedPlan) { ... } emit a warning via the service's existing logger (e.g., ctx.logger.warn or similar) including { customerId: <current customer id variable>, planId: defaultPlan.id, hash: defaultPlan.hash } and then continue; keep the existing continue behavior but ensure the structured warning is logged to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paykit/src/product/product.service.ts`:
- Around line 93-100: getProductByHash currently uses
database.query.product.findFirst with where: and(eq(product.id, id),
eq(product.hash, hash)) which can return an arbitrary row when multiple (id,
hash) duplicates exist; change the query in getProductByHash to
deterministically return the newest version by adding an orderBy on
product.version descending (or fetch all matches and select the max version) so
the returned StoredProduct is always the latest version for that id+hash combo;
update the call site in getProductByHash (referencing PayKitDatabase,
database.query.product.findFirst, and product.version) to enforce this
deterministic selection.
In `@packages/paykit/src/subscription/subscription.service.ts`:
- Around line 76-85: The webhook currently re-reads the current plan hash during
checkout.completed which causes mismatches when plans change; modify
prepareSubscribeCheckoutCompleted and loadSubscribeContext so that when
initiating checkout you persist normalizedPlan.hash into checkout metadata
(paykit_plan_id and a new paykit_plan_hash metadata key) and then, in
prepareSubscribeCheckoutCompleted, read and use that persisted hash instead of
calling normalizedPlan.hash again; update any call sites that only stored
paykit_plan_id to include the plan hash and ensure
getProductByHash(ctx.database, input.planId, ...) is invoked with the stored
metadata hash to resolve the exact version used at checkout.
---
Nitpick comments:
In `@packages/paykit/src/customer/customer.service.ts`:
- Around line 162-164: When getProductByHash returns no storedPlan, add a
warning log before continuing so operators can see which default plan was
skipped; specifically, in the block after const storedPlan = await
getProductByHash(ctx.database, defaultPlan.id, defaultPlan.hash); if
(!storedPlan) { ... } emit a warning via the service's existing logger (e.g.,
ctx.logger.warn or similar) including { customerId: <current customer id
variable>, planId: defaultPlan.id, hash: defaultPlan.hash } and then continue;
keep the existing continue behavior but ensure the structured warning is logged
to aid debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b7f7a71-cee3-45f0-acc9-b084965e69f2
📒 Files selected for processing (7)
landing/content/docs/concepts/cli.mdxlanding/content/docs/concepts/database.mdxlanding/content/docs/get-started/installation.mdxpackages/paykit/src/core/errors.tspackages/paykit/src/customer/customer.service.tspackages/paykit/src/product/product.service.tspackages/paykit/src/subscription/subscription.service.ts
…ution When a checkout is created, the plan hash is now stored in Stripe metadata (paykit_plan_hash). On checkout.completed webhook, the persisted hash is used to resolve the exact product version from checkout time, preventing mismatches if the plan changed between checkout and webhook. Also adds resolveStoredPlanFeatures fallback so the webhook path can load features from the database when the plan is no longer in the code schema.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paykit/src/subscription/subscription.service.ts`:
- Around line 161-164: The code currently prefers normalizedPlan.includes
unconditionally which can cause entitlements to be built from a mismatched plan
version; update the logic that sets planFeatures so it uses
normalizedPlan.includes only when the normalizedPlan's hash/version matches the
resolved/stored plan (compare whatever identifier exists on normalizedPlan,
e.g., normalizedPlan.hash or normalizedPlan.planHash, against
storedPlan/internalId or input.planHash), otherwise call await
resolveStoredPlanFeatures(ctx.database, storedPlan.internalId); change the
ternary/conditional setting of planFeatures to include this equality check so
downstream entitlement creation (lines referencing planFeatures) always uses the
correct plan version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 52023d54-a7e2-4d19-95bd-c449a89fcd52
📒 Files selected for processing (3)
packages/paykit/src/product/product.service.tspackages/paykit/src/subscription/subscription.service.tspackages/paykit/src/subscription/subscription.types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/paykit/src/subscription/subscription.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/paykit/src/product/product.service.ts
Comment on lines
+161
to
+164
| const planFeatures = normalizedPlan | ||
| ? normalizedPlan.includes | ||
| : await resolveStoredPlanFeatures(ctx.database, storedPlan.internalId); | ||
|
|
There was a problem hiding this comment.
Critical: feature source can drift from resolved plan version.
When input.planHash resolves an older/newer stored plan, normalizedPlan may still exist for a different hash. Line 161 currently prefers normalizedPlan.includes unconditionally, so entitlements can be created from the wrong version (downstream at Line 1085 and Line 1130).
💡 Suggested fix
- const planFeatures = normalizedPlan
- ? normalizedPlan.includes
- : await resolveStoredPlanFeatures(ctx.database, storedPlan.internalId);
+ const planFeatures =
+ normalizedPlan && normalizedPlan.hash === storedPlan.hash
+ ? normalizedPlan.includes
+ : await resolveStoredPlanFeatures(ctx.database, storedPlan.internalId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paykit/src/subscription/subscription.service.ts` around lines 161 -
164, The code currently prefers normalizedPlan.includes unconditionally which
can cause entitlements to be built from a mismatched plan version; update the
logic that sets planFeatures so it uses normalizedPlan.includes only when the
normalizedPlan's hash/version matches the resolved/stored plan (compare whatever
identifier exists on normalizedPlan, e.g., normalizedPlan.hash or
normalizedPlan.planHash, against storedPlan/internalId or input.planHash),
otherwise call await resolveStoredPlanFeatures(ctx.database,
storedPlan.internalId); change the ternary/conditional setting of planFeatures
to include this equality check so downstream entitlement creation (lines
referencing planFeatures) always uses the correct plan version.
…metadata Simpler and more direct - stores the product's primary key in Stripe checkout metadata instead of the hash. Webhook resolution is now a single PK lookup instead of a compound query.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/paykit/src/subscription/subscription.service.ts (1)
312-333:⚠️ Potential issue | 🔴 CriticalDo not silently fall back when
paykit_product_internal_idis missing.Passing
undefinedhere sendsloadSubscribeContext()back to current-hash resolution, so anycheckout.completedevent missing the new metadata still resolves against the current schema instead of the version that created the checkout. That reintroduces the mismatch window this PR is trying to close. Safer options are to reject the webhook whenpaykit_product_internal_idis absent, or usecheckoutSubscription.providerPriceIdas the backward-compatible fallback instead ofnormalizedPlan.hash.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paykit/src/subscription/subscription.service.ts` around lines 312 - 333, The code silently falls back to undefined for paykit_product_internal_id which causes loadSubscribeContext to resolve against the current schema; instead, validate and fail fast or provide a safe fallback: when productInternalId is absent, either throw a BAD_REQUEST (reject the webhook) or pass checkoutSubscription.providerPriceId into loadSubscribeContext so it uses the provider price identifier for version resolution (do not use normalizedPlan.hash as the fallback); update the call sites using productInternalId and the loadSubscribeContext invocation accordingly (referencing paykit_product_internal_id, loadSubscribeContext, checkoutSubscription, checkoutSubscription.providerPriceId, and normalizedPlan.hash).
♻️ Duplicate comments (1)
packages/paykit/src/subscription/subscription.service.ts (1)
156-158:⚠️ Potential issue | 🔴 CriticalUse normalized features only when the resolved version matches.
productInternalIdcan intentionally resolve an older/newer stored product, but this still prefersnormalizedPlan.includeswhenever the plan exists in the current schema. That means Line 1080 and Line 1125 can still create entitlements from the wrong version.Suggested fix
- const planFeatures = normalizedPlan - ? normalizedPlan.includes - : await resolveStoredPlanFeatures(ctx.database, storedPlan.internalId); + const planFeatures = + normalizedPlan && normalizedPlan.hash === storedPlan.hash + ? normalizedPlan.includes + : await resolveStoredPlanFeatures(ctx.database, storedPlan.internalId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paykit/src/subscription/subscription.service.ts` around lines 156 - 158, The current logic uses normalizedPlan.includes whenever normalizedPlan exists even if it refers to a different stored plan version; update the selection so planFeatures uses normalizedPlan.includes only when the normalizedPlan actually corresponds to the resolved stored plan (e.g., compare normalizedPlan.internalId or version to storedPlan.internalId/version), otherwise call await resolveStoredPlanFeatures(ctx.database, storedPlan.internalId); update any downstream entitlement creation sites (referenced at the previous review as lines creating entitlements) to use this corrected planFeatures value so entitlements are created from the matched plan version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paykit/src/subscription/subscription.service.ts`:
- Around line 100-105: The code resolves matchingProduct from
input.productInternalId without verifying it belongs to the requested planId,
which can cause mismatched product vs plan usage in loadSubscribeContext; update
the branch that fetches by getProductByInternalId to validate that the returned
matchingProduct.id (or matchingProduct.internalPlanId equivalent) equals
input.planId (or the normalizedPlan.id/hash as appropriate) and if it does not,
treat it as a mismatch—either set matchingProduct to null and surface a clear
validation error or throw a BadRequest/ValidationError; ensure this check occurs
before calling withProviderInfo so providerId resolution uses a verified
product.
---
Outside diff comments:
In `@packages/paykit/src/subscription/subscription.service.ts`:
- Around line 312-333: The code silently falls back to undefined for
paykit_product_internal_id which causes loadSubscribeContext to resolve against
the current schema; instead, validate and fail fast or provide a safe fallback:
when productInternalId is absent, either throw a BAD_REQUEST (reject the
webhook) or pass checkoutSubscription.providerPriceId into loadSubscribeContext
so it uses the provider price identifier for version resolution (do not use
normalizedPlan.hash as the fallback); update the call sites using
productInternalId and the loadSubscribeContext invocation accordingly
(referencing paykit_product_internal_id, loadSubscribeContext,
checkoutSubscription, checkoutSubscription.providerPriceId, and
normalizedPlan.hash).
---
Duplicate comments:
In `@packages/paykit/src/subscription/subscription.service.ts`:
- Around line 156-158: The current logic uses normalizedPlan.includes whenever
normalizedPlan exists even if it refers to a different stored plan version;
update the selection so planFeatures uses normalizedPlan.includes only when the
normalizedPlan actually corresponds to the resolved stored plan (e.g., compare
normalizedPlan.internalId or version to storedPlan.internalId/version),
otherwise call await resolveStoredPlanFeatures(ctx.database,
storedPlan.internalId); update any downstream entitlement creation sites
(referenced at the previous review as lines creating entitlements) to use this
corrected planFeatures value so entitlements are created from the matched plan
version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c441213-41dc-4c2c-969d-6e5da7092d0b
📒 Files selected for processing (3)
packages/paykit/src/product/product.service.tspackages/paykit/src/subscription/subscription.service.tspackages/paykit/src/subscription/subscription.types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/paykit/src/subscription/subscription.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/paykit/src/product/product.service.ts
Comment on lines
+100
to
+105
| const matchingProduct = input.productInternalId | ||
| ? await getProductByInternalId(ctx.database, input.productInternalId) | ||
| : normalizedPlan | ||
| ? await getProductByHash(ctx.database, input.planId, normalizedPlan.hash) | ||
| : null; | ||
| const storedPlan = matchingProduct ? withProviderInfo(matchingProduct, providerId) : null; |
There was a problem hiding this comment.
Validate that productInternalId belongs to planId.
When productInternalId is present, this branch resolves the stored product without checking matchingProduct.id === input.planId. The rest of loadSubscribeContext() still uses input.planId/normalizedPlan, so a mismatched caller can resolve one stored product while pricing or entitling against another.
Suggested guard
const matchingProduct = input.productInternalId
? await getProductByInternalId(ctx.database, input.productInternalId)
: normalizedPlan
? await getProductByHash(ctx.database, input.planId, normalizedPlan.hash)
: null;
+ if (matchingProduct && matchingProduct.id !== input.planId) {
+ throw PayKitError.from(
+ "BAD_REQUEST",
+ PAYKIT_ERROR_CODES.PROVIDER_WEBHOOK_INVALID,
+ `Product "${matchingProduct.internalId}" does not belong to plan "${input.planId}"`,
+ );
+ }
const storedPlan = matchingProduct ? withProviderInfo(matchingProduct, providerId) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paykit/src/subscription/subscription.service.ts` around lines 100 -
105, The code resolves matchingProduct from input.productInternalId without
verifying it belongs to the requested planId, which can cause mismatched product
vs plan usage in loadSubscribeContext; update the branch that fetches by
getProductByInternalId to validate that the returned matchingProduct.id (or
matchingProduct.internalPlanId equivalent) equals input.planId (or the
normalizedPlan.id/hash as appropriate) and if it does not, treat it as a
mismatch—either set matchingProduct to null and surface a clear validation error
or throw a BadRequest/ValidationError; ensure this check occurs before calling
withProviderInfo so providerId resolution uses a verified product.
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
1 participant
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
pushand deploy where a hash mismatch would reject subscriptionspushusageTest plan
Closes #103
Summary by CodeRabbit
Documentation
Bug Fixes