DataAction / AdoNetCore.AseClient

AdoNetCore.AseClient - a .NET Core DB Provider for SAP ASE
Apache License 2.0
106 stars 44 forks source link

[linq2db] Provider returns byte[1]{0x0} value for image type case, where native provider returns NULL #62

Closed MaceWindu closed 6 years ago

MaceWindu commented 6 years ago

Opposite case of #57

Assert.That(conn.Execute<byte[]>("SELECT @p", DataParameter.Image("p", new byte[0])), Is.EqualTo(null));

linq2db test: SybaseTests.TestBinary

senseibaka commented 6 years ago

Hi @MaceWindu sorry for the late reply.

I've been unable to reproduce this. Can you provide a reproduction of this issue without using linq2db (i.e. using only standard ado.net calls)?

Cheers

MaceWindu commented 6 years ago

Will check it tomorrow.

MaceWindu commented 6 years ago

Ok, checked what we do under the hood to execute such query:

using (var cmd = conn.CreateCommand())
{
    cmd.CommandText = "SELECT @p";
    var p = cmd.CreateParameter() as Sybase.Data.AseClient.AseParameter;
    p.ParameterName = "@p";
    p.AseDbType = Sybase.Data.AseClient.AseDbType.Image;
    p.Value = new byte[0];
    cmd.Parameters.Add(p);

    using (var rd = cmd.ExecuteReader())
    {
        rd.Read();
        var res = rd.GetValue(0);
        Assert.True(res is DBNull);
    }
}

for AdoNet provider just change namespace. This code works the same way for nuget version, but return byte[]{0} for version from master (due to this fix I assume: https://github.com/DataAction/AdoNetCore.AseClient/pull/61)

What I noticed is that native provider use different enum values for AseDbType.Binary and Image, and your provider use the same values so you cannot distinguish them in your code.

senseibaka commented 6 years ago

Ah, I see. We don't distinguish between Binary and Image because there doesn't seem to be much point in doing so (i.e. they're both byte arrays, why not treat them the same under-the-hood?). As a result, our implementation of AseDbType just sets the values to a suitable DbType, it's not a 1-to-1 mapping so you get the issue of not being able to distinguish Binary and Image.

In the general case, this is fine, but in the edge-cases, it causes a discrepancy between our driver and the reference driver. Therefore, it's probably a drop-in-replacement bug, but a fairly minor bug.

I've been messing around with this a bit yesterday/today and I think to solve this will require a bit of work.. some of the steps are:

  1. Write more tests which run against the reference driver and our own. One example would be to have tests that show an AseParameter constructed with a particular AseDbType emits the correct value for the DbType property.
  2. Separate AseDbType from DbType and introduce a translation between them.
  3. Revisit internal logic around building parameters/values for transmission, and see if this can be changed to suit.

I'd say due to the effort required vs the severity, this is a case of may fix eventually. @sheikhjabootie or @formicas may dispute though ;) (ping)

senseibaka commented 6 years ago

Fix will be in the next release

MaceWindu commented 6 years ago

Fixed