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 20 forks source link

Allow to generate the list of tables? #45

Closed MangelMaxime closed 1 year ago

MangelMaxime commented 1 year ago

Maintaining the list of Tables is a bit tedious.

When a table is removed it is easy to update the list because there is a compiler error.

However, when adding a table the compiler doesn't provide any help. Would it be possible to add support for generating the list of tables?

The things that would need to be configurable are the output and the namespace (could be inherited from the general.namespace.

In my project we generate it using this convention:

namespace Antidote.Database

[<RequireQualifiedAccess>]
module Tables =

    open SqlHydra.Query

    let acsIdentities = table<dbo.AcsIdentities> |> inSchema (nameof dbo)
    let acsTokens = table<dbo.AcsTokens> |> inSchema (nameof dbo)
    let callQueueEntries = table<dbo.CallQueueEntries> |> inSchema (nameof dbo)

    let callRatingImprovementsByCallRating =
        table<dbo.CallRatingImprovementsByCallRating> |> inSchema (nameof dbo)

    let callRatings = table<dbo.CallRatings> |> inSchema (nameof dbo)

Having Tables marked as RequireQualifiedAccess makes the consumer experience easy.

JordanMarr commented 1 year ago

Does it help that in v1.1 (just released a few hours ago) inSchema is no longer required (since it can be inferred from the parent schema module)?

So now it is more feasible to bypass the table list altogether:

select {
    for token in table<dbo.AcsTokens> do
    select token
}
MangelMaxime commented 1 year ago

Hum good question... that a difficult question.

Personally, I much prefer reading Tables.users vs table<dbo.Users>. And I love the simplicity of doing Tables.<ctrl+space>.

But if it was using table<dbo.Users> since the beginning I think I would have not question it.

Also, the Tables code structure is probably opiniated like some people could ask to add/remove the RequireQualifiedAccess because they would prefer to write usersTable instead of Tables.users.

So I will close this issue and try to get used to the new code. I will re-open it if I have issues with it.

JordanMarr commented 1 year ago

I am still open to the idea of generating the tables (and other conveniences). It could possibly be configurable in the .toml configuration file.

MangelMaxime commented 1 year ago

I fear this would open the door for people to ask support of this feature for others readers too.

In total honesty, I think Tables.XXX is providing a better user experience because it is a more common code to write I think.

If you think this is a good to have feature, I can help in the design phase and perhaps to implement it.

JordanMarr commented 1 year ago

Rather than rebuilding the schema structure underneath a Tables module, I would prefer generating the tables within the existing schema modules. For example:

[<CLIMutable>]
type ErrorLog =
    { ErrorLogID: int
      [<SqlHydra.ProviderDbType("DateTime")>]
      ErrorTime: System.DateTime
      UserName: string
      ErrorNumber: int
      ErrorSeverity: Option<int>
      ErrorState: Option<int>
      ErrorProcedure: Option<string>
      ErrorLine: Option<int>
      ErrorMessage: string }

let ErrorLogTable = SqlHydra.Query.Table.table<ErrorLog>

type ErrorLogReader(reader: Microsoft.Data.SqlClient.SqlDataReader, getOrdinal) =
    // ...
let! errorLogId = 
    insertTask (Shared ctx) {
        for e in dbo.ErrorLogTable do
        entity errorLog
        getId e.ErrorLogID
    }

That does seem like it would be a very nice feature!

JordanMarr commented 1 year ago

Another alternative would be to include a Tables module at the end of each schema module:

module dbo =
    [<CLIMutable>]
    type ErrorLog =
        { ErrorLogID: int
          [<SqlHydra.ProviderDbType("DateTime")>]
          ErrorTime: System.DateTime
          UserName: string
          ErrorNumber: int
          ErrorSeverity: Option<int>
          ErrorState: Option<int>
          ErrorProcedure: Option<string>
          ErrorLine: Option<int>
          ErrorMessage: string }

    type ErrorLogReader(reader: Microsoft.Data.SqlClient.SqlDataReader, getOrdinal) =
        member __.ErrorLogID = RequiredColumn(reader, getOrdinal, reader.GetInt32, "ErrorLogID")
        member __.ErrorTime = RequiredColumn(reader, getOrdinal, reader.GetDateTime, "ErrorTime")
        member __.UserName = RequiredColumn(reader, getOrdinal, reader.GetString, "UserName")
        member __.ErrorNumber = RequiredColumn(reader, getOrdinal, reader.GetInt32, "ErrorNumber")
        member __.ErrorSeverity = OptionalColumn(reader, getOrdinal, reader.GetInt32, "ErrorSeverity")
        member __.ErrorState = OptionalColumn(reader, getOrdinal, reader.GetInt32, "ErrorState")
        member __.ErrorProcedure = OptionalColumn(reader, getOrdinal, reader.GetString, "ErrorProcedure")
        member __.ErrorLine = OptionalColumn(reader, getOrdinal, reader.GetInt32, "ErrorLine")
        member __.ErrorMessage = RequiredColumn(reader, getOrdinal, reader.GetString, "ErrorMessage")
        member __.Read() =
            { ErrorLogID = __.ErrorLogID.Read()
              ErrorTime = __.ErrorTime.Read()
              UserName = __.UserName.Read()
              ErrorNumber = __.ErrorNumber.Read()
              ErrorSeverity = __.ErrorSeverity.Read()
              ErrorState = __.ErrorState.Read()
              ErrorProcedure = __.ErrorProcedure.Read()
              ErrorLine = __.ErrorLine.Read()
              ErrorMessage = __.ErrorMessage.Read() }

        member __.ReadIfNotNull() =
            if __.ErrorLogID.IsNull() then None else Some(__.Read())

    [<RequireQualifiedAccess>]
    module Tables = 
        let ErrorLog = SqlHydra.Query.Table.table<ErrorLog>
let! errorLogId = 
    insertTask (Shared ctx) {
        for e in dbo.Tables.ErrorLog do
        entity errorLog
        getId e.ErrorLogID
    }
MangelMaxime commented 1 year ago

Personally I don't like the version using Table as a suffix.

My reason for that is Table is the important information but because it is at the end of the it difficult to sport and also you don't get a nice user experience.

If you compare dbo.<ctrl+space> and Tables.<ctrl+space> in term of UX the first one will give you way too much information and the user will always need to filter himself the result between the object record and the tables.

I think using dbo.Tables.ErrorLog is a bit more verbose than Tables.ErrorLog but it has the nice benefit of supporting multiple schema if people needs to.

And in term of UX we have:

  1. dbo.<Ctrl+Space> to get access to the object types
  2. dbo.Tables.<Ctrl+Space> to get access to the tables definitions

It allows to have good separation of concerns and don't cause too much pollution.

To summarise, between your two propositions I do prefer dbo.Tables.ErrorLog

MangelMaxime commented 1 year ago

Another benefit of having it generated in the same file is that it can be generated by default when the user opt-in for SqlHydra Reader to be generated so we don't need another configuration parameter.

JordanMarr commented 1 year ago

Thinking ahead, there is also a request in the pipeline to generate SQL Server Table Valued Functions. This would involve generating a function definition and a function record return type. I think we could generate a similar Functions module for those:

CREATE FUNCTION udfProductInYear (
    @model_year INT
)
RETURNS TABLE
AS
RETURN
    SELECT 
        product_name,
        model_year,
        list_price
    FROM
        production.products
    WHERE
        model_year = @model_year;
module dbo =
    [<CLIMutable>]    
    type udfProductInYear =
        {
            ProductName: string
            ModelYear: int
            ListPrice: decimal
        }

    module Tables = 
        let ErrorLog = SqlHydra.Query.Table.table<ErrorLog>

    module Functions =
        let udfProductInYear = SqlHydra.Query.Function.udf<int, udfProductInYear>
JordanMarr commented 1 year ago

Anyways, the Tables change seems nice and is approved. Are you still interested in doing the implementation?

MangelMaxime commented 1 year ago

I am still interested for doing the implementation but I will only have some time to work on it in 1-2 weeks.

Do we generates the Tables module all the time or do we need to detect that user asked for SqlHydra reader?

JordanMarr commented 1 year ago

The current design assumes that the user wants integration with the SqlHydra.Query package when the "Generate HydraReader" option is enabled, and so your only check would be cfg.Readers.IsSome.

However, while that assumption is generally true, Isaac Abraham recently posted issue #42 because he wanted to generate the HydraReader and use the generated code without SqlHydra.Query.

The potential options for handling Isaac's issue would be:

1) Do nothing and continue with the assumption that the "Generate HydraReader" option will also generate SqlHydra.Query integration. SqlHydra.Query package will be required for users if they want to use this feature.

~or ~

2) Introduce a new toml configuration, disable_query_integration, that defaults to false if not present. This would allow users to generate the HydraReader without including any references from SqlHydra.Query.

What is your opinion on how to handle this?

Option 1 keeps things simple, but forces users with this edge case to install an extra package. Option 2 adds some complexity to handle an edge case.

MangelMaxime commented 1 year ago

Is the disable_query_integration config the same thing as exclude_providerdbtype_attributes or are they 2 differents config?

People could want to have the Table generated for others provider too. For example, in the Dapper example you also have:

let customerTable =         table<Customer>         |> inSchema (nameof SalesLT)

How would we handle this case?

I don't understand what reader_type is for? Does it changes the generated output?

JordanMarr commented 1 year ago

Re: configuration settings I haven't decided on what the configuration name would be yet; exclude_providerdbtype_attributes is a very specific name, and disable_query_integration is more general. I think I am leaning toward more general at the moment, but I'm not sure.

Re: Dapper.FSharp example If a user wanted to generate types for use with another library like Dapper.FSharp, then they would need to disable HydraReader, and they would also want the ability to disable/exclude any SqlHydra.Query references.
(In the Dapper example, table and inSchema are part of Dapper.FSharp.)
So I guess this is another example that suggests that having some sort of disable_query_integration is a good idea.

_Re: what is reader_type_ reader_type is required by the HydraReader because the more general System.Data.IDataReader does not provide us with all the functionality we need for some provider-specific columns. For example, getting Postgres array columns requires reader.GetFieldValue which does not exist on the more general IDataReader. Knowing the more specific provider ensure that we have access to any provider specific stuff that we might need. I looked into using System.Data.Common.DbDataReader in the past but I think I hit a blocker at some point that led me down this path. It may be worth looking into again at some point to see if it could be used with reader.GetFieldValue<'T> for everything.

MangelMaxime commented 1 year ago

_Re: what is reader_type_ reader_type is required by the HydraReader because the more general System.Data.IDataReader does not provide us with all the functionality we need for some provider-specific columns. For example, getting Postgres array columns requires reader.GetFieldValue which does not exist on the more general IDataReader. Knowing the more specific provider ensure that we have access to any provider specific stuff that we might need. I looked into using System.Data.Common.DbDataReader in the past but I think I hit a blocker at some point that led me down this path. It may be worth looking into again at some point to see if it could be used with reader.GetFieldValue<'T> for everything.

So basically it does open <value from reader_type>?

Re: Dapper.FSharp example If a user wanted to generate types for use with another library like Dapper.FSharp, then they would need to disable HydraReader, and they would also want the ability to disable/exclude any SqlHydra.Query references. (In the Dapper example, table and inSchema are part of Dapper.FSharp.) So I guess this is another example that suggests that having some sort of disable_query_integration is a good idea.

I don't think disable_query_integration would be enough because how would we know that the user want to generates the tables for SQL Hydra or Dapper?

As you said, we need to generate different code/import depending on the target library.

Would it make sense to have a config object dedicated to the tables?

[tables]
table_type = "SqlHydra"

#or
table_type = "Dapper

Personally, I would go with target specific settings like disable_tables_generation or

[tables]
disable=true

but it is true that I love explicit code in general :)

Or is the table generation tightly related to the readers and we could do?

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "SqlHydra" or "Dapper" or "Other"
disable_tables_generation=true/false

My idea is that reader_lib would solve #42 too because user could tell which library he is using for reading.

So if the user want to generate only the record he doesn't add the [readers] object.

If he wants the reader for hydra he does:

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "SqlHydra" # For backward compatibility if this option is not set we default to "SqlHydra" 

If he wants the reader for another library:

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "Other"

This would not generate the specific attributes mentioned in #42

And finally, if we provide better support for specific library like Dapper he would use:

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "Dapper"

Which would not generate the SqlHydra attributes but generate the tables with the correct types.

What I am failing to understand is for Dapper and others do we need the readers to be generated or just the Record + Tables info.

Sorry, if my proposition is incorrect I am still trying to understand the different case/configuration :)

JordanMarr commented 1 year ago

Great questions, and I appreciate you helping me sort through the intricacies. Ideally this library can be powerful without being too confusing or hard to reason about for the end users. šŸ˜„

_Re: what is reader_type_ reader_type is required by the HydraReader because the more general System.Data.IDataReader does not provide us with all the functionality we need for some provider-specific columns.

So basically it does open <value from reader_type>?

More specifically, it substitutes the reader_type when creating the {tableName}Reader classes. (I made a decision to generally avoid open statements in the generated code to keep things simple.)

For example, reader_type is passed in to the DepartmentReader:

module HumanResources =
    [<CLIMutable>]
    type Department =
        { DepartmentID: int16
          Name: string
          GroupName: string
          [<SqlHydra.ProviderDbType("DateTime")>]
          ModifiedDate: System.DateTime }

    type DepartmentReader(reader: Microsoft.Data.SqlClient.SqlDataReader, getOrdinal) =
        member __.DepartmentID = RequiredColumn(reader, getOrdinal, reader.GetInt16, "DepartmentID")
        member __.Name = RequiredColumn(reader, getOrdinal, reader.GetString, "Name")
        member __.GroupName = RequiredColumn(reader, getOrdinal, reader.GetString, "GroupName")
        member __.ModifiedDate = RequiredColumn(reader, getOrdinal, reader.GetDateTime, "ModifiedDate")
        member __.Read() =
            { DepartmentID = __.DepartmentID.Read()
              Name = __.Name.Read()
              GroupName = __.GroupName.Read()
              ModifiedDate = __.ModifiedDate.Read() }

        member __.ReadIfNotNull() =
            if __.DepartmentID.IsNull() then None else Some(__.Read())

Re: Dapper.FSharp example If a user wanted to generate types for use with another library like Dapper.FSharp, then they would need to disable HydraReader, and they would also want the ability to disable/exclude any SqlHydra.Query references. (In the Dapper example, table and inSchema are part of Dapper.FSharp.) So I guess this is another example that suggests that having some sort of disable_query_integration is a good idea.

I don't think disable_query_integration would be enough because how would we know that the user want to generates the tables for SQL Hydra or Dapper? As you said, we need to generate different code/import depending on the target library.

Would it make sense to have a config object dedicated to the tables?

[tables]
table_type = "SqlHydra"

#or
table_type = "Dapper

My main focus for the code generation is to serve SqlHydra.Query. I'm willing to put some minimal effort towards making it friendly for "stand-alone" / "general purpose" use, but that is a low priority.

Also, I don't think most people using Dapper.FSharp are using SqlHydra type generation. In my opinion, the best reason to choose Dapper.FSharp over SqlHydra would be for small projects where type generation is overkill. Some users just want to pull down a single library and start rolling their own types because it's simpler than having to configure type generation tooling.

But for projects where a user does want to generate types, I think it makes more sense to use SqlHydra.Query because it is essentially v2.0 of the query engine I designed for Dapper.FSharp with more features (most of which are made possible by the type generation, or by the underlying SqlKata library). Simply put, SqlHydra has a little bit more of a "getting started" curve due to the generation tools, but the tradeoff is that it provides is more type-safety and more features.

Personally, I would go with target specific settings like disable_tables_generation or

[tables]
disable=true

but it is true that I love explicit code in general :)

Or is the table generation tightly related to the readers and we could do?

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "SqlHydra" or "Dapper" or "Other"
disable_tables_generation=true/false

There are currently only a few SqlHydra.Query specific integrations:

1) SqlHydra.ProviderDbType attributes are generated to provide column metadata that SqlHydra.Query uses when creating ADO.NET parameter types. For example, SQL Server DateTime vs DateTime2, or Npgsql array types. 2) Your new Tables module. 3) A similar 'Functions' module is on the horizon.

Strongly typed readers are required for SqlHydra.Query integration, but should also be usable for stand-alone / general purpose.

The argument for having a more general disable_query_integration setting is that there is no scenario where someone would choose one but not the other. The decision point really is, "are you using SqlHydra.Query?" If so, then all of those integration features need to be generated. If not, then none of those integration features should be generated.

The SqlHydra.ProviderDbType attributes are added to the table record fields, so they are technically not related the [readers], but they are required for query integration.

The new Tables modules are also not related to [readers], but they are required for query integration.

My idea is that reader_lib would solve #42 too because user could tell which library he is using for reading.

So if the user want to generate only the record he doesn't add the [readers] object.

If he wants the reader for hydra he does:

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "SqlHydra" # For backward compatibility if this option is not set we default to "SqlHydra" 

If he wants the reader for another library:

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "Other"

This would not generate the specific attributes mentioned in #42

And finally, if we provide better support for specific library like Dapper he would use:

[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
reader_lib= "Dapper"

Which would not generate the SqlHydra attributes but generate the tables with the correct types.

What I am failing to understand is for Dapper and others do we need the readers to be generated or just the Record + Tables info.

Sorry, if my proposition is incorrect I am still trying to understand the different case/configuration :)

There are three use cases: 1) SqlHydra.Query integration 2) Other DB library 3) Standalone use

JordanMarr commented 1 year ago

Use Cases / Configured Features

This table shows the configured generation features needed for each of the three use cases.

Ā  SqlHydra.Query Integration (Default) Other DB Library Standalone Use
Strongly Typed Readers āœ”ļø āŒ āœ”ļø
DbProvider Attributes āœ”ļø āŒ āŒ
Tables Modules āœ”ļø āŒ āŒ
Functions Modules āœ”ļø āŒ āŒ
CLIMutable Attribute āœ”ļø āœ”ļø āœ”ļø
JordanMarr commented 1 year ago

As I look at the use case / features grid, it's becoming clear to me that the first-time run CLI wizard be simplified to only ask the user to pick one of the three use cases, and then should set the features accordingly.

For the toml options, this is what they might look like if they were more granular:

[general]
connection = "Server=localhost,12019;Database=AdventureWorks;User=sa;Password=password123;TrustServerCertificate=True"
output = "SqlServer/AdventureWorksNet6.fs"
namespace = "SqlServer.AdventureWorksNet6"
cli_mutable = false # default to false if missing
[sqlhydra_query_integration]
provider_db_type_attributes = true # default to true only if section exists
table_declarations = true # default to true only if section exists
functions_module = true # default to true only if section exists
[readers]
reader_type = "Microsoft.Data.SqlClient.SqlDataReader"
[filters]
include = [ "*" ]
exclude = [ "*/v*" ]
JordanMarr commented 1 year ago

Another idea occurred to me. You mentioned that having Tables modules would reduce the noise when using intellisense to get a table. It occurred to me that the noise you are probably referring to is likely due to the Reader classes that are generated alongside the table records.

Since the Reader classes are never used directly by the user (they are used by the HydraReader), perhaps they should be moved into the hydra Reader and made private. That would cut the noise in half when dotting into a scheme module to find a table.

It might even eliminate the need to generate a separate table definitions module .

MangelMaxime commented 1 year ago

Thank you for the detailed explanations.

From what I see in you table we have 3 scenarios to cover so a simple boolean is not enough. If we go with a single configuration option we need to go with an "enum" (SqlHydra, Standalone, Other). Still need work on the naming or have a good documentation to explains each cases :)

It occurred to me that the noise you are probably referring to is likely due to the Reader classes that are generated alongside the table records.

Hum the noise I am referring too is not related to only the Readers but to everything that is not what I am looking for.

And now, that you mention it:

So I think the generated code for me would be something like that:

// This code was generated by `SqlHydra.SqlServer` -- v1.0.3.0.
namespace Antidote.Database

module dbo =
    type Accounts =
        { Id: System.Guid
          Type: int
          Username: string }

    type private AccountsReader(reader: Microsoft.Data.SqlClient.SqlDataReader, getOrdinal) =
        member __.Id = RequiredColumn(reader, getOrdinal, reader.GetGuid, "Id")
        member __.Type = RequiredColumn(reader, getOrdinal, reader.GetInt32, "Type")
        member __.Username = RequiredColumn(reader, getOrdinal, reader.GetString, "Username")

        member __.Read() =
            { Id = __.Id.Read()
              Type = __.Type.Read()
              Username = __.Username.Read() }

        member __.ReadIfNotNull() =
            if __.Id.IsNull() then None else Some(__.Read())

    module Tables =

        let accounts = SqlHydra.Query.Table.table<Accounts>

    module Functions =

        let func1 = // ...

type HydraReader(reader: Microsoft.Data.SqlClient.SqlDataReader) =
    static member Read(reader: Microsoft.Data.SqlClient.SqlDataReader) = 
        // ...

The Readers are marked as private because we don't need to access it like you mentioned.

When doing dbo.<Ctrl+space> the user is presented with 3 options:

  1. He can access the Record types

    {
        dbo.Accounts.Id = Guid.NewGuid()
        dbo.Accounts.Type = 0
        dbo.Accounts.Username = "jdoe"
    }
  2. He can go to dbo.Tables.<Ctrl+space> to be presented with only tables
  3. He can go to dbo.Functions.<Ctrl+space> to be presented with only functions

One question that arise is do the functions need their own records? Where would you store them?


There is also this section generated at the top of the file which could be seen as code pollution.

// This code was generated by `SqlHydra.SqlServer` -- v1.0.3.0.
namespace Antidote.Database

type Column(reader: System.Data.IDataReader, getOrdinal: string -> int, column) =
        member __.Name = column
        member __.IsNull() = getOrdinal column |> reader.IsDBNull
        override __.ToString() = __.Name

type RequiredColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getter: int -> 'T, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = alias |> Option.defaultValue __.Name |> getOrdinal |> getter

type OptionalColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getter: int -> 'T, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = 
            match alias |> Option.defaultValue __.Name |> getOrdinal with
            | o when reader.IsDBNull o -> None
            | o -> Some (getter o)

type RequiredBinaryColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getValue: int -> obj, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = alias |> Option.defaultValue __.Name |> getOrdinal |> getValue :?> byte[]

type OptionalBinaryColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getValue: int -> obj, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = 
            match alias |> Option.defaultValue __.Name |> getOrdinal with
            | o when reader.IsDBNull o -> None
            | o -> Some (getValue o :?> byte[])

[<AutoOpen>]        
module Utils =
    type System.Data.IDataReader with
        member reader.GetDateOnly(ordinal: int) = 
            reader.GetDateTime(ordinal) |> System.DateOnly.FromDateTime

    type System.Data.Common.DbDataReader with
        member reader.GetTimeOnly(ordinal: int) = 
            reader.GetFieldValue(ordinal) |> System.TimeOnly.FromTimeSpan

Should it be marked as private? Should it be moved into its own module so the types Column, RequiredColumn, OptionalColumn, RequiredBinaryColumn, OptionalBinaryColumn are not exposed when opening Antidote.Database?

JordanMarr commented 1 year ago

From what I see in you table we have 3 scenarios to cover so a simple boolean is not enough. If we go with a single configuration option we need to go with an "enum" (SqlHydra, Standalone, Other). Still need work on the naming or have a good documentation to explains each cases :)

I liked your suggestion to keep the configuration granular. In fact, yesterday I released v1.1.1 that simplifies the CLI wizard to only ask for the very high-level use case, and then uses that to set the more granular configuration options (and then discards the "use case"). It also added a new configuration (to deal with Isaac's issue where he was using as "Standalone" but was getting SqlHydra.Query attributes generated):

[sqlhydra_query_integration]
provider_db_type_attributes = false

(The future tables_modules and functions_modules configuration options will also be added to this section.)

It occurred to me that the noise you are probably referring to is likely due to the Reader classes that are generated alongside the table records.

Hum the noise I am referring too is not related to only the Readers but to everything that is not what I am looking for.

  • If I am looking for Tables I want to only see Tables related information.
  • If I am looking for Functions I want to only see Functions related information.

Ok, it sounds like the Tables (and eventually Functions) modules are still a good idea. And since they will have their own configuration switches, people can disable them if they wish.

And now, that you mention it:

  • If I am looking for Records/Generated types I want to only see Records/Generated types related information not the readers.

So I think the generated code for me would be something like that:

// This code was generated by `SqlHydra.SqlServer` -- v1.0.3.0.
namespace Antidote.Database

module dbo =
    type Accounts =
        { Id: System.Guid
          Type: int
          Username: string }

    type private AccountsReader(reader: Microsoft.Data.SqlClient.SqlDataReader, getOrdinal) =
        member __.Id = RequiredColumn(reader, getOrdinal, reader.GetGuid, "Id")
        member __.Type = RequiredColumn(reader, getOrdinal, reader.GetInt32, "Type")
        member __.Username = RequiredColumn(reader, getOrdinal, reader.GetString, "Username")

        member __.Read() =
            { Id = __.Id.Read()
              Type = __.Type.Read()
              Username = __.Username.Read() }

        member __.ReadIfNotNull() =
            if __.Id.IsNull() then None else Some(__.Read())

    module Tables =

        let accounts = SqlHydra.Query.Table.table<Accounts>

    module Functions =

        let func1 = // ...

type HydraReader(reader: Microsoft.Data.SqlClient.SqlDataReader) =
    static member Read(reader: Microsoft.Data.SqlClient.SqlDataReader) = 
        // ...

The Readers are marked as private because we don't need to access it like you mentioned.

I am definitely in favor of reworking the readers code so that it is hidden from intellisense.

When doing dbo.<Ctrl+space> the user is presented with 3 options:

  1. He can access the Record types
    {
      dbo.Accounts.Id = Guid.NewGuid()
      dbo.Accounts.Type = 0
      dbo.Accounts.Username = "jdoe"
    }
  2. He can go to dbo.Tables.<Ctrl+space> to be presented with only tables
  3. He can go to dbo.Functions.<Ctrl+space> to be presented with only functions

That sounds good.

One question that arise is do the functions need their own records? Where would you store them?

I believe that the "table valued functions" will need to have their return types generated. They will live directly within the schema, alongside the table records. However, that should be fine since we will have Tables and Functions modules.

There is also this section generated at the top of the file which could be seen as code pollution.

// This code was generated by `SqlHydra.SqlServer` -- v1.0.3.0.
namespace Antidote.Database

type Column(reader: System.Data.IDataReader, getOrdinal: string -> int, column) =
        member __.Name = column
        member __.IsNull() = getOrdinal column |> reader.IsDBNull
        override __.ToString() = __.Name

type RequiredColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getter: int -> 'T, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = alias |> Option.defaultValue __.Name |> getOrdinal |> getter

type OptionalColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getter: int -> 'T, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = 
            match alias |> Option.defaultValue __.Name |> getOrdinal with
            | o when reader.IsDBNull o -> None
            | o -> Some (getter o)

type RequiredBinaryColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getValue: int -> obj, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = alias |> Option.defaultValue __.Name |> getOrdinal |> getValue :?> byte[]

type OptionalBinaryColumn<'T, 'Reader when 'Reader :> System.Data.IDataReader>(reader: 'Reader, getOrdinal, getValue: int -> obj, column) =
        inherit Column(reader, getOrdinal, column)
        member __.Read(?alias) = 
            match alias |> Option.defaultValue __.Name |> getOrdinal with
            | o when reader.IsDBNull o -> None
            | o -> Some (getValue o :?> byte[])

[<AutoOpen>]        
module Utils =
    type System.Data.IDataReader with
        member reader.GetDateOnly(ordinal: int) = 
            reader.GetDateTime(ordinal) |> System.DateOnly.FromDateTime

    type System.Data.Common.DbDataReader with
        member reader.GetTimeOnly(ordinal: int) = 
            reader.GetFieldValue(ordinal) |> System.TimeOnly.FromTimeSpan

Should it be marked as private? Should it be moved into its own module so the types Column, RequiredColumn, OptionalColumn, RequiredBinaryColumn, OptionalBinaryColumn are not exposed when opening Antidote.Database?

I'm not particularly concerned with the way the generated code looks since it hidden from view by default. However, I am not against reworking to make them hidden from Intellisense.

MangelMaxime commented 1 year ago

From what I see in you table we have 3 scenarios to cover so a simple boolean is not enough. If we go with a single configuration option we need to go with an "enum" (SqlHydra, Standalone, Other). Still need work on the naming or have a good documentation to explains each cases :)

I liked your suggestion to keep the configuration granular. In fact, yesterday I released v1.1.1 that simplifies the CLI wizard to only ask for the very high-level use case, and then uses that to set the more granular configuration options (and then discards the "use case"). It also added a new configuration (to deal with Isaac's issue where he was using as "Standalone" but was getting SqlHydra.Query attributes generated):

Really cool :)

One question that arise is do the functions need their own records? Where would you store them?

I believe that the "table valued functions" will need to have their return types generated. They will live directly within the schema, alongside the table records. However, that should be fine since we will have Tables and Functions modules.

I asked this question with a specific situation in mind. I suppose the name of the function record will come from the function name.

Is it possible to have a Table and a Function sharing the same name? If yes, how will the conflict of name be resolved?

I'm not particularly concerned with the way the generated code looks since it hidden from view by default. However, I am not against reworking to make them hidden from Intellisense.

I am not particularly concerned neither but if we are doing some clean up why not taking that chance too :)

JordanMarr commented 1 year ago

I believe that the "table valued functions" will need to have their return types generated. They will live directly within the schema, alongside the table records. However, that should be fine since we will have Tables and Functions modules.

I asked this question with a specific situation in mind. I suppose the name of the function record will come from the function name.

There will need to be a function very similar to the table function that will return a QuerySource<'T>, where 'T is the table valued function's return type. I would call it function, but that is a reserved keyword, so it will have to be something else. So if the table valued function's name is udfGetOrders that takes an 'int' as input, the generated code might look like this:

module dbo = 
    type udfGetOrders = { OrderId: int; Total: decimal } // the generated function return type

    module Functions = 
        // Partially applied, takes an `int`, returns a `QuerySource<udfGetOrders>`
        let udfGetOrders = SqlHydra.Query.Function.udf<int, udfGetOrders> 
    let udf<'Input, 'Output> (input: 'Input) = 
        let ent = typeof<'Output>
        let tables = Map [Root, { Name = ent.Name; Schema = ent.DeclaringType.Name}]
        QuerySource<'Output>(tables)

Usage:

select {
    for o in dbo.Functions.udfGetOrders 123 do
    select o
}

Is it possible to have a Table and a Function sharing the same name? If yes, how will the conflict of name be resolved?

My assumption was that SQL Server should disallow naming a function with the same name as an existing table. I just tried it and verified that it does throw an error:

image

I'm not particularly concerned with the way the generated code looks since it hidden from view by default. However, I am not against reworking to make them hidden from Intellisense.

I am not particularly concerned neither but if we are doing some clean up why not taking that chance too :)

Agreed!

JordanMarr commented 1 year ago

Messing around with the AdventureWorksNet5.fs file in Tests.fsproj to see how things could be better hidden. The best I have come up with so far to hide the readers is to follow the Tables and Functions pattern by putting them in a Readers module. (Then the Reader suffix could be removed too.)

However, making the Readers module private doesn't allow the HydraReader to access them. But at least moving them into a Readers module will clean up the schema module: image

JordanMarr commented 1 year ago

The Readers change has been made and is checked into the main branch.

MangelMaxime commented 1 year ago

Awesome, really liking the directing we are going right now.

Thank you for your work :)

JordanMarr commented 1 year ago

Hmm... don't know why I didn't try this before, but this works:


module dbo =
    [<CLIMutable>]
    type ErrorLog =
        { ErrorLogID: int
          [<SqlHydra.ProviderDbType("DateTime")>]
          ErrorTime: System.DateTime
          UserName: string
          ErrorNumber: int
          ErrorSeverity: Option<int>
          ErrorState: Option<int>
          ErrorProcedure: Option<string>
          ErrorLine: Option<int>
          ErrorMessage: string }

    let ErrorLog = SqlHydra.Query.Table.table<ErrorLog>
select {
    for e in dbo.ErrorLog do
    select e.ErrorNumber
}
module Person =
    [<CLIMutable>]
    type Address =
        { AddressID: int
          AddressLine1: string
          AddressLine2: Option<string>
          City: string
          StateProvinceID: int
          PostalCode: string
          rowguid: System.Guid
          [<SqlHydra.ProviderDbType("DateTime")>]
          ModifiedDate: System.DateTime }

    let Address = SqlHydra.Query.Table.table<Address>
select {
    for a in Person.Address do
    where (a.City |=| [ "Seattle"; "Santa Cruz" ])
}

I think I like that the best because the table identifier looks the same as it does in SQL.

JordanMarr commented 1 year ago

Released here! https://github.com/JordanMarr/SqlHydra/releases/tag/v1.2.0

MangelMaxime commented 1 year ago

Thank you for working on this issue @JordanMarr