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

API Proposal: SqlJson class design to support JSON datatype #2665

Closed
apoorvdeshmukh opened this issue Jul 11, 2024 · 12 comments
Closed

API Proposal: SqlJson class design to support JSON datatype #2665

apoorvdeshmukh opened this issue Jul 11, 2024 · 12 comments
Assignees
Labels
Area\Json Issues that are targeted for the Json feature in the driver. 💡 Enhancement Issues that are feature requests for the drivers we maintain.

Comments

@apoorvdeshmukh
Copy link
Contributor

apoorvdeshmukh commented Jul 11, 2024

Background:
As part of enhancing support in SqlClient for JSON datatype for Sql Server as mentioned in #2622, SqlDbType needed to be enhanced with enum called Json and introduction of a SqlType class for JSON type was needed.
An enum called Json with value 35 in SqlDbType was recently added in the runtime repo through issue #103925
The next follow up item is to provide a SqlType class to work with the JSON data. This proposal talks about the design of SqlJson class which aims to represent the JSON data stored in or retrieved from a server.

API Proposal

using System;
using System.Data.SqlTypes;
using System.Text.Json;

namespace Microsoft.Data.SqlTypes
{
    /// <summary>
    /// Represents the Json Data type in SQL Server.
    /// </summary>
    public class SqlJson : INullable
    {
        /// <summary>
        /// Parameterless constructor. Initializes a new instance of the SqlJson class which 
        /// represents a null JSON value.
        /// </summary>
        public SqlJson() { }

        /// <summary>
        /// Takes a <see cref="string"/> as input and initializes a new instance of the SqlJson class.
        /// </summary>
        /// <param name="jsonString"></param>
        public SqlJson(string jsonString) { }

        /// <summary>
        /// Takes a <see cref="JsonDocument"/> as input and initializes a new instance of the SqlJson class.
        /// </summary>
        /// <param name="jsonDoc"></param>
        public SqlJson(JsonDocument jsonDoc) { }


        /// <inheritdoc/>
        public bool IsNull => throw null;

        /// <summary>
        /// Represents a null instance of the <see cref="SqlJson"/> type.
        /// </summary>
        public static SqlJson Null { get { throw null; } }

        /// <summary>
        /// Gets the string representation of the Json content of this <see cref="SqlJson" /> instance.
        /// </summary>
        public string Value { get ; }
    }
}

Additionally, SqlDataReader will be enhanced with addition of API for returning SqlJson type

        /// <summary>
        /// Gets the value of the specified column as JSON value.
        /// </summary>
        virtual public SqlJson GetSqlJson(int i)
@apoorvdeshmukh apoorvdeshmukh added the Area\ClientX Issues that are targeted for ClientX codebase. label Jul 11, 2024
@apoorvdeshmukh apoorvdeshmukh self-assigned this Jul 11, 2024
@apoorvdeshmukh
Copy link
Contributor Author

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 11, 2024

Assume this will be .NET 8+ only, or??

@apoorvdeshmukh
Copy link
Contributor Author

apoorvdeshmukh commented Jul 11, 2024

Assume this will be .NET 8+ only, or??

We will support netfx and all the runtime versions.

@roji
Copy link
Member

roji commented Jul 11, 2024

.NET 9 and above

Just pointing out that SqlDbType.Json isn't necessary for making read work, and and should also not be necessary for making write work with JsonDocument. Even for writing strings as JSON, there's the possibility of the user hardcodIng the numeric enum value - this is ugly and hacky, but it would allow people to use JSON without forcing them to upgrade to .net 9. So I'm just wondering if the plan is to only enable JSON support for 9.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 11, 2024

@roji Reply was edited 🥳

@saurabh500
Copy link
Contributor

The namespace should be Microsoft.Data.SqlTypes

@saurabh500
Copy link
Contributor

@David-Engel what is the guidance about the docs for public APIs?
Do we create an XML file like it exists for other public APIs today? Also, I see that the docs when I hover over in VS, is broken.
The related issue is here. #2013
So for the docs of this new type what should be done ? Do you create the docs for this in a more compliant way (not sure what the technical term is)

@saurabh500 saurabh500 added 💡 Enhancement Issues that are feature requests for the drivers we maintain. and removed Area\ClientX Issues that are targeted for ClientX codebase. labels Jul 11, 2024
@JRahnama
Copy link
Contributor

@saurabh500 I have been working on converting the docs to a more modern style to address issue #2013. However, this is a big job, and it will take some more time for me to complete as I am doing it alongside my scheduled tasks. When it is done, I need to request a sample published test from the documentation team to ensure everything works as expected, so that will take some time.

So far, I have noticed a few issues with the documentation contents inside the CDATA block, which have not been marked by the compiler as it reads them as a text block.

So, I think for now we need to continue the old style as the job needs to be done in a separate PR, but I will wait for @David-Engel 's comment on this.

@saurabh500
Copy link
Contributor

Thanks @JRahnama for the inputs.

@JRahnama
Copy link
Contributor

So for the docs of this new type what should be done ? Do you create the docs for this in a more compliant way (not sure what the technical term is)

To add more on this, I have found out below:

  1. all tables and lists needs to be converted to (this is in progress)
  2. All CDATA need to be converted to sections (this part is done in my work).
  3. all xrefs needs to be converted to , this will need more consideration as it does not accept %2A at the end of the link and each overload needs to be specified. For example <see cref="T:Microsoft.Data.SqlClient.SqlCommand()" /> will link to default constructor and <see cref=" Microsoft.Data.SqlClient.SqlCommand(string, SqlConnection)" /> will link to other constructor.
  4. For Note and Caution sections I need to test to see if we can use SandCastle xml

this is how it will look after changes are done:
image

image

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 16, 2024

We should have a stream constructor so that people don't have to go through string to load data.

@saurabh500
Copy link
Contributor

@Wraith2 For the SqlTypes i.e. SqlJson we wont be adding streaming support.

This is because these types need to validate the datatype before sending it out to the server. For JSON, the stream will need to be parsed till the end to validate if the JSON is valid, and will diminish the value of stream on SqlJson

However streaming is needed with SqlParameter.Value when SqlDbType=Json in accordance with this article
https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/sqlclient-streaming-support

Also this will be interesting when we use SqlBulkCopy to stream Json data from one Sql server to another.

@saurabh500 saurabh500 self-assigned this Aug 6, 2024
@apoorvdeshmukh apoorvdeshmukh added the Area\Json Issues that are targeted for the Json feature in the driver. label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area\Json Issues that are targeted for the Json feature in the driver. 💡 Enhancement Issues that are feature requests for the drivers we maintain.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants