Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matthewerwin
Copy link

@matthewerwin matthewerwin commented Mar 21, 2025

Tested that this is working with mssql against Azure Sql Server.

Notes:

  • validates JSON if parameter value is a string
  • encodes as JSON if parameter value is an object
  • passes 'null' to the server if parameter value is null
  • The inclusion of this new data type does not create any breaking changes to the codebase

I used the following logic to deduce the implementation:

  1. Referenced 0xF4 and PLP based on SqlClient
  2. That no parameter collation bytes are sent because Microsoft is very clear that JSON collation is fixed to Latin1_General_100_BIN2_UTF8 to match JSON Spec
  3. That the protocol only needed 0xf4 id prefix and no length bytes since its a newer data type which doesn't have a legacy <= 8000 implementation like varchar did (and the 'json' type has no length setting in SQL)
  4. That due to the fixed collation that UTF-8 encoding in validate() is sufficient

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.

@matthewerwin matthewerwin changed the title feature: Json data type feat: Json data type Mar 21, 2025
@matthewerwin
Copy link
Author

@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

tediousjs/node-mssql#1727

and it seems a deprecation on Microsoft's side related to that as well:
#1684

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.

@arthurschreiber
Copy link
Collaborator

👋🏻 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. 🙇

@matthewerwin
Copy link
Author

@arthurschreiber you bet -- I'll make a pass through the open PR's & tickets over the next week and see what stands out.

@arthurschreiber arthurschreiber requested a review from Copilot May 20, 2025 10:40
Copy link

@Copilot Copilot AI left a 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 in src/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'))
Copy link
Preview

Copilot AI May 20, 2025

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.

Suggested change
if ((parameter.value instanceof String || typeof (parameter.value) == 'string'))
if (typeof parameter.value === 'string')

Copilot uses AI. Check for mistakes.

Comment on lines +16 to +17
return parameter.value.length;
else return JSON.stringify(parameter.value).length;
Copy link
Preview

Copilot AI May 20, 2025

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.

Suggested change
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'))
Copy link
Preview

Copilot AI May 20, 2025

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.

Suggested change
if (!(value instanceof String || typeof (value) == 'string'))
if (typeof value !== 'string')

Copilot uses AI. Check for mistakes.

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

Successfully merging this pull request may close these issues.

2 participants