JordanMarr / SqlHydra

SqlHydra is a suite of NuGet packages for working with databases in F# including code generation tools and query expressions.
MIT License
212 stars 21 forks source link

Allow SQL with full datetime2 precision on SQL Server databases #33

Closed ntwilson closed 1 year ago

ntwilson commented 1 year ago

I do not think this PR is ready to be merged yet. The fix ended up being more complicated than it initially looked. The issue is that QueryContext.setProviderDbType is ultimately only called on INSERT and UPDATE (SET & entity) commands, but not on SELECT or the WHERE clause of an UPDATE. This was fixed in this PR by altering LinqExpressionVisitors to call KataUtils.getQueryParameterForValue for a simple "\ \ \" expression, but

  1. I'm not confident that a similar change doesn't need to be made to other spots in LinqExpressionVisitors, like a WHERE IN/NOT IN query, or a HAVING query, etc.
  2. I'm not sure what unintended consequences of this change would be. e.g., getQueryParameterForValue mucks with handling Options in addition to setting the ProviderDbType.

I think these two at least are going to need answers before this can be merged.

I'd appreciate if @JordanMarr (or whoever else is investigating) would run the full test suite. I have something wrong with my setup, and many of my tests fail even when running on main, so I'm not sure if these changes broke any existing tests. I can work on fixing any breaking tests, but I don't think I can detect it very easily.

JordanMarr commented 1 year ago

I have pull your branch and will start reviewing it in more detail, but this looks great so far.

JordanMarr commented 1 year ago

Trying to get your install.sql script to run but I always forget how to make Docker rebuild a DB from scratch.

EDIT: docker-compose up --build mssql

JordanMarr commented 1 year ago

So after running the tests I realized that we really can't convert the parameter values into QueryParameters within the LinqExpressionVisitor because SqlKata apparently inspects the parameter values and has its own encapsulated logic at that level. For example, I have a test query that checks if a column <> None, and SqlKata has logic that inspects the value and changes it to IS NOT NULL instead of <> NULL.

So right now I am experimenting with returning a Query * MemberInfo list from visitWhere and visitHaving, so that I can pass the MemberInfo downstream.

JordanMarr commented 1 year ago

On second thought, screw that. We can check for = null and <> null in the LinqExpressionVisitors and route those to SqlKata WhereNull and WhereNotNull instead of relying on their logic to do it. I think that will be easier and we can just get this thing shipped.

ntwilson commented 1 year ago

Thanks! I feel like this one ended up sucking up a lot of your time, and I appreciate the time you put in!

JordanMarr commented 1 year ago

Thank you for making the project better! It's really great to see some of these details getting resolved due to contributions.

ntwilson commented 1 year ago

Now with KataUtils.getQueryParameterForValue being called by LinqExpressionVisitors would the recommended approach for querying a nullable field change from where (row.field.Value = value) to where (row.field = Some value)? Because the Option would be removed at runtime from the literal value passed to the database?

JordanMarr commented 1 year ago

My intent was to allow you the flexibility to use either style for Option types.

Did I inadvertently break that? If so, then some more tests may be in order to pinpoint the issues.

ntwilson commented 1 year ago

Oh sorry, I wasn't pointing out any breakages or anything, just asking what best practice is. I noticed beforehand that if I said where (row.field = Some value) it would crash at runtime because the Some value would get sent as an Option over to the DB, which wouldn't know how to interpret it. I'm surmising that it would work now because KataUtils.getQueryParameterForValue is called on the RHS, but I haven't actually tested that. I thought maybe you knew offhand which forms did/didn't work and had a best practice in mind.

JordanMarr commented 1 year ago

That scenario should have been supported all along. The Value active pattern should have been detecting and unwrapping Option values. (The only caveat being that it doesn't handle properties on nested entities.)

So you should submit an issue if you find that it is not working.