Implicit conversion from data type varbinary to date is not allowed

[puruzio]

Repo.Update fails when the changeset includes a nil date value with an intent to update a date field to set to null. The error message states:

[2021-07-06 16:34:24.100] [debug] Line 1 (Error 8180): Statement(s) could not be prepared.

** (Tds.Error) Line 1 (Error 257): Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.

    (ecto_sql 3.6.1) lib/ecto/adapters/sql.ex:749: Ecto.Adapters.SQL.raise_sql_call_error/1

    (ecto 3.6.1) lib/ecto/repo/schema.ex:717: Ecto.Repo.Schema.apply/4

    (ecto 3.6.1) lib/ecto/repo/schema.ex:426: anonymous fn/15 in Ecto.Repo.Schema.do_update/4

The changeset looks like

#Ecto.Changeset<
   action: nil,
   changes: %{date_completed: nil},
   errors: [],
   data: #MyApp.MyModel<>,
   valid?: true
>

[markquezada]

@puruzio did you ever figure out a fix? We're running into the same issue

[rschenk]

I did some digging into this and wrote a failing test for it. The issue is caused by Tds.Parameter.fix_data_type/1 which casts a nil parameter to a :binary type. This existing approach works for every commonly-used column type, except dates. (Weirdly, even datetimes work with the existing code, but not dates!)

A quick way to almost fix this is to change the function linked above from :binary to :string. This works for every commonly-used column type, except varbinary, which explodes with a similar error.

I should caveat the following by saying that I know almost nothing about the guts of the TDS protocol.

That being said, I believe the proper way to fix this is to introduce a :null parameter type. TDS has a null type (see Tds.Types@tds_data_type_null) and this is being decoded when it comes out of the database in Tds.Types.decode_data/2. There unfortunately is no corresponding logic to encode it back into the db.

I think we need to update Tds.Parameter.fix_data_type to introduce a null parameter type:

  def fix_data_type(%__MODULE__{type: nil, value: nil} = param) do
    %{param | type: :null}
  end

Then add that :null to the case statement in Tds.Types.encode_data_type and add a corresponding helper method to handle that case. The correct implementation of the latter is a bit beyond me since I don't actually know TDS.

Finally the real nitty gritty, Tds.Types.encode_data/3 will need to actually put that null type as bits on the wire, which I don't know how to do.

I can hopefully dig into this a little more when I get some time, or otherwise, perhaps someone who knows the intricacies of Ecto guts and the TDS protocol can use what I've sleuthed out and make this change (assuming my suggested change is on the right track).

[markquezada]

@moogle19 @mjaric Hi guys. Wondering if you could weigh in on the above as this is a major pain point for us right now. We'd be happy to create a PR to address this issue but need some guidance about the right way to solve it. Could you weigh in on the above and let us know if we're on the right track here?

[markquezada]

cc: @ewitchin 👆

[mjaric]

@markquezada or @rschenk could you please create simple repo that can I check? Thing is that ecto is layer above tds, and there are a lot of points where this could go wrong. Maybe fix could be done in Ecto.Tds.Adapter instead of in Tds driver.

[rschenk]

Hi @mjaric sorry for the delay, I've made a repo here: https://github.com/rschenk/ecto_tds_null_dates

[markquezada]

@mjaric just making sure you saw the above ☝️

[mjaric]

Tnx. I will check it

[mlooney]

Hi folks, any news on this issue? I bonked into it today. I was able to work around it by changing the column type to string, but i suspect i'm losing some storage there. any other workarounds that make more sense?

[rschenk]

@mlooney I've not heard any news myself. I made a repo illustrating the problem with a failing test, linked above, but have not heard about any progress towards fixing it

[mjaric]

Any attempt to make nil be something else by default will end up with some other failed conversion. I strongly suggest that anyone who experience this issue, instead use type function from Query API.

If you want to know why is this, here is a bit detailed explanation, and it all relates to INPUT parameters and may affect output parameters also, but rarely.

Back then, when ecto was young lib, it was built to support postgresql, then mysql and both backends have ability to tell driver (client lib):

Hey this parameters you put in your query, they need to be of this and that db type

Then TDS came, with all its shame not being honest to your driver. So you had to explicitly specify the type for your input parameter. Prepare statement does not "conclude" of what db type parameter should be, it is dev responsibility.... We tried to make assumptions, yet again there is always edge case that will not work! Simply, SqlServer typing sistem is so strict that some conversions is not possible to do, like you see in PR #162. Now this is when you use parameters, if you don't, then string (true for VARCHAR and not NVARCHAR) usually can be converted to any other type (again almost 🙄, but less edge cases... read it "string encoding issues"). But fear SQL injection, you don't want to do this and ecto will not allow it except, maybe you use framgent macro!!!!

Back to the nil issue, it is impossible to solve it without type hint in query, nil is atom, and with this strict conversions it is not possible to assume what is correct db type for parameter. So please use type(my_date, :datetime) in your query. That will work even for if value is nil.

It is probably annoying, but as long as you keep your queries under control/ not scattered around, you should be fine. Make reusable fragments of query.

[mjaric]

One more thing, please 🙏 don't rely o implicit conversion, sometimes it may make your queries ro spil in TEMP leading to slow execution!

[abueloshika]

I'm pretty new to Elixir and Ecto so I may have misunderstood, but wouldn't having to use type(my_date, :date) on things like update operations mean we would have to do updates via a composed query and lose the ability to use changesets for things like casting and validations?

[mjaric]

I'm pretty new to Elixir and Ecto so I may have misunderstood, but wouldn't having to use type(my_date, :date) on things like update operations mean we would have to do updates via a composed query and lose the ability to use changesets for things like casting and validations?

When working with models (Schema) and using Ecto.Changeset.cast, we don't need to worry about types because they are declared in the schema. During casting, the function will call Ecto.Changeset.process_param and assign the correct types, with assistance from Ecto adapters (such as Ecto.Adapters.Tds in this case). Even if there are edge cases, you can create custom types, as we did with UUID and Varchar. It's worth noting that schemaless changesets also work since you need to specify the field types anyway.

There are two scenarios where issues can arise:

• Read Queries: When schema information is not available in the AST, such as when using fragments or schemaless queries.
• Direct Usage of the :tds Library: When you want the library to infer the type without explicit declaration.

[mjaric]

Just to clarify, this issue is fundamentally challenging due to the flexibility of the typing system in Elixir. In the TDS protocol, we must allocate storage for a type even if its value is NULL. The size of NULL storage varies depending on the type, making it crucial to specify the correct type in your parameters or allow :tds to infer it for you.

Storage Definition in TDS

1. Token Initialization:

   • Fixed-length tokens typically include the length of the value within the token code itself as a single byte.
   • Variable-length tokens are followed by a length prefix (2 or 4 bytes, depending on the type).
   • For PLP (Partially Length Prefixed) types, the "null termination" size is specific (usually 2 or 4 bytes).

2. Variable Length Tokens (Types):

   • CHAR, VARCHAR, TEXT, VARBINARY, BINARYuse 2 bytes to store a NULL value (0x0000).
   • NCHAR, NVARCHAR, NTEXT use 4 bytes to store a NULL value (0x00000000).

3. Fixed Size Types:

   • Types such as int, bigint, etc., use a 1-byte flag (0x00) to indicate NULL, meaning there is no data to parse after the type code.

Summary

• Variable-length types require different storage sizes for NULL values.
• Fixed-size types use a simple 1-byte flag for NULL.

Due to these variations, it is impossible to directly map Elixir’s nil to these different representations. Any attempt to infer the database type from nil would likely introduce new bugs.

[wojtekmach]

In hindsight, this issue should probably have been tracked on elixir-ecto/ecto_sql because this comes up when using Ecto with Tds. As mentioned above, if someone uses Tds they are able to set the proper types.

There are two scenarios where issues can arise:
Read Queries
Direct Usage of the :tds Library

I believe by far the most common issue is people using changesets to update to nil for date type (and all the other calendar types as well as float and potentially others. See #162 (comment) for a "test rig").

Here is a minimal end-to-end example with Ecto SQL:

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

Application.put_env(:myapp, Repo, url: "mssql://sa:secret1!A@localhost/mix_install_examples")

defmodule Repo do
  use Ecto.Repo, adapter: Ecto.Adapters.Tds, otp_app: :myapp
end

defmodule Migration do
  use Ecto.Migration

  def change do
    create table("posts") do
      add(:date, :date)
    end
  end
end

defmodule Post do
  use Ecto.Schema

  schema "posts" do
    field(:date, :date)
  end
end

defmodule Main do
  import Ecto.Query, warn: false

  def main do
    children = [Repo]
    _ = Repo.__adapter__().storage_down(Repo.config())
    _ = Repo.__adapter__().storage_up(Repo.config())
    {:ok, _} = Supervisor.start_link(children, strategy: :one_for_one)
    Ecto.Migrator.run(Repo, [{0, Migration}], :up, all: true)

    post = Repo.insert!(%Post{date: ~D[2024-01-01]})

    Ecto.Changeset.change(post, date: nil) |> Repo.update!()
    # ** (Tds.Error) Line 1 (Error 257): Implicit conversion from data type varbinary to date is not allowed. Use the CONVERT function to run this query.
  end
end

Main.main()

Any attempt to infer the database type from nil would likely introduce new bugs.

100% agreed.

it is impossible to solve it without type hint in query

I think so too.

I believe we have two options.

Option 1. We get known types from changesets/queries and send them to Tds. Here's an early proof of concept:

• https://github.com/elixir-ecto/ecto/compare/wm-types
• https://github.com/elixir-ecto/ecto_sql/compare/wm-types

Option 2. We allow users to set option on changesets/queries with either specifically how nulls should be handled or perhaps more broadly with type hints.

@mjaric what do you think about these possible solutions?

[mjaric]

I believe by far the most common issue is people using changesets to update to nil for date type (and all the other calendar types as well as float and potentially others. See #162 (comment) for a "test rig").

Rig will never work unfortunately, even in if you directly type query into some DB IDE simply because implicit conversation don't work on some-to-any basis, here is the link that documents implicit conversation matrix (I'm attaching matrix also below)

None of db types can be implicitly converted to all other types.

I believe we have two options.

Option 1. We get known types from changesets/queries and send them to Tds. Here's an early proof of concept:

• https://github.com/elixir-ecto/ecto/compare/wm-types
• https://github.com/elixir-ecto/ecto_sql/compare/wm-types

Option 2. We allow users to set option on changesets/queries with either specifically how nulls should be handled or perhaps more broadly with type hints.

I think we only need Option 1, since schemaless changeset requires types to be set too. Question is, if there are code paths that could ignore the type declaration from ecto toward tds driver prepare params function.

[mjaric]

Hmm, this is probably the source of issues, it is first match and ignores type if value is nil, so next overload makes it always of binary type.

Looks like @rschenk was on right track.

I'll make new PR with making sure we at least follow the matrix above in first run, then I'll check MS TDS docs to see how to properly encode with exact token that is specified on schema (or both if it is not risky change)