azure-sdk-for-js icon indicating copy to clipboard operation
azure-sdk-for-js copied to clipboard

Fix 36840

Open azerum opened this issue 1 month ago • 2 comments

Packages impacted by this PR

@azure/cosmos

Issues associated with this PR

#36840

Describe the problem that is addressed by this PR

Some methods (described in the issue) used to throw non-Error objects, which makes debugging of such errors difficult to users, due to lack of stack traces

Fixed:

  • Replace isItemResourceValid() snippets with assertItemResourceValid(), that has the same logic, but instead of setting err.message, it throws new Error() with such message instead

  • Refactor the code of function to follow a bit easier. Replace 3 indexOf() with the equivalent regex check

  • Use return await withDiagnostics() in Items.create(), Items.upsert() and Item.replace() for better stack traces†

† - on better stack traces:

There is a difference between return asyncFn() and return await asyncFn() - it affect stack traces. Compare:

async function foo() {
  return goo()
}

async function goo() {
  // Make sure throwing happens after await
  await fetch('https://example.com')
  
  throw new Error()
}

void foo()

Giving output without foo in the stack trace:

file:///Users/user/src/issues/azure-cosmos-nodejs-unhandled-rejections/src/foo.js:9
  throw new Error()
        ^

Error
    at goo (file:///Users/user/src/issues/azure-cosmos-nodejs-unhandled-rejections/src/foo.js:9:9)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)

And

async function foo() {
  return await goo()
}

async function goo() {
  // Make sure throwing happens after await
  await fetch('https://example.com')
  
  throw new Error()
}

void foo()

Giving output with foo()

file:///Users/user/src/issues/azure-cosmos-nodejs-unhandled-rejections/src/foo.js:9
  throw new Error()
        ^

Error
    at goo (file:///Users/user/src/issues/azure-cosmos-nodejs-unhandled-rejections/src/foo.js:9:9)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    at async foo (file:///Users/user/src/issues/azure-cosmos-nodejs-unhandled-rejections/src/foo.js:2:10)

Node.js v22.18.0

You may want to find and replace all return withDiagnostics() with return await withDiagnostics(). Apart from stack traces, that must not change the behavior of the code

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Throwing Error seems like the most obvious and simple solution

Are there test cases added in this PR? (If not, why?)

No, the logic remained largely the same, the bugs are very unlikely

Provide a list of related PRs (if any)

N/A

Command used to generate this PR:**(Applicable only to SDK release request PRs)

N/A

Checklists

  • [x] Added impacted package name to the issue description
  • [x] Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • [ ] Added a changelog (if necessary) - not sure if it is

azerum avatar Dec 09 '25 18:12 azerum

Thank you for your contribution @azerum! We will review the pull request and get back to you soon.

github-actions[bot] avatar Dec 09 '25 18:12 github-actions[bot]

@microsoft-github-policy-service agree

azerum avatar Dec 09 '25 18:12 azerum