-
Notifications
You must be signed in to change notification settings - Fork 438
feat: Json data type #1683
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
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.ts
to handle JSON validation, length resolution, and PLP data streaming. - Register the new
Json
type 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 'json'; | ||
}, | ||
resolveLength: (parameter) => { | ||
if ((parameter.value instanceof String || typeof (parameter.value) == 'string')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using instanceof String
is uncommon and only matches String objects, not primitives. You can simplify to typeof parameter.value === 'string'
to cover all string inputs.
if ((parameter.value instanceof String || typeof (parameter.value) == 'string')) | |
if (typeof parameter.value === 'string') |
Copilot uses AI. Check for mistakes.
return parameter.value.length; | ||
else return JSON.stringify(parameter.value).length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'); |
Copilot uses AI. Check for mistakes.
return null; | ||
} | ||
|
||
if (!(value instanceof String || typeof (value) == 'string')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above, prefer typeof value === 'string'
instead of instanceof String
to reliably detect primitive strings.
if (!(value instanceof String || typeof (value) == 'string')) | |
if (typeof value !== 'string') |
Copilot uses AI. Check for mistakes.
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.