jdupont commented 3 years ago

Describe the bug

We're seeing type conversion exceptions between database columns and get-only properties, even though Insight doesn't actually populate get-only properties with database values. Specifically, if a get-only property maps to a column in table, and the column's type isn't compatible with the property's type, InsightDb will throw an exception. But, if the types are compatible, InsightDb won't populate the get-only property with the database values. It seems like the mapping shouldn't be throwing an exception since there won't be any assignment happening.

We've been upgrading from v4.1.4 to v6.3.6, and v4.1.4 didn't throw these exceptions for get-only properties (not that it really narrows down the range of changes much). This issue isn't blocking our update, but it seems potentially surprising since the mapping exceptions should be consistent with property assignment.

Also -- Jon's quick fix on this previous issue really helped us out on this upgrade, so thanks again for that!

Steps to reproduce

Unit test reproducing the exception is in this commit.

The specific incompatible types in the unit test aren't really relevant -- this seems to happen for any pairs of incompatible types.

Expected behavior

No type-mapping exception for get-only properties, even if property type is incompatible with column type. Insight won't assign values from the column to the get-only property, so the incompatibility doesn't seem like it should cause an exception.

Dotnet version: dotnet48
Database: SQL Server
Library version: 6.3.6

jonwagner commented 3 years ago

Actually - insight WILL populate get only properties using the internal implementation of the setter/field.

This is to support immutable object reads from the database.

So…

we can fix type conversion issues
you can omit the fields in your query
you can tag the fields with an attribute to have them ignored

On Jun 17, 2021, at 3:32 PM, Jules Dupont @.***> wrote:

Describe the bug

We're seeing type conversion exceptions between database columns and get-only properties, even though Insight doesn't actually populate get-only properties with database values. Specifically, if a get-only property maps to a column in table, and the column's type isn't compatible with the property's type, InsightDb will throw an exception. But, if the types are compatible, InsightDb won't populate the get-only property with the database values. It seems like the mapping shouldn't be throwing an exception since there won't be any assignment happening.

We've been upgrading from v4.1.4 to v6.3.6, and v4.1.4 didn't throw these exceptions for get-only properties (not that it really narrows down the range of changes much). This issue isn't blocking our update, but it seems potentially surprising since the mapping exceptions should be consistent with .

Also -- Jon's quick fix on this previous issuehttps://github.com/jonwagner/Insight.Database/issues/456 really helped us out on this upgrade, so thanks again for that!

Steps to reproduce

Unit test reproducing the exception is in this commithttps://github.com/jonwagner/Insight.Database/commit/e22d92ee21e3586588750ecd1e3cef15c3621dbd.

In general, seems to happen SADFSDF

Expected behavior

No type-mapping exception for get-only properties, even if property type is incompatible with column type. Insight won't assign values from the column to the get-only property, so the incompatibility doesn't seem like it should cause an exception.

Dotnet version: dotnet48
Database: SQL Server
Library version: 6.3.6

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/issues/459, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAMTO5BUWQ2YCNLTQFE5JFDTTJEUDANCNFSM464JQYQA.

jdupont commented 3 years ago

I might be missing something, but it doesn't seem like Insight is populating get-only properties? For example, this test will pass:

[Test]
public void GetOnlyProperty()
{
    try
    {
        Connection().ExecuteSql(
            @"CREATE TABLE [ItemTable](
                [ItemName] [varchar](50) NOT NULL
            );
            INSERT INTO [ItemTable] ([ItemName]) VALUES ('My Item Name');");

        var nullName = Connection().QuerySql<MyItem>("SELECT [ItemName] FROM [ItemTable];");
        Assert.Null(nullName.First().ItemName);
    }
    finally
    {
        Connection().ExecuteSql("DROP TABLE [ItemTable]");
    }
}

private class MyItem
{
    public string ItemName { get; }
}

And then adding a setter to ItemName causes Insight to populate that property with the value My Item Name from the database.

I definitely agree that all of the fixes you suggested mitigate the type mapping exceptions, so this definitely isn't a critical issue. It mostly just feels like the behavior of the mapping is inconsistent with how data is actually being populated.

jonwagner commented 3 years ago

Or i could have forgotten what it actually does. It’s been a while. :) i’ll grab some time over the weekend.

On Jun 17, 2021, at 4:15 PM, Jules Dupont @.***> wrote:

I might be missing something, but it doesn't seem like Insight is populating get-only properties? For example, this test will pass:

[Test] public void GetOnlyProperty() { try { Connection().ExecuteSql( @"CREATE TABLE [ItemTable]( [ItemName] varchar NOT NULL ); INSERT INTO [ItemTable] ([ItemName]) VALUES ('My Item Name');");

    var nullName = Connection().QuerySql<MyItem>("SELECT [ItemName] FROM [ItemTable];");
    Assert.Null(nullName.First().ItemName);
}
finally
{
    Connection().ExecuteSql("DROP TABLE [ItemTable]");
}

}

private class MyItem { public string ItemName { get; } }

And then adding a setter to ItemName causes Insight to populate that property with the value My Item Name from the database.

I definitely agree that all of the fixes you suggested mitigate the type mapping exceptions, so this definitely isn't a critical issue. It mostly just feels like the behavior of the mapping is inconsistent with how data is actually being populated.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/issues/459#issuecomment-863533887, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAMTO5CMM5O4ZYXHQAVW7VLTTJJVFANCNFSM464JQYQA.

jonwagner commented 3 years ago

Alright, I refreshed my memory....

public int X { get; private set; } // creates a private setter which Insight has access to
public int X { get; } // doesn't create a setter. there is a backing field called __k_backing_X or something (Insight does not use)

Either way, the mapping engine is performing the read from the recordset and then attempting type conversion. If there's no setter (as in the second case) this is at least a minor performance loss, as no field is set.

Looking for the right place to fix this.

jonwagner commented 3 years ago

This will be a little tricky to fix. The best place is in ClassDeserializerGenerator.CreateClassDeserializerDynamicMethod.

Columns can map to either class fields/properties or constructor arguments.

Currently, it's implemented as:

Get all mappable columns and convert them to target types. Store in local variables.
Stack any local variables matching constructor arguments. Call the constructor.
Loop through all settable properties and store them to the correct property/field.

Since the column maps to the get property it is eligible for constructor setting. We'll have to do some refactoring in that method. But since it's the core IL generator it will take some careful work.

jdupont commented 3 years ago

Ah OK. And we can't filter out the get-only properties during the mappable column step? Like in ClassPropInfo.SearchForMatchingField?

jonwagner commented 3 years ago

This is fixed in 6.3.7

jonwagner / Insight.Database

Type Conversion Exceptions with Get-Only Properties #459

Describe the bug

Steps to reproduce

Expected behavior