Skip to content

Conversation

@rjstanley
Copy link

@rjstanley rjstanley commented Jan 22, 2026

Summary

This PR enables tax providers to return metadata alongside tax lines, which workflows automatically store on the cart or order. This addresses a common limitation where tax providers need to store calculation references (e.g., calculation_id) for later use during order completion (e.g., committing tax transactions).

Use case: Tax providers like TaxJar, Avalara, and Numeral often perform a "calculation" during checkout that returns a calculation_id. During order completion, this calculation_id is used to "commit" the transaction with the tax service. Previously, providers had to use raw SQL to store this ID on the cart since they lacked access to the Cart module from within the ITaxProvider interface.

Changes

  • Add TaxLinesResult type that providers can return instead of just a tax lines array
  • Update ITaxProvider.getTaxLines return type to accept either array or TaxLinesResult
  • Update getItemTaxLinesStep to handle TaxLinesResult and pass sourceMetadata through
  • Update updateTaxLinesWorkflow to store sourceMetadata on cart via updateCartsStep
  • Update upsertTaxLinesWorkflow to store sourceMetadata on cart via updateCartsStep
  • Update updateOrderTaxLinesWorkflow to store sourceMetadata on order via updateOrdersStep

Example usage in a tax provider

async getTaxLines(itemLines, shippingLines, context) {
  const calculation = await this.taxClient.calculate(...)

  return {
    taxLines: [...],
    sourceMetadata: {
      tax_calculation_id: calculation.id
    }
  }
}

The workflow automatically merges this metadata into the cart/order's metadata field.

Backward compatibility

This change is fully backward compatible:

  • Tax providers can continue returning just an array of tax lines
  • The new TaxLinesResult type is optional and only used when providers need to store metadata

Test plan

  • Verify existing tax calculation workflows continue to work with providers returning just tax line arrays
  • Test that returning TaxLinesResult with sourceMetadata properly stores metadata on cart
  • Test that returning TaxLinesResult with sourceMetadata properly stores metadata on order
  • Verify metadata merging preserves existing cart/order metadata

@rjstanley rjstanley requested a review from a team as a code owner January 22, 2026 04:43
@changeset-bot
Copy link

changeset-bot bot commented Jan 22, 2026

🦋 Changeset detected

Latest commit: c7288d6

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

This PR includes changesets to release 74 packages
Name Type
@medusajs/types Patch
@medusajs/core-flows Patch
@medusajs/draft-order Patch
@medusajs/framework Patch
@medusajs/js-sdk Patch
@medusajs/modules-sdk Patch
@medusajs/orchestration Patch
@medusajs/utils Patch
@medusajs/workflows-sdk Patch
@medusajs/medusa-oas-cli Patch
@medusajs/admin-bundler Patch
@medusajs/dashboard Patch
integration-tests-http Patch
@medusajs/medusa Patch
@medusajs/test-utils Patch
@medusajs/analytics Patch
@medusajs/api-key Patch
@medusajs/auth Patch
@medusajs/cache-inmemory Patch
@medusajs/cache-redis Patch
@medusajs/caching Patch
@medusajs/cart Patch
@medusajs/currency Patch
@medusajs/customer Patch
@medusajs/event-bus-local Patch
@medusajs/event-bus-redis Patch
@medusajs/file Patch
@medusajs/fulfillment Patch
@medusajs/index Patch
@medusajs/inventory Patch
@medusajs/link-modules Patch
@medusajs/locking Patch
@medusajs/notification Patch
@medusajs/order Patch
@medusajs/payment Patch
@medusajs/pricing Patch
@medusajs/product Patch
@medusajs/promotion Patch
@medusajs/region Patch
@medusajs/sales-channel Patch
@medusajs/settings Patch
@medusajs/stock-location Patch
@medusajs/store Patch
@medusajs/tax Patch
@medusajs/user Patch
@medusajs/workflow-engine-inmemory Patch
@medusajs/workflow-engine-redis Patch
@medusajs/analytics-local Patch
@medusajs/analytics-posthog Patch
@medusajs/auth-emailpass Patch
@medusajs/auth-github Patch
@medusajs/auth-google Patch
@medusajs/caching-redis Patch
@medusajs/file-local Patch
@medusajs/file-s3 Patch
@medusajs/fulfillment-manual Patch
@medusajs/locking-postgres Patch
@medusajs/locking-redis Patch
@medusajs/notification-local Patch
@medusajs/notification-sendgrid Patch
@medusajs/payment-stripe Patch
@medusajs/oas-github-ci Patch
@medusajs/cli Patch
@medusajs/deps Patch
@medusajs/telemetry Patch
@medusajs/admin-sdk Patch
@medusajs/admin-shared Patch
@medusajs/admin-vite-plugin Patch
@medusajs/icons Patch
@medusajs/toolbox Patch
@medusajs/ui-preset Patch
create-medusa-app Patch
medusa-dev-cli Patch
@medusajs/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
Copy link

vercel bot commented Jan 22, 2026

@rjstanley is attempting to deploy a commit to the medusajs Team on Vercel.

A member of the Team first needs to authorize it.

…rt/order storage

This change enables tax providers to return metadata alongside tax lines,
which workflows automatically store on the cart or order. This is useful
for tax providers that need to store calculation references (e.g.,
calculation_id) for later use during order completion.

Changes:
- Add TaxLinesResult type that providers can return instead of just tax lines array
- Update ITaxProvider.getTaxLines return type to accept either array or TaxLinesResult
- Update getItemTaxLinesStep to handle TaxLinesResult and pass sourceMetadata through
- Update updateTaxLinesWorkflow to store sourceMetadata on cart via updateCartsStep
- Update upsertTaxLinesWorkflow to store sourceMetadata on cart via updateCartsStep
- Update updateOrderTaxLinesWorkflow to store sourceMetadata on order via updateOrdersStep
@rjstanley rjstanley force-pushed the feat/tax-provider-query-access branch from 82b609c to c7288d6 Compare January 22, 2026 07:11
@rjstanley rjstanley changed the title feat(modules-sdk,types,core-flows): enable Query service access for module providers feat(types,core-flows): Allow tax providers to return metadata for cart/order storage Jan 22, 2026
@NicolasGorga
Copy link
Contributor

@claude review this

@claude
Copy link

claude bot commented Jan 22, 2026

Pull Request Review - #14605

Summary

This PR adds a valuable feature allowing tax providers to return metadata alongside tax lines, which is automatically stored on carts/orders. The implementation is well-designed and maintains backward compatibility.


✅ Strengths

1. Clean Architecture

  • ✅ Excellent use of union return types (TaxLinesResult | array) for backward compatibility
  • ✅ Type-safe implementation with proper TypeScript discriminated union checking via isTaxLinesResult() helper
  • ✅ Consistent pattern across all three workflows (cart update, cart upsert, order update)
  • ✅ Follows the codebase's workflow pattern conventions correctly

2. Backward Compatibility

  • ✅ Existing providers can continue returning arrays without any changes
  • ✅ Type system gracefully handles both old and new return formats
  • ✅ No breaking changes to the ITaxProvider interface

3. Implementation Quality

  • ✅ Proper use of when() conditional workflow steps to only update metadata when present
  • ✅ Metadata merging preserves existing cart/order metadata using spread operators
  • ✅ Correct use of transform() for data preparation in workflows
  • ✅ Adheres to Medusa's code style (no semicolons, double quotes, proper formatting)

⚠️ Issues & Recommendations

1. CRITICAL: Missing Return Type Update 🔴

File: packages/modules/tax/src/services/tax-provider.ts:46-54

The TaxProviderService.getTaxLines() method's return type is not updated to reflect the new TaxLinesResult possibility:

// Current - INCORRECT ❌
async getTaxLines(
  providerId: string,
  itemLines: TaxTypes.ItemTaxCalculationLine[],
  shippingLines: TaxTypes.ShippingTaxCalculationLine[],
  context: TaxTypes.TaxCalculationContext
): Promise<(TaxTypes.ItemTaxLineDTO | TaxTypes.ShippingTaxLineDTO)[]> {
  const provider = this.retrieveProvider(providerId)
  return provider.getTaxLines(itemLines, shippingLines, context)
}

This creates a type mismatch since ITaxProvider.getTaxLines now returns Promise<(ItemTaxLineDTO | ShippingTaxLineDTO)[] | TaxLinesResult>, but this service method promises to return only the array form. TypeScript may not catch this if strict type checking isn't enforced on the return statement.

Recommendation:

async getTaxLines(
  providerId: string,
  itemLines: TaxTypes.ItemTaxCalculationLine[],
  shippingLines: TaxTypes.ShippingTaxCalculationLine[],
  context: TaxTypes.TaxCalculationContext
): Promise<(TaxTypes.ItemTaxLineDTO | TaxTypes.ShippingTaxLineDTO)[] | TaxTypes.TaxLinesResult> {
  const provider = this.retrieveProvider(providerId)
  return provider.getTaxLines(itemLines, shippingLines, context)
}

2. Potential Bug: Metadata Collision ⚠️

Files:

  • packages/core/core-flows/src/cart/workflows/update-tax-lines.ts:169-185
  • packages/core/core-flows/src/cart/workflows/upsert-tax-lines.ts:162-176
  • packages/core/core-flows/src/order/workflows/update-tax-lines.ts:256-271

When tax providers return metadata for both item lines AND shipping lines separately, there's a metadata collision risk:

// In getItemTaxLinesStep
if (items.length) {
  const itemTaxResult = await taxService.getTaxLines(...)
  if (itemTaxResult.sourceMetadata) {
    collectedMetadata = { ...collectedMetadata, ...itemTaxResult.sourceMetadata }
  }
}

if (shippingMethods.length) {
  const shippingTaxResult = await taxService.getTaxLines(...)
  if (shippingTaxResult.sourceMetadata) {
    collectedMetadata = { ...collectedMetadata, ...shippingTaxResult.sourceMetadata }
  }
}

If both calls return a key like calculation_id, the shipping metadata will silently overwrite the item metadata.

Recommendations:

  1. Document the behavior - Add a comment explaining that shipping metadata takes precedence if keys collide
  2. Consider namespacing - Use prefixed keys like item_calculation_id and shipping_calculation_id
  3. Add validation - Detect collisions and either merge arrays or throw a descriptive error

Example improvement:

if (shippingTaxResult.sourceMetadata) {
  Object.keys(shippingTaxResult.sourceMetadata).forEach(key => {
    if (collectedMetadata[key] \!== undefined) {
      // Log warning or namespace the keys
      console.warn(`Metadata key '${key}' exists in both item and shipping metadata`)
    }
  })
  collectedMetadata = { ...collectedMetadata, ...shippingTaxResult.sourceMetadata }
}

3. Missing Test Coverage ⚠️

The PR lacks integration tests for the new functionality. Based on the test plan in the PR description, tests should verify:

Required test cases:

  • ✅ Backward compatibility: Providers returning arrays still work
  • Missing: Provider returning TaxLinesResult with sourceMetadata for cart workflows
  • Missing: Provider returning TaxLinesResult with sourceMetadata for order workflows
  • Missing: Metadata merging preserves existing cart/order metadata
  • Missing: Empty/undefined sourceMetadata doesn't break workflows
  • Missing: Both item and shipping metadata are correctly collected

Recommendation: Add integration tests to integration-tests/modules/__tests__/tax/workflow/tax.spec.ts or create a new test file specifically for this feature.

Example test structure:

it("should store tax provider metadata on cart", async () => {
  // Setup: Create a mock tax provider that returns TaxLinesResult with metadata
  // Execute: Run updateTaxLinesWorkflow  
  // Assert: Verify cart metadata contains the sourceMetadata
})

4. Security Consideration: Metadata Injection 🔒

Impact: Low to Medium (depends on downstream usage)

Tax provider metadata is directly merged into cart/order metadata without validation. Malicious or buggy tax providers could:

  • Overwrite critical metadata fields
  • Inject large amounts of data
  • Store sensitive information inappropriately

Recommendations:

  1. Validate metadata keys - Ensure keys follow a safe pattern (e.g., must start with tax_)
  2. Size limits - Enforce a maximum size for sourceMetadata
  3. Reserved keys - Document/prevent overwriting reserved metadata fields
  4. Type validation - Ensure values are serializable and safe

Example:

const ALLOWED_METADATA_PREFIX = 'tax_'
const MAX_METADATA_SIZE = 1024 * 10 // 10KB

function validateTaxMetadata(metadata: Record<string, unknown>) {
  Object.keys(metadata).forEach(key => {
    if (\!key.startsWith(ALLOWED_METADATA_PREFIX)) {
      throw new MedusaError(
        MedusaError.Types.INVALID_DATA,
        `Tax provider metadata keys must start with '${ALLOWED_METADATA_PREFIX}'`
      )
    }
  })
  
  if (JSON.stringify(metadata).length > MAX_METADATA_SIZE) {
    throw new MedusaError(
      MedusaError.Types.INVALID_DATA,
      'Tax provider metadata exceeds maximum size'
    )
  }
}

5. Documentation Improvements 📝

The JSDoc for TaxLinesResult.sourceMetadata is good, but could be enhanced:

Current:

/**
 * Optional metadata to be stored on the source cart or order.
 * This is useful for storing tax calculation references...
 */
sourceMetadata?: Record<string, unknown>

Suggested enhancement:

/**
 * Optional metadata to be stored on the source cart or order.
 * This is useful for storing tax calculation references (e.g., calculation_id)
 * that need to be used later during order completion (e.g., committing transactions).
 * 
 * **Important:** 
 * - Metadata is merged into existing cart/order metadata
 * - If both item and shipping calculations return metadata with the same keys,
 *   the shipping metadata will take precedence
 * - Keys should be namespaced to avoid collisions (e.g., 'taxjar_calculation_id')
 * 
 * @example
 * ```ts
 * return {
 *   taxLines: [...],
 *   sourceMetadata: {
 *     taxjar_calculation_id: "calc_123",
 *     taxjar_calculation_date: "2025-01-22"
 *   }
 * }
 * ```
 */
sourceMetadata?: Record<string, unknown>

🧪 Testing Recommendations

  1. Add integration tests for new functionality (see issue Common query patterns #3 above)
  2. Test edge cases:
    • Empty sourceMetadata: {}
    • sourceMetadata: undefined
    • Very large metadata objects
    • Special characters in metadata keys/values
    • Concurrent tax calculations updating the same cart
  3. Test backward compatibility with existing tax providers

📊 Performance Considerations

No significant performance concerns:

  • The when() conditional prevents unnecessary updates when no metadata is present
  • Metadata merging is a shallow operation
  • Type checking via isTaxLinesResult() is O(1)

🎯 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5) - Well-architected with minor issues
Backward Compatibility: ⭐⭐⭐⭐⭐ (5/5) - Excellent
Security: ⭐⭐⭐ (3/5) - Needs validation
Test Coverage: ⭐⭐ (2/5) - Missing tests

Verdict: Approve with Required Changes 🟡

This is a valuable feature with clean implementation, but requires:

  1. MUST FIX: Update TaxProviderService.getTaxLines() return type
  2. SHOULD FIX: Add integration tests
  3. SHOULD CONSIDER: Metadata validation and collision handling

Great work on this PR! The feature design is solid and will be very useful for tax provider integrations. Once the return type is fixed and tests are added, this will be ready to merge. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants