NH-3634 - Fix comparing property to component with nullable properties in Criteria/QueryOver #390
Conversation
|
Ready for code review |
|
Could you please add tests for second level cache? |
|
@hazzik Sure thing. |
|
@hazzik Second-level cache test added. Are there any other cases I've missed? |
|
@hazzik I just pushed 2 more tests which I believe are more inline with what you were asking for. Sorry for the delay! |
|
@jlevitt can you please squash your code and rebase on top of master? |
|
@hazzik Sure thing. I may not be able to get to it for a while though, as I will be travelling this week. |
|
@jlevitt any chance to clean up the pull request? |
|
@hazzik Yes. Sorry I forgot about this. I will set a reminder to work on it this weekend. |
|
@jlevitt thanks! |
|
Weekends has passed, any news? @jlevitt |
|
@hazzik I'm sorry I've been too busy lately to get to this. I've been moving houses. I still have a reminder for myself to do it, but I need to find time. |
|
@hazzik This branch has been squashed and rebased. Sorry for the delay. |
| .Add(Op) | ||
| .Add(parameters[i]); | ||
|
|
||
| if (columnNullness[i]) |
There was a problem hiding this comment.
I did not check the code yet, but shouldn't it be opposite?
|
@jlevitt thanks |
NH-3634 - Fix comparing property with component with nullable properties in Criteria/QueryOver
|
Thanks a lot, @jlevitt |
hazzik
changed the title
📙 Ready for code review
Description
Test case and fix for NH-3634.
Please do a thorough code review. I'm new to the NHB code base, and I'm not entirely sure that this was the best place to make the fix.
Todo
Add test where component only has one property and case is ignored. If fails, fix (in SimpleExpression).This case is broken, but not related to NH-3634.MetaType.ToColumnNullnessAnyIs_QueryOverEachEmbeddedBasicTypeIsSerializableCode Review Questions
if (ignoreCase)code path inSimpleExpression.ToSqlStringseems broken. What if this is applied to a component with a single property. Because ofobject icvalue = ignoreCase ? value.ToString().ToLower() : value;inGetParameterTypedValuethe compared value will be changed from the component object to a string. This is broken. Maybe it doesn't matter, because it's a edge case, but it might be indicitive of a larger problem around handling case insensitivity forSimpleExpression. I won't attempt to fix as part of this pull request unless asked to.MetaType.ToColumnNullnesshere? I couldn't see any reason not to, and it didn't fail any tests.