Bug: No coercion operator is defined between types 'System.String' and 'System.Guid'.

[davidrot]

Hi, I'm right now verifying RepoDb for my project, but I found a problem that I'm not able to fix on my own. I have this entity I'm trying to insert into my sqlite database but receiving following exception.

No coercion operator is defined between types 'System.String' and 'System.Guid'.
   at System.Linq.Expressions.Expression.GetUserDefinedCoercionOrThrow(ExpressionType coercionType, Expression expression, Type convertToType)
   at System.Linq.Expressions.Expression.Convert(Expression expression, Type type, MethodInfo method)
   at RepoDb.Reflection.FunctionFactory.<>c__DisplayClass6_0`1.<GetDataEntityDbCommandParameterSetterFunction>b__0(Expression instance, ParameterExpression property, DbField dbField, ClassProperty classProperty, Boolean skipValueAssignment, ParameterDirection direction)
   at RepoDb.Reflection.FunctionFactory.GetDataEntityDbCommandParameterSetterFunction[TEntity](IEnumerable`1 inputFields, IEnumerable`1 outputFields, IDbSetting dbSetting)
   at RepoDb.FunctionCache.GetDataEntityDbCommandParameterSetterFunctionCache`1.Get(String cacheKey, IEnumerable`1 inputFields, IEnumerable`1 outputFields, IDbSetting dbSetting)
   at RepoDb.DbConnectionExtension.<>c__DisplayClass242_0`2.<InsertInternalBase>b__0()
   at RepoDb.Contexts.Execution.InsertExecutionContextCache`1.Get(String tableName, IEnumerable`1 fields, Func`1 callback)
   at RepoDb.DbConnectionExtension.InsertInternalBase[TEntity,TResult](IDbConnection connection, String tableName, TEntity entity, IEnumerable`1 fields, String hints, Nullable`1 commandTimeout, IDbTransaction transaction, ITrace trace, IStatementBuilder statementBuilder, Boolean skipIdentityCheck)
   at RepoDb.DbConnectionExtension.InsertInternal[TEntity,TResult](IDbConnection connection, TEntity entity, String hints, Nullable`1 commandTimeout, IDbTransaction transaction, ITrace trace, IStatementBuilder statementBuilder)
   at roda.framework.entity.Upsert.UpsertRepository`1.<>c__DisplayClass6_0.<InsertEntity>b__0(IDbConnection db)

db.ExecuteNonQuery(@"CREATE TABLE IF NOT EXISTS DemoUpsertObj
    (
        Id TEXT PRIMARY KEY,
        EntityId INTEGER,
        Name TEXT NOT NULL,

        ValidFrom Datetime,
        ValidTo  datetime,
        Deleted byte
    );
");

public class DemoUpsertObj : IUpsertEntity
{
    // [PropertyHandler(typeof(GuidToStringPropertyHandler))]
    public Guid Id { get; set; }
    public long EntityId { get; set; }
    public DateTime ValidFrom { get; set; }
    public DateTime ValidTo { get; set; }
    public byte Deleted { get; set; }

    public string Name { get; set; }

    public DemoUpsertObj() { }
    public DemoUpsertObj(string name)
    {
        Name = name;
    }
}

I also tried it with a PropertyHandler and PropertyHandlerMapper.Add(typeof(Guid), new GuidToStringPropertyHandler()); but the functions in GuidToStringPropertyHandler were never called. I'm running it on MacOS 10.13.6 in a dotnet core 2.0 project .

[mikependon]

The reason to this is because your Id column is of type TEXT and your property is of type Guid. Property handler is the best option here.

There is another work around, you can use the Converter.ConversionType = Automatic. But I will not recommend this over Property Handler.

Would you share your PropertyHandler class here?

[davidrot]

Hi Mike, thank you for your reply. The Get/Set function is never called.

public class GuidToStringPropertyHandler : IPropertyHandler<Guid, string>
    {
        public string Get(Guid input, ClassProperty property)
        {
            return input.ToString();
        }

        public Guid Set(string input, ClassProperty property)
        {
            var output = Guid.Empty;
            Guid.TryParse(input, out output);
            return output;
        }
    }

[mikependon]

I will check this one later when I am sitting with my laptop :). Never I tested such scenario in my test suites (Unit/Integration Tests) to put the PropertyHandler in the PrimaryKey or IdentityKey. When do you need this to be fixed?

[davidrot]

Oh I see your point. There's no rush. :) Maybe I start to invest on my own, I havent found the right setup yet to debug easily.

[mikependon]

@davidrot , I would assume that you made it work with Automatic conversion type. Anyway, it seems that your property handler is opposite? Below is the correct handler for your model and DB table schema.

public class StringToGuidPropertyHandler : IPropertyHandler<string, Guid>
{
	public Guid Get(string input, ClassProperty property)
	{
		var output = Guid.Empty;
		Guid.TryParse(input, out output);
		return output;
	}

	public string Set(Guid input, ClassProperty property)
	{
		return input.ToString();
	}
}

Can you test it?

[mikependon]

Your issue was not really a bug due to the opposite ordering of the generic types in your PH. Please use the PH I had shared above. However, during my investigation on your issue, I found out that the PH mappings defined via FluentMapper and PropertyHandlerMapper is not being triggered. So I filed a new bug at #438 in which it is fixed at RepoDb v1.11.1-beta1.

[mikependon]

If you can't make it work in your local machine, then please use this solution for your reference.

[davidrot]

Hi Mike,

thank you for your help and your great work! I tried two things both were working for me. Sadly I found another problem by querying the Guid property.

Converter.ConversionType = RepoDb.Enumerations.ConversionType.Automatic;
FluentMapper.Entity<DemoUpsertObj>().PropertyHandler<StringToGuidPropertyHandler>(e => e.Id);

I have following function, the first Query call is not working, because the == >id< is added as a parameter of type guid, but will be inserted as a string (the handler). Never the less the second Query call is working for me. Would it be the correct way to use the same PropertyHandler for the SQLParameter? I'm not sure about my proposal... If you give me some instructions I can try to implement it and create a PR what you can review.

public T GetByVersionId(Guid id)
{
    var db = ...
        // return db.Query<T>(where: x => x.Id == id).FirstOrDefault();
        return db.Query<T>(id.ToString()).FirstOrDefault();
}

// Edit: I have the same problem with the db.Update(obj).

[mikependon]

I think it is correct that you have to explicitly call the ToString() method due to the fact that the type is in the DB is TEXT. But I understand that your problem is that you can't coerce it properly via Linq Expression due to the fact that the property type of Id is Guid (since GUID is not-equals to STRING/TEXT).

Would it be the correct way to use the same PropertyHandler for the SQLParameter?

You're correct, the property handler is ignore in the expression parsing and it is a good thing to reconsider the already mapped property handler :) . Let me check this one in the code level, and I would be very happy if you can create a PR for this if necessary.

[mikependon]

I have found the area where the code changes must be done. Can you do PR for me for the non-fixed area?

• For Dictionary param - already fixed
• For Object/Dynamic param
• For QueryField param

The possible changes were all residing at DbCommandExtension class.

Note: Make sure to update your local copies before issuing the fix. Also, please be aware that property handler are being identified in order (Attribute Mapping followed by Property Level Mapping and then by Type Level Mapping).

[EDIT]

The changes here will surely affect the performance of the library as the methods are being hit in every iteration/request. Also, it might be affecting huge scenarios in the Integration Tests, let us work together to make everything green.

[mikependon]

@davidrot - I saw you just forked. Can you wait for an hour before doing the code changes? I am currently organizing the structure of Mappers, Cachers and Resolvers. So you would not have conflict in your upcoming PR.

An hour from now, would you be able to update your local copy and start the changes?

[davidrot]

@mikependon Yes I created a fork. No worries it will take a will until i figure out how to implement. :)

[mikependon]

@davidrot - there you go, both my code changes the code organization is done. Would you be able to update your local copy before doing any code changes.

it will take a will until i figure out how to implement.

Just follow the code written on this line. Of course, do not hesitate to revert back to me anytime :)

[mikependon]

@davidrot - I have deployed a package RepoDb v1.11.1-beta2 that would fix your issue when using the PH via Linq Expressions.

However, the 2 remaining items that is related to Query Tree Expression (Dynamic and Query Objects) are not yet fixed. I will be waiting for your PR on that.

[mikependon]

@davidrot - I have deployed a package RepoDb v1.11.1-beta3 including your PR and some of my enhancements. This will address your concerns.

[mikependon]

The fix is now available in version RepoDb v1.11.1. Please revert if you still encounter the issue there. Otherwise, I will close this in a few days.

[calumchisholm]

I know I'm late to this conversation, but I really don't like this.

The problem is that a UUID/GUID isn't just a raw byte array - it's a data structure comprised of 5 records (4-2-2-2-6 bytes). The encoding of the last 2 fields is platform-dependent, and while this will usually be big-endian you can't guarantee this - mixed-endian encodings are possible.

None of this is a problem in the 'normal' case when you just need a unique value - it only becomes an issue when you start casting, serializing or otherwise manipulating GUIDs (or if you need to create them based on known values) and then pass them across the network. I wouldn't be 100% confident that the GUID string representation returned by a database engine running on a little-endian platform would definitely match that generated by Guid.ToString() on a big-endian Windows machine. Of course, it should, but it's not behaviour I would rely on.

Not only would this helper function potentially open up this corner case, but it supports poor practice in the first place (storing GUID values as strings). I would recommend leaving the conversion up to the consumer of the library, or at least add a warning or Obsolete attribute to strongly discourage its use.

Note that this concern is the same reason why System.Guid provides this constructor for when you're potentially passing GUIDs across an endianness boundary:
Guid(UInt32, UInt16, UInt16, Byte, Byte, Byte, Byte, Byte, Byte, Byte, Byte)

[mikependon]

Thank you for your comments. Strongly agree with you in all case here. But this issue is related to a BUG in the library in which it is not triggering the mapped Property Handlers.

In anyway, to answer your concern in the recommendation, below is our take.

I would recommend leaving the conversion up to the consumer of the library, or at least add a warning or Obsolete attribute to strongly discourage its use.

RepoDb does not do auto-conversion from Guid to String (or vice-versa). Like you, I strongly not recommending this because of the additional layer of process of conversion, "PLUS" the reality that it might/will lost precision.

But RepoDb has an option to support this automatically via Converter.ConversionType = Automatic, in which the library adds additional layer-checks of conversion using the default System.Convert class, and again, I am strongly not recommending that the user would use this unless otherwise they are really required. FYI: It affects performance and efficiency.

But since this issue is pertaining to the Property Handlers in which the conversion is handled by the user itself, then the user is responsible for such conversion. With the bugs here, both Push/Pull and Query Expression is covered by our recent fix in RepoDb v1.11.1.

[davidrot]

@mikependon I think I haven't fixed all places in my PR. Please see:
https://github.com/mikependon/RepoDb/blob/master/RepoDb.Core/RepoDb/Extensions/PropertyInfoExtension.cs#L125

I'm preparing a PR for this but I don't know how to fix this. How can I get the ProperyHandler here.
I would suggest to think about a GetValue function what is respecting the PropertyHandler and use this instead of having this code several times in the code.

[mikependon]

Make sense! Please use the PropertyHandlerMapper.Get(property.DeclaringType), property) to get the instance of PropertyHandler.

May I request if you can create an addtional extenstion method in the ProperyInfoExtension class named GetHandledValue(this PropertyInfo propertyInfo, object entity)? Then reuse this method in any GetValue() if needed?

Thank you in advance.

[davidrot]

Thank you i will try!

I also found another issue with an update statement. When I remove this line (https://github.com/mikependon/RepoDb/blob/master/RepoDb.Core/RepoDb/Operations/DbConnection/Update.cs#L954), it is working correctly. With this line, my update statement will not update any line. It looks like the where statement do not match any line. I haven't figured out why... I have only figured out when adding a command parameter within the name a _ char it will be added to the command with "@_", but this should also fit to the sql... Maybe you have an idea?

Should I open a new Issue for this?

[mikependon]

Can you open a new issue with a replicate(able) code? A class with DB schema and a very small console app would do.

[davidrot]

PropertyHandlerMapper.Get(property.DeclaringType), property) is signature is not existing, only some with PropertyHandlerMapper.Get<TPropertyHandler>(property.DeclaringType), property). Please further instructions :)

[mikependon]

Yes, the 2nd signature as you mentioned must be available. Just return an object, then invoke the method named Set via reflection as what we have done earlier in the DbCommandExtensions.

[davidrot]

@mikependon

I have some feedback for you. I can see a future problem by resolving the values from the caches. The keys in the cache can come from the PropertyType / PropertyType + DeclaringType / ...
Under some circumstances it is possible to receive null. I had a similar problem. Therefore I would suggest to add a function what is checking the PropertyCache, PropertyHandlerCache what is checking a various keys.

Does this look correct?

    public static object GetHandledValue(this PropertyInfo propertyInfo, object entity)
    {
        var classProperty = PropertyCache.Get(propertyInfo.DeclaringType, propertyInfo);
        var propertyHandler = classProperty.GetPropertyHandler();

        if (propertyHandler != null)
        {
            var setMethod = propertyHandler.GetType().GetMethod("Set");
            return setMethod.Invoke(propertyHandler, new[] { propertyInfo.GetValue(entity), classProperty });
        }

        return propertyInfo.GetValue(entity);
    }

[mikependon]

Liked it! I will use your methods in the compiler as well. Btw, may I request to add one more additional overload method that accepts the argument 'declaringType' of the GetHandledValue?

PropertyInfo.DeclaringType is failing if the property was implemented in the 'base' class or 'interface'. It returns the 'interface type' or 'base class' instead of the derived/implementing class.

That is is the purpose of the argument 'declaringType' in the overload method, which allows you to pass it over manually.

Then in your code you do it like this PropertyCache.Get(declaringType ??propertyInfo.DeclaringType, propertyInfo);

[mikependon]

@davidrot - you mentioned above the problem in the Update() operation in connection with where?.IsForUpadte() calls. Would you be able to help me replicate this problem by creating a separate issue? Thanks

[davidrot]

@mikependon False alarm :) Cant reproduce anymore

[mikependon]

@davidrot - good that we do not have problem there. :)

Anyway, please feel free to close this if the recent build addressed your issue.

Or, do you need a beta build with your recent PR? Thanks!