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

Send parameters with nil type as type :string #162

Closed
wants to merge 1 commit into from

Conversation

wojtekmach
Copy link
Member

@wojtekmach wojtekmach commented Jun 7, 2024

Closes #124.

Before this patch we were sending them as type :binary:

Mix.install([
  {:tds, "2.3.5"}
])

defmodule Main do
  def main do
    {:ok, pid} =
      Tds.start_link(
        hostname: "localhost",
        username: "sa",
        password: "secret1@A",
        database: "mix_install_examples",
        port: 1433,
        pool_size: 1
      )

    Tds.query!(pid, "create table #date_test (x date)", [])

    Tds.query!(pid, "insert into #date_test values (@param1)", [
      %Tds.Parameter{name: "@param1", value: nil}
    ])
    # ** (Tds.Error) Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.

    # above is same as this:
    Tds.query!(pid, "insert into #date_test values (@param1)", [
      %Tds.Parameter{name: "@param1", type: :binary, value: nil}
    ])
    # ** (Tds.Error) Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.

    # setting the type as :string works:
    Tds.query!(pid, "insert into #date_test values (@param1)", [
      %Tds.Parameter{name: "@param1", type: :string, value: nil}
    ])
  end
end

Main.main()

And so type: :string seems like a better default.

Before this patch we were sending them as type `:binary`:

```elixir
Mix.install([
  {:tds, "2.3.5"}
])

defmodule Main do
  def main do
    {:ok, pid} =
      Tds.start_link(
        hostname: "localhost",
        username: "sa",
        password: "secret1@A",
        database: "mix_install_examples",
        port: 1433,
        pool_size: 1
      )

    Tds.query!(pid, "create table #date_test (x date)", [])

    Tds.query!(pid, "insert into #date_test values (@param1)", [
      %Tds.Parameter{name: "@param1", value: nil}
    ])
    # ** (Tds.Error) Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.

    # above is same as this:
    Tds.query!(pid, "insert into #date_test values (@param1)", [
      %Tds.Parameter{name: "@param1", type: :binary, value: nil}
    ])
    # ** (Tds.Error) Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.

    # setting the type as :string works:
    Tds.query!(pid, "insert into #date_test values (@param1)", [
      %Tds.Parameter{name: "@param1", type: :string, value: nil}
    ])
  end
end

Main.main()
```

And so `type: :string` seems like a better default.
@rschenk
Copy link

rschenk commented Jun 7, 2024

The tests are giving you a false positive because they don't exercise the TDS.Parameter.prepare_params/1 function that you changed.

I don't have push access, but add this test to test/binary_test.exs

  test "Binary NULL Types Without An Explicit :type", context do
    query("DROP TABLE bin_test", [])

    query(
      """
        CREATE TABLE bin_test (
          char char NULL,
          varchar varchar(max) NULL,
          nvarchar nvarchar(max) NULL,
          binary binary NULL,
          varbinary varbinary(max) NULL,
          uuid uniqueidentifier NULL
          )
      """,
      []
    )

    params_no_type =
      [
        %Parameter{name: "@1", value: nil},
        %Parameter{name: "@2", value: nil},
        %Parameter{name: "@3", value: nil},
        %Parameter{name: "@4", value: nil},
        %Parameter{name: "@5", value: nil},
        %Parameter{name: "@6", value: nil}
      ]
      |> Parameter.prepare_params()  # <-- exercise the changes in this PR

    assert :ok =
             query(
               """
               INSERT INTO bin_test
               (char, varchar, nvarchar, binary, varbinary, uuid)
               VALUES (@1, @2, @3, @4, @5, @6)
               """,
               params_no_type
             )

    assert [[nil, nil, nil, nil, nil, nil]] = query("SELECT TOP(1) * FROM bin_test", [])
  end

@wojtekmach
Copy link
Member Author

I've spoken with @rschenk and yes, the binary type is a problem, we would not be able to set null values to it which is a deal breaker especially when thinking about people using UUID as :binary_id type.

For posterity, I created a little script below that tries to asses the impact of changing the default type with regards to setting nils:

# $ docker run -e "ACCEPT_EULA=Y" -e 'MSSQL_SA_PASSWORD=Secret1@' -p 1433:1433 \
#     --platform linux/amd64 \
#     mcr.microsoft.com/mssql/server:2022-latest
Mix.install([
  {:tds, "~> 2.3"}
])

defmodule Main do
  def main do
    Logger.configure(level: :none)

    {:ok, pid} =
      Tds.start_link(
        username: "sa",
        password: "Secret1@",
        database: "master",
        port: 1433
      )

    # https://learn.microsoft.com/en-us/sql/t-sql/data-types/data-types-transact-sql?view=sql-server-ver16#data-type-categories

    Tds.query!(
      pid,
      """
      CREATE TABLE #test_types (
        int int,
        bigint bigint,
        bit bit,
        decimal decimal,
        money money,

        float float,
        real real,

        date date,
        time time,

        char char,
        varchar varchar,
        text text,

        binary binary,
        varbinary varbinary,

        geography geography
      )
      """,
      []
    )

    names = ~w[
      int
      bigint
      bit
      decimal
      money
      float
      real
      date
      time
      char
      varchar
      text
      binary
      varbinary
      geography
    ]

    f = fn type ->
      for name <- names do
        sql = "insert into #test_types (#{name}) values (@value)"
        params = [%Tds.Parameter{name: "@value", type: type, value: nil}]

        case Tds.query(pid, sql, params) do
          {:ok, _} ->
            IO.puts("(#{type}) #{name} ok")

          {:error, %Tds.Error{message: nil, mssql: %{msg_text: message}}} ->
            IO.puts("(#{type}) #{name} error: #{message}")
        end
      end
    end

    IO.puts("type: :string")
    f.(:string)
    IO.puts("\ntype: :binary")
    f.(:binary)
  end
end

Main.main()
type: :string
(string) int ok
(string) bigint ok
(string) bit ok
(string) decimal ok
(string) money ok
(string) float ok
(string) real ok
(string) date ok
(string) time ok
(string) char ok
(string) varchar ok
(string) text ok
(string) binary error: Implicit conversion from data type nvarchar to binary is not allowed. Use the CONVERT function to run this query.
(string) varbinary error: Implicit conversion from data type nvarchar to varbinary is not allowed. Use the CONVERT function to run this query.
(string) geography ok

type: :binary
(binary) int ok
(binary) bigint ok
(binary) bit ok
(binary) decimal ok
(binary) money ok
(binary) float error: Operand type clash: varbinary is incompatible with float
(binary) real error: Operand type clash: varbinary is incompatible with real
(binary) date error: Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.
(binary) time error: Implicit conversion from data type varbinary to time is not allowed. Use the CONVERT function to run this query.
(binary) char ok
(binary) varchar ok
(binary) text error: Operand type clash: varbinary is incompatible with text
(binary) binary ok
(binary) varbinary ok
(binary) geography ok

@mjaric
Copy link
Member

mjaric commented Jun 10, 2024

I'm not sure why this must be done, and I would choose to support :binary_id over anything else.

Please check my response to #124 to be precise this particularly it explains WHY this can't be solved.

Defaulting this to "string" would mean that type is nvarchar, where 4 bytes are used to describe NULL value (terminator) or if not PLP, zero length of string. That will cause CHAR, VARCHAR, TEXT, BINARY and VARBINARY to fail.

@wojtekmach
Copy link
Member Author

Closing since we shouldn't break :binary_id and other types. Thanks everyone.

@wojtekmach wojtekmach closed this Jun 11, 2024
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.

Implicit conversion from data type varbinary to date is not allowed
3 participants