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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add provider specific column types #10
Add provider specific column types #10
Conversation
@@ -32,4 +32,8 @@ | |||
<PackageReference Include="SqlKata" Version="2.3.7" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="..\SqlHydra\SqlHydra.fsproj" /> |
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.
This might be a wrong thing to do, I needed to reference the attribute I created inside SqlHydra
. Does it mean the nuget package will get a dependency to SqlHydra
?
Also had to change the target framework to be compatible with that project. Is it on purpose that this project is targeting netstandard2.0
to be compatible with the older versions of dotnet? Does it make sense to create a shared project that will be referenced from both projects instead?
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.
Yes, SqlHydra.Query
is netstandard2.0
to make it compatible with more project types. Since SqlHydra.Query
doesn't use any new features, it would be nice to allow greater compatibility for a little while longer if possible.
I think you should be able to:
- Change
SqlHydra
project to usenetstandard2.0
so that it can be used inSqlHydra.Query
- Add the following line into the
SqlHydra.Query
project so that it will copy theSqlHydra
dll to nuget (this has already been done in the provider projects that referenceSqlHydra
.):
<TargetsForTfmSpecificBuildOutput>$(TargetsForTfmSpecificBuildOutput);CopyProjectReferencesToPackage</TargetsForTfmSpecificBuildOutput>
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.
It seems like SqlHydra.fsproj
references package Microsoft.Build
version 16.11.0 which gives warning that it may not be fully compatible with netstandard 2.0
. Not sure whether this can seriously break things somewhere.
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.
SqlHydra.Query
really doesn't need to reference Microsoft.Build
or anything like that since most of that stuff is only used by the generator projects.
So we will have to do the following:
- Create a new
netstandard2.0
project,SqlHydra.Domain
. - Move
Domain.fs
and the new attribute to the new project. - In
Domain.fs
,open GlobExpressions
andapplyFilters
function need to be moved back to theSqlHydra
project; they can be added to a new fileFilters.fs
(since this is only used by the generators).
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.
This looks really good so far! 馃榾
FYI, when testing code generation changes, there are .bat files in the Tests.fsproj
folder that will regenerate the AdventureWorks.fs
file for each provider so you can quickly iterate.
| "json" -> { TypeName = nameof NpgsqlDbType; TypeValue = nameof NpgsqlDbType.Json } |> Some | ||
| "jsonb" -> { TypeName = nameof NpgsqlDbType; TypeValue = nameof NpgsqlDbType.Jsonb } |> Some | ||
| _ -> None | ||
|
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.
Rather than defining a lookup function, I think that this mapping should be added as a string option
field on the supportedTypeMappings
for each of the providers (NpgsqlDataTypes.fs
, SqliteDataTypes.fs
, and SqlServerDataTypes.fs
. Most of them can default to None
.
This value can then be moved from the Column
record to to the TypeMapping
record.
The TypeMapping
record already has a DbType
property that holds the System.Data.DbType
enum.
Since each provider has its own more specific enumeration (i.e. NpgsqlDbType
for Postgres, SqlDbType
for SQL Server -- and I don't think SQLite has a more specific enum), I suppose the new property should be ProviderDbType
.
@@ -87,6 +93,7 @@ let getSchema (cfg: Config) : Schema = | |||
|> Option.map (fun typeMapping -> | |||
{ | |||
Column.Name = col.ColumnName | |||
Column.DbColumnType = getDbColumnType col.ProviderTypeName |
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.
As stated above, move this property to TypeMapping
and rename to ProviderDbType
.
let value = System.Enum.Parse(property.PropertyType, type'.TypeValue) | ||
dbTypeSetter.Invoke(param, [|value|]) |> ignore | ||
| _ -> () | ||
|
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.
It would be nice to add a handler here for SQL Server as well:
| Some type', :? SqlKata.Compilers.SqlServerCompiler when type'.TypeName = "SqlDbType" ->
let property = param.GetType().GetProperty("SqlDbType")
...
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.
I had TypeName
and TypeValue
in the attribute before, but I figured you meant to just have a string
there so I stored only the enum value of the provider type without the type name (like NpgsqlDbType.Json
became just Json
). So now I can't really do the check on type'.TypeName = "SqlDbType"
. What are you thoughts on this? Do we need to store the full type like NpgsqlDbType.Json
or SqlDbType.Whatever
?
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.
What you have in NpgsqlDataTypes.fs
and the attribute looks perfect. I think that it will be safe enough to assume that if they are using a given SqlKata compiler that they should be passing in types with the appropriate attributes.
So this should work:
let setParameterDbType (param: DbParameter) (qp: QueryParameter) =
match qp.ProviderDbType, compiler with
| Some dbType, :? SqlKata.Compilers.PostgresCompiler ->
let property = param.GetType().GetProperty("NpgsqlDbType")
let dbTypeSetter = property.GetSetMethod()
let value = System.Enum.Parse(property.PropertyType, dbType)
dbTypeSetter.Invoke(param, [|value|]) |> ignore
| Some dbType, :? SqlKata.Compilers.SqlServerCompiler ->
let property = param.GetType().GetProperty("SqlDbType")
let dbTypeSetter = property.GetSetMethod()
let value = System.Enum.Parse(property.PropertyType, dbType)
dbTypeSetter.Invoke(param, [|value|]) |> ignore
| _ -> ()
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.
Yeah, the code now looks almost as yours, just with a helper function to hide the reflection ugliness 馃檪
src/SqlHydra/SchemaGenerator.fs
Outdated
@@ -631,7 +660,10 @@ let substitutions = | |||
[ | |||
/// Reader classes at top of namespace | |||
"open Substitute.Extensions", | |||
"""type Column(reader: System.Data.IDataReader, getOrdinal: string -> int, column) = | |||
$""" | |||
open {nameof SqlHydra}.{nameof ProviderDbTypeAttribute} |
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.
Is this ugly? Is there a better way of doing it? Like maybe specifying the full name of the attribute so that there is no need to import any modules? Smth like [<SqlHydra.ProviderDbType(...)>]
? I couldn't really figure out how do it with SynAttribute
.
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.
It's no uglier than any of the other substitutions there. 馃槄
But I think I would prefer using fully qualified attributes to reduce the number of ugly substitutions.
But this just sparked an idea:
We shouldn't need a shared project with an attribute because we could just recreate an attribute of the same name in the SqlHydra.Query
project! That would simplify things a bit because we wouldn't need a shared project at all, so better separation.
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.
hm... Are there any downsides of having a shared project?
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.
No downsides per se, other than the added complexity of having another project and copying project references to NuGet. But that's already done anyway, so I suppose it can stay as-is. Maybe it could come in handy at some point.
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.
Oh right, didn't think about copying it to nuget
. It is easy to revert with git
, so I don't really mind, if you think it is going to be better without it 馃檪
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.
It really seems like 50/50 - points could be made for either way. So let's just keep it as-is for now. 馃檪
I would like to add a field of type |
You can just add a test that drops and then creates a new table as is done in Dapper.FSharp. |
let createProviderDbTypeAttribute (mapping: TypeMapping) = | ||
mapping.ProviderDbType | ||
|> Option.map (fun type' -> | ||
let attributeFullName = typeof<ProviderDbTypeAttribute>.FullName |
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.
In order to avoid adding open SqlHydra....
to the string code block below I am using the full name of the attribute split by dot, and it seems like LongIdentWithDots.Create
manages to properly handle it.
If we were to duplicate the attribute here without having a shared project, we wouldn't be able to do this I think, and would have to hardcode the full name of the attribute from SqlHydra.Query
since it is parsed there.
@@ -74,8 +74,16 @@ let tests = | |||
|
|||
let pks = allColumns |> List.filter (fun c -> c.IsPK) | |||
|
|||
Expect.equal schema.Tables.Length 68 "" | |||
Expect.equal schema.Tables.Length 69 "" |
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.
Here I had to increase the number since I have a test that creates one more table to test the provider specific types (json
and jsonb
), however the test that creates that table (here) is not guaranteed to run before this one, so it seems very wrong to do this kind of thing 馃檪 Is there a way to add this new table to the database with all the other tables that were already there (person
, sales
etc)? Do I need to add an sql script that creates this table under /src/AdventureWorks/SalesLT/Tables
?
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.
One option would be to add it to the Docker postgres install.sql.
Another option might be to create the table setup test according to the Expecto Setup/Teardown example for a setup test, which I think would guarantee that it would always run first in the postrges tests.
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.
This approach seems to ensure that the setup fn is always executed before the tests:
let generateProviderDbTestTable () =
dropProviderDbTestTable ()
use ctx = openContext()
let createProviderDbTestTableCmd = ctx.Connection.CreateCommand ()
createProviderDbTestTableCmd.CommandText <-
"create schema provider_test; create table provider_test.test(id serial, json_field json not null, jsonb_field jsonb not null);"
createProviderDbTestTableCmd.ExecuteNonQuery () |> ignore
[<Tests>]
let tests =
generateProviderDbTestTable ()
categoryList "Npgsql" "Query Integration Tests" [
testTask "Where city Starts With S" {
use ctx = openContext()
let addresses =
select {
for a in addressTable do
where (a.city |=| [ "Seattle"; "Santa Cruz" ])
}
|> ctx.Read HydraReader.Read
gt0 addresses
Expect.isTrue (addresses |> Seq.forall (fun a -> a.city = "Seattle" || a.city = "Santa Cruz")) "Expected only 'Seattle' or 'Santa Cruz'."
}
...
Then you can remove generateProviderDbTestTable ()
from line 476.
@@ -52,12 +51,14 @@ let getSchema (cfg: Config) : Schema = | |||
TableName = col.["TABLE_NAME"] :?> string | |||
ColumnName = col.["COLUMN_NAME"] :?> string | |||
ProviderTypeName = col.["DATA_TYPE"] :?> string | |||
OrdinalPosition = col.["ORDINAL_POSITION"] :?> int |
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.
I noticed that every time you run the generator the order of the fields changes, and then one of the tests for insert query failed because the order of the fields has changed (see the test here) so I thought I would add sorting by ordinal position (I assume it is the order columns were added to the table) to avoid random order after every re-generation.
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.
That's a good idea, but I'm not sure that "ORDINAL_POSITION" is given for the other database providers.
If not, then I think a reasonable alternative would be to sort by column name.
I have added some tests but stumbled into a couple of problems:
even though it is running in docker. Could it be smth OS-specific? I am running mac, but I assume you are developing on windows (from the bat scripts and windows-like paths)? |
See response above.
Assuming the (Maybe I should add this detail to the Contributing section.) |
How's it going? Is this ready to merge or still 馃毀馃毀? No rush, of course; just wanted to make sure you weren't waiting on me. |
Was away for a couple of days 馃檪 Just ran the tests and all of them passed. Will you be able to review the code one last final time? I saw there was another PR fixing the order of the columns, but I think it is only for |
For some reason it is not adding the new table in postgres. I think it may have something to do with with the files on the local volume not be removed, so I tried |
I think I had to remove and re-create |
Fantastic work, Margarita. Thank you! 馃帀 |
Thanks a lot for your help! 馃槃 |
This PR attempts to fix #7 by adding a new attribute for fields that require db provider-specific type to be set on the corresponding command parameter. E.g.
postgres
requiresNpgsqlDbType.Json
andNpgsqlDbType.Jsonb
to be set on the command parameter for columns of typesjson
andjsonb
correspondingly when usingNpgsql
data provider.I am planning to add tests for the functionality after the initial review, and when I figure out how to add a new
json
column to the existing database used in tests 馃槃