Skip to content

Add user defined data type support for SQL Server SSDT #779

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

Merged
merged 6 commits into from
Jan 8, 2023

Conversation

MMagueta
Copy link
Contributor

@MMagueta MMagueta commented Jan 7, 2023

Proposed Changes

Currently, SSDTs for SQL Server do not support user defined data types. This means if a simple custom type is defined, such as the one from below, the column does not get translated to F#.

A simple UDDT:

CREATE TYPE LATITUDE FROM REAL;
GO
CREATE
    RULE RuleLatitudeRange
    AS
    @per >= -180.0 AND @per <= 180.0
GO
EXEC SP_bindrule 'RuleLatitudeRange', 'LATITUDE'
GO

From the lsp:
Screenshot 2023-01-07 at 01 03 14

On the following table, where all the types are also simple overloads on REAL and INT:

CREATE TABLE dbo.TestUDDTs
(
    ID INT PRIMARY KEY,
    A MINUTES NOT NULL,
    B COMPASS NOT NULL,
    C LATITUDE NOT NULL,
    D LONGITUDE NOT NULL,
    E DAYS NOT NULL,
);

With this PR we now have:
Screenshot 2023-01-07 at 01 12 33

A further explanation on the implementation may be found under Further comments

Types of changes

Changes this PR introduces:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Build and tests pass locally
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Further comments

  • The solution is:
    • Parse the nodes in XML:
<Element Type="SqlUserDefinedDataType" Name="[dbo].[MINUTES]">
	<Relationship Name="Schema">
		<Entry>
			<References ExternalSource="BuiltIns" Name="[dbo]" />
		</Entry>
	</Relationship>
	<Relationship Name="Type">
		<Entry>
			<References ExternalSource="BuiltIns" Name="[int]" />
		</Entry>
	</Relationship>
</Element>
  • Generate a Map<UDDTName, UDDTInheritedType> (both algebraic aliases for string), where UDDTName is the given name to the type, and UDDTInheritedType is the base type that UDDTName inherits from
  • Add a new parameter to tryFindMappingOrVariant where such map is passed
  • Attempts to find the type in the normal typeMappingsByName map, if it does not find, attempts once again on the UDDT map, if something was found, searches for the base type in typeMappingsByName, otherwise defaults to SQL_VARIANT and consequently to System.Object
  • Since UDDTs can only be referenced to simple primitives in SQL Server, I leverage the fact I don't have to worry on UDDTs being defined in terms of another UDDT.


let ssdtTableToTable (tbl: SsdtTable) =
{ Schema = tbl.Schema ; Name = tbl.Name ; Type = if tbl.IsView then "view" else "base table" }

let ssdtColumnToColumn (tbl: SsdtTable) (col: SsdtColumn) =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Thorium, about this function, I was doubtful if I should remove the Option from it since I am unaware of the consequences it might lead into in the GetColumns member with the List.choose. In any case, I removed it in favor of the UDDT map

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this, @JordanMarr knows better. Maybe the idea was that totally unknown column types are just skipped by SQLProvider. I would expect that e.g. stored procedures could return something unknown that is then just treated as an obj.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an arbitrary decision on my part to omit the column instead of adding as a variant / obj.
Your change to call tryFindMappingOrVariant seems ok to me.

@JordanMarr
Copy link
Contributor

Nice. 👍

@MMagueta MMagueta marked this pull request as ready for review January 7, 2023 05:28
@MMagueta
Copy link
Contributor Author

MMagueta commented Jan 7, 2023

I have added a simple test, but I am having problems with running them locally on my machine. If someone can build them I would appreciate.

I tested using this script with a database I defined myself:

open FSharp.Data.Sql
open FSharp.Data.Sql.Common

let [<Literal>] vendor = DatabaseProviderTypes.MSSQLSERVER_SSDT
type Schema = SqlDataProvider<vendor, SsdtPath="/Users/mmagueta/Projects/FOSS/TestSQLProvider/base.dacpac">

type Config =
    { Host: string
      Port: string
      Database: string
      User: string
      Password: string }

[<EntryPoint>]
let main _ = 
    let config = {Host = "localhost"; Port = "1433"; Database = "msdb"; User = "sa"; Password = "P@ssw0rd"}
    let dataCtx =
        Schema.GetDataContext($"TrustServerCertificate = true; Server={config.Host},{config.Port}; Initial Catalog={config.Database}; User Id={config.User}; Password={config.Password}")
    let entity = dataCtx.Dbo.TestUddTs.Create()
    printfn "%A" <| ((entity.A.GetType().FullName) = "System.Int32")
    0

@MMagueta
Copy link
Contributor Author

MMagueta commented Jan 7, 2023

Anything I would still have to go through? If it's ok, I am done with the scope of this PR @JordanMarr

| Some tm -> tm
| None -> (typeMappingsByName.TryFind "SQL_VARIANT").Value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're in there, can you improve this line so that it doesn't call TryFind and Value (I was probably rushing):

| None -> typeMappingsByName["SQL_VARIANT"]

Copy link
Contributor Author

@MMagueta MMagueta Jan 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, the map should always have it at this point, so makes sense.

Changed it on this commit: f059201

@JordanMarr
Copy link
Contributor

I have submitted a PR to add my old SSDT unit test project back to the test sln.
If @Thorium wants to add this back, I think it could be very useful way to add some test coverage around the SSDT provider, including your changes.

@Thorium Thorium merged commit 5e1ce5e into fsprojects:master Jan 8, 2023
@Thorium
Copy link
Member

Thorium commented Jan 8, 2023

Thanks!

@Thorium
Copy link
Member

Thorium commented Jan 8, 2023

I'll release new version tomorrow.

@Thorium
Copy link
Member

Thorium commented Jan 11, 2023

Since UDDTs can only be referenced to simple primitives in SQL Server

You can do user-defined table type like

CREATE TYPE [F#].[Payment] AS TABLE(
	[Date] [smalldatetime] NULL,
	[Amount] [money] NULL
)

Although I'm not sure if SQL is a good tool for this kind of domain modelling as it's not meant to be a general purpose programming language.

@MMagueta
Copy link
Contributor Author

MMagueta commented Jan 11, 2023

Since UDDTs can only be referenced to simple primitives in SQL Server

You can do user-defined table type like

CREATE TYPE [F#].[Payment] AS TABLE(
	[Date] [smalldatetime] NULL,
	[Amount] [money] NULL
)

Although I'm not sure if SQL is a good tool for this kind of domain modelling as it's not meant to be a general purpose programming language.

This is somewhat useful for hidden behavior inside procedures (to group a query and insert somewhere from it), triggers (like error handling to persist a rollback), etc; but not so much to expose it via the provider. That's why I parsed a user defined data type only, UDTTs are not even being parsed on the XML from the dacpac.

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.

4 participants