Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: provide better error messages when input param is invalid #1603

Conversation

jtomaszewski
Copy link
Contributor

@jtomaszewski jtomaszewski commented Feb 15, 2024

Currently when I run a DB procedure that requires a TVP input that has many columns and rows, if at least one value of that is incorrect (e.g. fails with a The number NaN cannot be converted to a BigInt because it is not an integer error), I only get that error message - without highlighting which param value is the faulty one. It looks like this:

RequestError: The number NaN cannot be converted to a BigInt because it is not an integer
  File "/home/site/wwwroot/node_modules/mssql/lib/tedious/request.js", line 811, col 19, in Request.userCallback
    err = new RequestError(err, 'EREQUEST')
  File "/home/site/wwwroot/node_modules/tedious/lib/request.js", line 239, col 14, in Request.callback
    this.userCallback(err, rowCount, rows);
  File "/home/site/wwwroot/node_modules/tedious/lib/connection.js", line 2623, col 22, in Parser.onEndOfMessage
    sqlRequest.callback(sqlRequest.error, sqlRequest.rowCount, sqlRequest.rows);
  File "node:events", line 628, col 28, in Object.onceWrapper
  File "node:events", line 514, col 28, in Parser.emit
  File "node:domain", line 489, col 12, in Parser.emit
  File "/home/site/wwwroot/node_modules/tedious/lib/token/token-stream-parser.js", line 24, col 12, in Readable.<anonymous>
    this.emit('end');
  File "node:events", line 514, col 28, in Readable.emit
  File "node:domain", line 489, col 12, in Readable.emit
  File "node:internal/streams/readable", line 1359, col 12, in endReadableNT
  File "node:internal/process/task_queues", line 82, col 21, in process.processTicksAndRejections

Finding the cause is very difficult then, I basically have to check every value 1-by-1 manually to see which one is wrong... or put a debugger into the tedious source code.

This PR a bit improves that behaviour. If an error is thrown during the generateParameterData call, the error is wrapped, and its' message is kept, but also enhanced with information about which param is the wrong one.

P.S. I can add some tests or change the implementation if you want, just let me know if you're okay with the general idea of doing this.

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.28%. Comparing base (3856dc5) to head (87dbc1a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1603      +/-   ##
==========================================
- Coverage   78.97%   78.28%   -0.70%     
==========================================
  Files          93       93              
  Lines        4871     4876       +5     
  Branches      937      937              
==========================================
- Hits         3847     3817      -30     
- Misses        718      761      +43     
+ Partials      306      298       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelSun90
Copy link
Contributor

Hi @jtomaszewski, sorry for the long silence period on this PR. I have worked with @arthurschreiber , and made some change to this PR, and fix the failing tests. Please give a look, see if you ok with the changes.
Hi @arthurschreiber, I found a way to get the change in here, not s smart way, but I think it works. Please take a look, see if this one is ok to be merged.

@jtomaszewski
Copy link
Contributor Author

Awesome work, it looks great to me :)

@MichaelSun90 MichaelSun90 merged commit 209b9f3 into tediousjs:master Jul 31, 2024
26 of 27 checks passed
Copy link

🎉 This PR is included in version 18.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants