search

mikependon / RepoDB

A hybrid ORM library for .NET.
Apache License 2.0
1.68k stars 122 forks source link

Convert enum to type based on DbType for PostgreSQL #1152

Closed PauloHMattos closed 1 year ago

PauloHMattos commented 1 year ago

Attempt to fix #1151

mikependon commented 1 year ago

Related to my old discussion with Shay Rojansky, the author of Npgsql. Link: https://twitter.com/mike_pendon/status/1323998871681441792

mikependon commented 1 year ago

The same logic is alreay present: https://github.com/mikependon/RepoDB/blob/8213ca425f2d3d4f83adc51bf507224c20206e82/RepoDb.Core/RepoDb/Reflection/Compiler/Compiler.cs#L667

Enum handling for Non-String: https://github.com/mikependon/RepoDB/blob/8213ca425f2d3d4f83adc51bf507224c20206e82/RepoDb.Core/RepoDb/Reflection/Compiler/Compiler.cs#L702

Enum handling for String: https://github.com/mikependon/RepoDB/blob/8213ca425f2d3d4f83adc51bf507224c20206e82/RepoDb.Core/RepoDb/Reflection/Compiler/Compiler.cs#L680

PauloHMattos commented 1 year ago

Seens that this treatment is only applied to the fluent methods. Calling Insert to create a row in the database converts the enum to int, but writing the raw SQL does not.

Is this the expected behaviour? I agree that this should probably be supported in npgsql but feels weird that the Insert method converts, while the raw SQL does not.

Anyway, I will bump this in the npgsql repository. Feel free to close this PR and https://github.com/mikependon/RepoDB/issues/1151 if it's not actionable here

mikependon commented 1 year ago

That's a good point actually and also thanks for testing the 2 recommendations from the issue #1151. In this PR, there are failing Integration Tests, can you make that one green and I will approve this right away. Thanks and we really appreciated this PR.

PauloHMattos commented 1 year ago

I tried running the PostgreSQL integrations test but many of then fail (in master and this PR) with this assert:

Assert.AreEqual failed. Expected:<6/28/2023 12:00:00 AM>. Actual:<6/27/2023 12:00:00 AM>. Assert failed for 'ColumnDateAsArray'. The values are 'System.DateTime[] (System.Array)' and 'System.DateTime[] (System.Array)'.

The correct date in my timezone is 6/28/2023. Cheking the row in the database shows the wrong date in this column, but the correct one in column that is not an array. So the error seens to be during writing.

Since the tests pass in the CI and in your machine, it's likely somethin wrong on my environment.

I will keep looking into this here, but do you have any idea to what I'm doing wrong?

Thanks for the help and for this great library.

mikependon commented 1 year ago

Failing integration tests in relation to dates are not connected to your PR, that's for sure. Never I had investigated it yet, but it sounds to me that it is environmental problem as you said (pertained to some sort of timezone). If there is a bug in the Integration Tests code, please feel free to just correct it. We can also do it in the latter part.

PauloHMattos commented 1 year ago

I've ignored the ColumnDateAsArray from the tests and made changes so that all tests pass.

I 've modified so that the conversion only happens when Options.ConvertType is set to Automatic. It differs from the behaviour of the fluent methods, but is enough for my use case and makes the tests pass without changing then.

if you think it should be consistent, we can remove the ConvertType check and change the tests to consider the new behaviour.

Let me know what you think.