Bug: Exceptions are thrown when using a custom PropertyHandler and a Where Expression to process an Enum value (that has NULL or invalid Enum Id); the Get/Set is never invoked.

[cajuncoding]

Bug Description

When using a Property Handler and a Where Expression (Lambda filtering using the Model) to process an Enum value (that may be NULL or invalid/out-of-range Enum Id) the Get/Set are never called because an exception is raised while parsing the Enum; it appears this happens before the property handler is ever called. So the property Handler cannot be used (as expected) to override the RepoDb behvior and provide custom parsing/mapping logic to handle the edge cases.

This precludes the ability to use the Property Handler to correct the values and override the default parsing behavior of RepoDb.

Our use case specifically has an Enum LinkType that only has values 1,2,3 (see below), but the DB has records with values of NULL and 0 (Zero). Therefore these records fail to de-serialize into the Model; both with the same error message.

In many cases this strict validation is great, and points to the the fact that we likely have bad data in our DB. But none-the-less for backwards compatibility there are many valid use-cases where it's critical to offer the flexibility for the consumer to control this.

And, ideally this would be done in a Property Handler whereby no additional validation from RepoDb is done. RepoDb should allow the PropertyHandler to completely control how a value from the DB is Mapped to/from the Model and the table without any exceptions being thrown outside of the PropertyHandler.

Exception Message:

System.ArgumentNullException : Value cannot be null.
Parameter name: value
   at System.Enum.EnumResult.SetFailure(ParseFailureKind failure, String failureParameter)
   at System.Enum.TryParseEnum(Type enumType, String value, Boolean ignoreCase, EnumResult& parseResult)
   at System.Enum.Parse(Type enumType, String value, Boolean ignoreCase)
   at RepoDb.QueryField.Parse[TEntity](BinaryExpression expression)
   at RepoDb.QueryGroup.Parse[TEntity](BinaryExpression expression)
   at RepoDb.QueryGroup.Parse[TEntity](Expression expression)
   at RepoDb.QueryGroup.Parse[TEntity](Expression`1 expression)
   at RepoDb.DbConnectionExtension.QueryAsync[TEntity](IDbConnection connection, String tableName, Expression`1 where, IEnumerable`1 fields, IEnumerable`1 orderBy, Nullable`1 top, String hints, String cacheKey, Nullable`1 cacheItemExpiration, Nullable`1 commandTimeout, IDbTransaction transaction, ICache cache, ITrace trace, IStatementBuilder statementBuilder, CancellationToken cancellationToken)

Schema and Model:

Please share to us the schema of the table (not actual) that could help us replicate the issue if necessary.

CREATE TABLE [dbo].[RepoDbTests](
	[Id] [int] IDENTITY(1,1) NOT NULL,
	[StringVal] [nvarchar](max) NULL,
	[LinkType] [int] NULL,
	CONSTRAINT [PK_Identity_Id] PRIMARY KEY CLUSTERED (	[Id] ASC )
		WITH (STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY]

And also the model that corresponds the schema.

    public class RepoDbTestItem
    {
        public int Id { get; set; }
	public StringVal { get; set; },
        //TODO: FAILING, this Property Handler is not intercepting as expected, Exceptions are still thrown before our Handler Get/Set is invoked...
        [PropertyHandler(typeof(LinkTypeEnumPropertyHandler))]
        public LinkType LinkType { get; set; }
    }

    public enum LinkType
    {
        //NOTE: There is NO Zero Value here...
        CampaignLink = 1,
        EventDetailLink = 2,
        EventNotLinked = 3
    }

    public class LinkTypeEnumPropertyHandler : IPropertyHandler<int?, LinkType>
    {
        //LOOSELY Parse LinkType Enum without strict validation...
        public LinkType Get(int? input, ClassProperty property) => (LinkType)(input ?? 0);

        //LOOSELY Parse LinkType Enum without strict validation...
        public int? Set(LinkType input, ClassProperty property) => (int)input;
    }

Library Version:
Example: RepoDb v1.12.9 and RepoDb.SqlServer v1.1.4

[mikependon]

@cajuncoding - we have created an Integration Test to guard this scenario, but on such Integration Test, this scenario is not failing.

We (me personally) can able to intercept the Property Handlers even the values of the field from the database is NULL or OutOfEnumRange. We tested it on the columns with NVARCHAR and BIT data types and the corresponding class properties are Nullable<EnumType>. We are using the latest code branch from the main.

We will create a small project that would simulate this using the latest releases at Nuget. We will share it here afterwards.

[mikependon]

@cajuncoding - here, we have simulated your scenarios and the issues are not occuring here. Would you be able to test it from here?

For it to run, create a local DB named TestDB and run the scripts residing in the Database folder.

The following will be covered.

• Insert the normal values
• Insert the null values via the models
• Insert the null values via explicit path (using anonymous)
• Insert the out-of-range values.

The model in topic has 2 nullable properties that is linked to a one enumeration. Both are targetting the NVARCHAR and INT data type from the database.

Please do reshare this small solution if you can able to replicate your issue here. Thanks!

Issue991.zip

[mikependon]

BTW, we used the latest deployed version of RepoDB on the smal project we created - not the main branch.

[cajuncoding]

@mikependon

Ok, I was finally able to re-factor the repro solution, you provided, to match my own repro poc (with minimal changes I believe).... and finally got it to fail as I was expecting (initially I thought i tmight be that I'm on .Net v4.8 Framework, etc. but now it's failing the same way in the repro attached.

I WAS able to get it working when using QueryAll() in our code base so that narrows it down a little I hope, to failing only when passing in an Expression for filtering (which is critical for us to use RepoDb in a compabile whay with our legacy code).

One of the key differences is that our Legacy code Models did not allow LinkType to be nullable, so the Property Handler is expected to be able to map that to never be a null value, and the legacy ORM we're migrating from did this by defaulting to Zero. So my goal is for the Property Handler to allow me to provide that same behavior. And it's Working perfectly when we do a QueryAll(), but NOT when we specify a filtering where expression.

I'll def. keep an eye on the thread and/or Gitter in case you need anything else!

Failure in Console:

Source Code Attached:
Issue991-RefactoredToFailAsExpected.zip

WOOPS, looks like I left my Temp DB Connection String in the project; it's already been reset now so that connection is invalid.

[mikependon]

Thanks @cajuncoding - we missed that the exception is being triggered on the Expression parsing, and not on the hydration of the model. That would hopefully simplify the fix. We will give you an update on this soon. This is our priority.

[mikependon]

The fix is now available at RepoDb v1.12.10-beta2 and RepoDb.SqlServer v1.1.5-beta2. Please do not hesitate to revert if the issue still persists on the said version.

[cajuncoding]

@mikependon Thanks for pushing the Fix. Good news is that the exceptions are definitely resolved, and the PropertyHandler is executing as expected so our interception code works well now! However, the results are not as expected (translated into incorrect SQL)...

I wasn't seeing expected results from our database because the where expression (using a raw int that is out of range):
sqlConnection.QueryAsync<T>(i => i.LinkType == 0)

Is being translated into the raw SQL:
SELECT [Id], [LinkType] FROM [TestTable] WHERE ([LinkType] IS NULL)

Why would and expression testing for Zero result in an IS Null in the raw SQL? If LinkType was nullable then this would make sense and I'd expect the Expression i => i.LinkType == null to translate to this (if LinkType is nullable)... But not if an Enum casted int is given.

So currently the wrong results may occur for the out-of-range int values (but are still very valid .Net values).

So it is working correctly to generate the expected SQL for in-range Enum values such as: ✅

• sqlConnection.QueryAsync(i => i.LinkType = LinkType.One) ==> WHERE ([LinkType] = @LinkType)
• sqlConnection.QueryAsync(i => i.LinkType = (LinkType)1) ==> WHERE ([LinkType] = @LinkType)

However, the expression is translated to IS NULL for out-of-range values such as the following: ⚠

• sqlConnection.QueryAsync(i => i.LinkType == 0) ==> WHERE ([LinkType] IS NULL)
• sqlConnection.QueryAsync(i => i.LinkType == (LinkType)0) ==> WHERE ([LinkType] IS NULL)
• sqlConnection.QueryAsync(i => i.LinkType == (LinkType)100) ==> WHERE ([LinkType] IS NULL)
• For fun I even tried jumping through some hoops: i => i.LinkType == (LinkType)Enum.ToObject(typeof(LinkType), 0) ==> WHERE ([LinkType] IS NULL) 😆 (worth a shot)

[mikependon]

We do understand this as we purposely return the null on the recent fix if it is out of bounds. This is an edge-case as the dirty records are persistent to the DB. This will never ever happen if RepoDB or EF is the ORM, I had supposed. We will fix this very soon.

BTW, why not just add a new entry in your Enum for the value 0 as a recrification?

[mikependon]

Just FYI, here is the problem on the code as per your use-case. We will do issue you a new beta later.

The fix for this would be below, in which it forces to utilize the value argument if the parsing has failed with the given enum value (i.e.: 0, NULL, or OutOfRange).

private static object ToEnumValue(Type enumType,
     object value) =>
     (value != null ? ToEnumValue(enumType, Enum.GetName(enumType, value)) : null) ?? value;

But, we need to capture the scenario by Integration Tests to guard it properly.

[cajuncoding]

@mikependon yeah, the current beta definitely unblocked us. So that was great! I just wanted to highlight that there are some odd behaviours on these edge cases. Especially since .Net behaves this way and allows and Enum to be cast to any int.

We don't actually filter by the Enum in the business logic (that I'm aware of yet), but I had added some integration tests on our side that does as a validation of our migration to RepoDb.

I'm pretty sure that we will add an Enum UNKNOWN = 0, and will probably clean up our data in our table too. But, it's always good to have RepoDb behave intuitively and consistently with .Net 👍

Thanks very much for the help! 👏👏👏

[mikependon]

We have push a new build that does contain a targeted fix on the issue you last mentioned here. Would you be able to update your local referenced packages to the following versions (RepoDb v1.12.10-beta3, RepoDb.SqlServer v1.1.5-beta3)? Please do not hesitate to revert if you still encountered the problems.

[cajuncoding]

@mikependon yes the new version has resolved all of our issues and my tests are passing as expected!

All of the rendered raw sql looks good now for various Enum scenarios… I think this will likely help others using Enums eventually :-)

Thanks again for the fast support…RepoDb is awesome and is my go to ORM!

[mikependon]

@cajuncoding - nice to know that the latest release has resolved all your problems.

[mikependon]

@cajuncoding - Enum is always a challenge to us (or maybe in general in ORM space), specially for Npgsql, in which they not handling the coercion to non-custom type in PGSQL. We also currently do the development to support this on the BulkOperations. 😃