feat: Json data type#1683
Conversation
|
@arthurschreiber @dhensby -- is there a plan to roll any updates out to Tedious in the near future? Noticed there has been no movement on the repo in 6+ months. I had a couple in-flight items including this PR and would like to do what I can to help if needed. Another one was and it seems a deprecation on Microsoft's side related to that as well: Generally it seems some maintenance things are accumulating without any regular interval releases from the Tedious team. Happy to help organize and move some things through for a release including reviewing other PR's if it's a matter of limited time to organize/test/review. |
|
👋🏻 Heya! You're right, I didn't have a lot of time to spend on tedious recently. If you don't mind, can you mention me on whatever issues are the most urgent in your eyes, and I'll promise to carve out some time to get them reviewed and merged. 🙇 |
|
@arthurschreiber you bet -- I'll make a pass through the open PR's & tickets over the next week and see what stands out. |
There was a problem hiding this comment.
Pull Request Overview
Adds support for a JSON data type by implementing its PLP encoding and registering it in the type registry.
- Introduce
src/data-types/json.tsto handle JSON validation, length resolution, and PLP data streaming. - Register the new
Jsontype insrc/data-type.ts, update the type lookup tables, and augment the documentation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/data-types/json.ts | New module defining the JSON type’s declaration, validation, and PLP encoding. |
| src/data-type.ts | Imports and registers Json in TYPE/TYPES and updates the docs table. |
| return parameter.value.length; | ||
| else return JSON.stringify(parameter.value).length; |
There was a problem hiding this comment.
Using .length on a string gives UTF-16 code-unit count, not the byte length for UTF-8. Switch to Buffer.byteLength(value, 'utf8') to get the correct byte length.
| return parameter.value.length; | |
| else return JSON.stringify(parameter.value).length; | |
| return Buffer.byteLength(parameter.value, 'utf8'); | |
| else return Buffer.byteLength(JSON.stringify(parameter.value), 'utf8'); |
There was a problem hiding this comment.
This also isn't accurate. To my knowledge this is sent by Tedious as nvarchar at serialization time -- the correct length is as indicated. @arthurschreiber thoughts?
|
Is there a plan to merge this PR? It would be highly appreciated my many users I believe |
|
Thanks for the work on this @matthewerwin, and apologies again for the long silence — I've gone through this in depth now. The core wire encoding for sending a Issues found, roughly by severity:
Plus some minor style alignment (braced conditionals, I'll start pushing to this branch shortly. Thanks again for doing the protocol digging — the SqlClient/MS-TDS references in the description made verifying this much easier. |
The unknown-length PLP header emitted by generateParameterLength obligates a terminator, but zero-length values returned without yielding one, producing a malformed TDS stream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Buffers previously took the JSON.stringify branch and were silently mangled into their internal representation, and values that stringify to undefined (functions, symbols) produced a confusing error from Buffer.from. Both now throw an explicit TypeError. Also drop resolveLength: it ran after validate() had already replaced the value with a Buffer, so it returned the length of the Buffer's JSON.stringify representation. Nothing consumes the length for this type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per MS-TDS, the json data type (0xF4) is enabled through the JSONSUPPORT feature extension (0x0D). Request version 1 in the LOGIN7 feature extension block, parse the server's acknowledgement, and track it as Connection#serverSupportsJson. Sending a TYPES.Json parameter to a server that did not acknowledge the feature now fails client-side with a descriptive RequestError instead of a cryptic protocol error from the server. Also add the read path: json TYPE_INFO carries no additional metadata (no length or collation bytes), and values arrive as PLP-encoded UTF-8 strings in ROW, NBCROW and RETURNVALUE tokens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unit coverage for the Json type's wire encoding (type info, PLP parameter length and data, validation), the JSON_SUPPORT feature extension acknowledgement, and parsing of json COLMETADATA and PLP-encoded json values in ROW and NBCROW tokens. Integration tests exercise both server capabilities: round-trips of string, object and null parameters plus json result columns on servers that acknowledge JSON_SUPPORT, and the client-side EJSONNOTSUPPORTED error on servers that do not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All of the above is now resolved on this branch:
CI status: lint, CodeQL and the unit suite pass. The One remaining caveat: the integration tests lint and typecheck, but I haven't been able to run them against a real SQL Server 2025 / Azure SQL instance. @matthewerwin since you have an Azure SQL setup that supports |
Matches how JavaScript itself names it (the global `JSON` object), which reads more natively than the PascalCase `Json`. The type has never been released, so this is not a breaking change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1683 +/- ##
==========================================
- Coverage 80.84% 80.08% -0.77%
==========================================
Files 90 91 +1
Lines 4887 4945 +58
Branches 924 935 +11
==========================================
+ Hits 3951 3960 +9
- Misses 640 696 +56
+ Partials 296 289 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@arthurschreiber thanks for the teamwork effort on this! I’ll have a closer look (and run all tests) when I return from Miami (Tues 7/21) but looks solid based on your write up. |
Tested that this is working with mssql against Azure Sql Server.
Notes:
I used the following logic to deduce the implementation:
I did not use the iconv-lite library b/c it seemed like unnecessary overhead vs the direct Buffer.from( ) support in Node.
This resolves this Feature Request I created earlier before deciding to address with a PR after a little digging.