fix(commerce-server): return 400/404 from order-status instead of 500 by anam-godaddy · Pull Request #1488 · godaddy/javascript

anam-godaddy · 2026-10-01T01:37:26Z

What changed

GET /api/commerce/order-status returned 500 for every failure, so a bad order ID or a missing order looked the same as a credential or upstream outage. Its 500 body also echoed the internal error message.

Case	Before	After
Missing, blank, whitespace-padded, `.` or `..` `orderId`	500 (or 400 only when absent)	400
Orders API rejects the ID format (422 `{ code: 'VALIDATION_FAILED' }`)	500	400
Orders API reports the order missing (404 `{ code: 'NOT_FOUND' }`)	500	404
Returned order belongs to another store or channel	500	404 — doesn't reveal the order exists elsewhere
Any other 404 (unrouted path, wrong API base URL), a returned order with a different or non-string ID, incomplete store/channel binding, token failure, other upstream status	500 with `message`	500 `{ success: false, error: 'Failed to get order status' }`; detail logged with `console.error`

getOrderStatus() throws InvalidOrderIdError / OrderNotFoundError so in-process callers can branch without parsing messages. Check error.name rather than instanceof, which fails when a host has more than one copy of this package; the route itself matches by name. The 404 body is generic and never includes upstream content.
Error response bodies are cancelled when they aren't read, so they don't hold pooled connections while the status is polled.
ORDER_STATUS_UNKNOWN ('unknown') is exported so callers compare against a constant. status stays string: the Orders API's full paymentStatus set isn't documented here, and a closed union would mistype unlisted values.
Padded IDs are rejected (400), not trimmed — deliberately, so the lookup never targets an ID other than the one supplied. Previously ' abc' passed validation, was sent upstream untrimmed, and failed the binding check as a 500. Documented in the README and covered by tests.

Status semantics

status is the Orders REST API's statuses.paymentStatus (e.g. PAID, PENDING), or ORDER_STATUS_UNKNOWN when absent. This has been the behaviour since 5314b51 moved order lookup from the storefront GraphQL subgraph (which only returns draft orders and has no status) to REST; this PR doesn't change it. The README now also states that the status is display enrichment and callers must never present an unconfirmed payment as paid.

Why

Callers couldn't tell a bad or missing order ID from a credential or upstream outage, so they couldn't show the right message or alert on the right failure. The changeset is minor: this adds public exports and removes message from the 500 body, which hosts may read. On 0.x that is the breaking bump, so consumers on @godaddy/gd-commerce-server@^0.1.1 need their range bumped to pick it up.

Only a 404 with the Orders API's own NOT_FOUND code is mapped to "not found". From order-api's REST handler:

A missing order, or one in another store, is 404 { code: 'NOT_FOUND' } from its error handler (dataloaders/orders.ts, rest/middlewares/errorHandler.ts). Completed orders are returned, so a paid order is a 200, never a 404.
An unrouted path gets Express's default HTML 404 with no code; a wrong base URL or gateway returns its own 404. These are misconfiguration, so they stay a logged 500 instead of every lookup reporting "Order not found".
A wrong storeId fails order-api's authorization check with 403, not 404, and was already a logged 500.
An ID order-api can't decode (no Prefix_ part) is 422 { code: 'VALIDATION_FAILED' }, reported as 400. This package doesn't enforce the prefix itself.

Out of scope

The other 10 routes (catalog, cart, checkout) still return message in their 500 bodies. feat(commerce-server): standardize route error handling #1494 standardizes error handling across all routes, including this one, and builds on this PR.
draftOrder { statuses } on the checkout subgraph.

Testing

pnpm --filter @godaddy/gd-commerce-server typecheck, lint, test — 193 passed
Route tests for 400 (absent, blank, padded, ., .., repeated param — no upstream call; Orders API VALIDATION_FAILED), 404 (Orders API NOT_FOUND, store mismatch), and 500 (denied token, upstream 500, HTML 404 from an unrouted path, incomplete order, missing store binding, different order ID) asserting the exact generic body and that the detail is logged
Helper tests: 404s and 422s without the expected code (HTML, JSON without code, another code, non-JSON) are upstream failures; unread error bodies are cancelled; a plain Error named InvalidOrderIdError / OrderNotFoundError still maps to 400 / 404 at the route

🤖 Generated with Claude Code

Invalid order IDs (blank, padded, `.`, `..`) and missing orders previously surfaced as 500s, indistinguishable from credential or upstream failures. - getOrderStatus throws InvalidOrderIdError for invalid IDs and OrderNotFoundError when the Orders API returns 404 or the order is bound to another order ID, store, or channel. Incomplete responses stay generic. - The order-status route maps these to 400 and 404; everything else is 500. - Padded IDs are rejected rather than trimmed so the lookup never targets a different ID than the caller supplied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

changeset-bot · 2026-10-01T01:37:31Z

🦋 Changeset detected

Latest commit: c43083d

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

This PR includes changesets to release 1 package

Name	Type
@godaddy/gd-commerce-server	Minor

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

The 500 body echoed the internal error message, which can name upstream HTTP statuses, OAuth/token failures, or configuration problems. Return only { success: false, error } and log the detail with console.error. Export ORDER_STATUS_UNKNOWN so callers compare against a constant rather than the 'unknown' literal; status stays a string because the Orders API's full paymentStatus set is not documented here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

sbolinger-godaddy · 2026-10-05T23:37:37Z

I found one error-mapping issue worth fixing. No P0/P1 blockers found.

[P2] Keep incomplete bindings as upstream failures. In get-order-status.ts, lines 93–96, the completeness check only tests whether context exists. An upstream 200 containing { id: 'order-1', context: {} }, or a context missing just storeId or channelId, reaches the mismatch branch and becomes OrderNotFoundError. I reproduced all three cases through the built payments router: each returned 404 with Order not found. These are incomplete upstream responses, not confirmed orders belonging to another store/channel, so this tells callers the order is missing and hides the upstream failure. Check that both binding fields are present and valid before comparing them; incomplete bindings should retain the generic 500 and server-side logging. Add helper and route coverage for partially missing context fields.

Checks performed on commit 1201a91:

Read the full diff, repository guidance, and the router, configuration, OAuth, and order lookup call paths.
All 175 commerce-server tests passed on Node 24.20.0, including the local HTTP route tests for invalid IDs, missing orders, generic 500 bodies, and logging. Successful lookup tests also confirm payment-status passthrough and exclusion of private customer data.
Typecheck, Biome check, and the JS/declaration build passed. Ran the underlying package tools directly against an isolated copy of the PR, using existing workspace dependencies, because the local pnpm wrapper rejected linked dependency directories.
Exercised the incomplete-context cases above through the compiled router with mocked upstream responses. No live Orders API or browser verification was performed.

…lures An Orders API response whose context lacks storeId or channelId fell through to the store/channel mismatch check and was reported as 404. Require both binding fields before comparing them so malformed responses keep the generic 500 and server-side logging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

anam-godaddy · 2026-10-06T01:01:01Z

Thanks, good catch. Fixed in b238a33.

getOrderStatus now requires both context.storeId and context.channelId to be non-empty strings before the store/channel comparison. A response with context: {}, a missing storeId or channelId, or an empty storeId now throws the generic upstream error, so the route returns 500 and logs the detail server-side instead of returning 404.

Coverage added:

Helper: those four partial-context cases are in the incomplete-response it.each and are asserted not to be OrderNotFoundError.
Route: an order missing its store binding returns the generic 500 body and is logged.

Typecheck and Biome pass; 180 tests pass.

pbennett1-godaddy

Review notes inline. The most important are the bare-404 classification and the changeset bump level. The broader message-leak issue (other routes still return error.message in 500 bodies) looks like it's addressed by #1494.

pbennett1-godaddy · 2026-10-06T15:56:31Z

      cache: 'no-store',
    },
  );
+  if (response.status === 404) throw new OrderNotFoundError();


Any upstream 404 becomes OrderNotFoundError, including a 404 caused by a wrong storeId in the path or an apiBaseUrl that doesn't serve the Orders route. Every lookup would then report "Order not found" with nothing logged (the 404 branch skips console.error), so a misconfiguration looks like missing orders. Can we tell a real missing-order 404 apart (e.g. by body shape or error code) and treat anything else as an upstream failure?

Also: the response body isn't consumed or cancelled on this path or the !response.ok path. Under undici, unconsumed bodies can hold pooled connections while the order status is being polled. await response.body?.cancel() before throwing would avoid that.

Partly addressed in 456728b.

The body is now cancelled before throwing on every non-OK response, including 404. Tests assert ReadableStream.cancel is called for 404, 401, 403, and 500.

I left the 404 classification as is. The Orders API doesn't document an error code or body shape that separates a missing order from a wrong store ID or base URL, and guessing could turn real missing orders into 500s. Instead, upstream 404s are now logged with console.warn (including the configured store ID), so a misconfiguration shows up in logs instead of passing silently. If there's a documented not-found code we can key on, I'm happy to check for it.

Follow-up in c43083d (supersedes my reply above): the 404 is now classified by the Orders API's error body, after checking order-api's REST handler.

A missing order (or one in another store) is 404 { code: 'NOT_FOUND' } from its error handler. Only that maps to OrderNotFoundError.

Any other 404 (Express's HTML "Cannot GET" for an unrouted path, or a different base URL's 404) is now an upstream failure: logged 500. The console.warn is removed.

422 { code: 'VALIDATION_FAILED' }, which order-api returns when it can't decode the order ID, maps to InvalidOrderIdError (400) instead of 500.

A wrong storeId doesn't produce a 404 at all: order-api's authorization check returns 403, which already goes down the logged 500 path.

The 404/422 body is read to check code; other error bodies are still cancelled.

pbennett1-godaddy · 2026-10-06T15:56:31Z

-    throw new Error('Order lookup returned a different store or channel');
+  const context = order?.context;
+  // A binding missing either field is a malformed upstream response, not proof of another store or channel.
+  if (!order?.id || !isBindingId(context?.storeId) || !isBindingId(context?.channelId)) {


The store and channel IDs are type-checked with isBindingId, but order.id is only checked for truthiness. A non-string id (e.g. a number) passes here, then fails order.id !== orderId below and is reported as 404 instead of a malformed-response 500. Suggest !isBindingId(order?.id) for consistency.

Done in 456728b. order.id now goes through isBindingId, so a missing or non-string ID throws the generic error and becomes a 500. Both cases are in the incomplete-response it.each.

pbennett1-godaddy · 2026-10-06T15:56:31Z

+    throw new Error('Order lookup did not return the requested order');
+  }
+  // Report an order bound to another store or channel as missing so the route doesn't reveal it exists.
+  if (order.id !== orderId || context.storeId !== storeId || context.channelId !== channelId) {


Only the store/channel mismatch needs the existence-hiding 404. An order.id !== orderId mismatch means upstream returned the wrong record or a canonicalized ID, which is a malformed response. As written, a real (possibly paid) order is shown as not found with nothing logged. Could the id mismatch throw the generic error (500 + log) instead?

Agreed, done in 456728b. An order.id mismatch now throws the generic malformed-response error (500 + console.error). Only a store/channel mismatch returns 404. Added helper and route tests for the ID mismatch and removed it from the not-found cases.

pbennett1-godaddy · 2026-10-06T15:56:31Z

-      message: error instanceof Error ? error.message : String(error),
-    });
+    if (error instanceof InvalidOrderIdError) {
+      res.status(400).json(invalidOrderIdBody);


instanceof breaks if a host ends up with two copies of the package (nested node_modules, or one bundled and one external copy). A not-found order would then return 500. The constructors already set name, so checking error.name (or a brand symbol) would hold up better.

Done in 456728b. The route now matches by error.name instead of instanceof. I added a route test that throws a plain Error with each name, standing in for a different copy of the package, and asserts 400/404. The README and changeset tell in-process callers to check error.name too. I didn't add exported guard functions for now.

pbennett1-godaddy · 2026-10-06T15:56:31Z

 export default async function handler(req: Request, res: Response): Promise<void> {
  try {
    const { orderId } = req.query;
    if (!orderId || typeof orderId !== 'string') {


nit: getOrderStatus already validates the ID and throws InvalidOrderIdError, so there are two 400 paths to keep in sync. Only the non-string (array query value) case needs handling here. Consider narrowing to typeof orderId !== 'string' and delegating the rest.

Done in 456728b. The route only rejects non-string query values now; getOrderStatus handles blank, padded, . and ... The existing 400 cases still pass with no upstream call.

pbennett1-godaddy · 2026-10-06T15:56:31Z

@@ -0,0 +1,5 @@
+---
+'@godaddy/gd-commerce-server': patch


This adds three public exports and removes message from the 500 response body, which hosts may be reading. That seems like at least minor rather than patch.

Agreed, bumped to minor in 456728b (the package is 0.x, so that's the breaking bump).

pbennett1-godaddy · 2026-10-06T15:56:31Z

+      const result = await requestOrderStatus(`?orderId=${order.id}`);
+      expect(result).toEqual({ status: 500, body: { success: false, error: 'Failed to get order status' } });
+      expect(log).toHaveBeenCalledWith('order-status: failed to get order status', expect.any(Error));
+      log.mockRestore();


nit: if an assertion above fails, mockRestore() never runs and console.error stays mocked for the rest of the file. Move it to afterEach or a try/finally.

Done in 456728b. vi.restoreAllMocks() now runs in the top-level afterEach, and the inline mockRestore() is gone.

- Treat an upstream order ID mismatch or non-string ID as a malformed response (500 + log) instead of not found. - Cancel unread error response bodies and log upstream 404s. - Match route errors by name so they survive duplicate package copies. - Leave string ID validation to getOrderStatus. - Restore mocks in afterEach. - Bump the changeset to minor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ror code The Orders API reports a missing order as 404 with code NOT_FOUND and an undecodable order ID as 422 with code VALIDATION_FAILED. Only those map to OrderNotFoundError and InvalidOrderIdError; any other 404, such as an unrouted path or misconfigured base URL, is an upstream failure (logged 500). This replaces the console.warn on every upstream 404. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…rror-bodies Brings in godaddy#1488's review fixes and main. Conflicts resolved toward this branch's error types: - get-order-status: godaddy#1488's classification (Orders API NOT_FOUND -> 404, VALIDATION_FAILED -> 400, incomplete/mismatched order or any other 404 -> malformed) now throws UpstreamError (502) instead of a plain Error. - order-status GET: keeps commerceRoute, with godaddy#1488's narrowed `typeof orderId !== 'string'` check. The name-based error check and its "another copy" test are dropped; the route and lib share one module. - README and tests: godaddy#1488's cases expressed as 502/`code` bodies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

anam-godaddy and others added 2 commits October 5, 2026 12:20

Merge branch 'main' into fix/order-status-http-errors

1201a91

anam-godaddy mentioned this pull request Oct 5, 2026

feat(commerce-server): standardize route error handling #1494

Open

anam-godaddy marked this pull request as ready for review October 6, 2026 01:02

anam-godaddy requested a review from a team as a code owner October 6, 2026 01:02

pbennett1-godaddy reviewed Oct 6, 2026

View reviewed changes

scottopolis approved these changes Oct 6, 2026

View reviewed changes

anam-godaddy and others added 3 commits October 6, 2026 11:57

Merge branch 'main' into fix/order-status-http-errors

8be9490

anam-godaddy requested review from pbennett1-godaddy and scottopolis October 6, 2026 19:52

sbolinger-godaddy approved these changes Oct 6, 2026

View reviewed changes

Conversation

anam-godaddy commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Status semantics

Why

Out of scope

Testing

Uh oh!

changeset-bot Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

sbolinger-godaddy commented Oct 5, 2026

Uh oh!

anam-godaddy commented Oct 6, 2026

Uh oh!

pbennett1-godaddy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anam-godaddy Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anam-godaddy commented Oct 1, 2026 •

edited

Loading

changeset-bot Bot commented Oct 1, 2026 •

edited

Loading

anam-godaddy Oct 6, 2026 •

edited

Loading