NH-3693 - AliasToBeanResultTransformer fails under Firebird #597
Conversation
| { | ||
| if (_propertiesByNameCaseSensitive.TryGetValue(alias, out var property)) | ||
| { | ||
| checkMember(property); |
There was a problem hiding this comment.
On case sensitive case, we should never have an ambiguous case, but better be on the safe side.
| properties.AddRange( | ||
| currentType | ||
| .GetProperties(bindingFlags) | ||
| .Where(p => p.CanWrite) |
There was a problem hiding this comment.
Need to check p.GetIndexParameters().Length ==0 as well
There was a problem hiding this comment.
Yes. I have done some cleanup by the way.
fredericDelaporte
force-pushed
the
NH-3693
branch
from
e53c78f
to
20f17b2
Compare
| .SetResultTransformer(transformer) | ||
| .List<PublicInheritedPropertiesSimpleDTO>(); | ||
| Assert.That(l.Count, Is.EqualTo(2)); | ||
| Assert.That(l, Has.All.Not.Null); |
There was a problem hiding this comment.
Can you also check that all properties get the desired values?
| if (member.AmbiguousMembers == null || member.AmbiguousMembers.Length < 2) | ||
| throw new InvalidOperationException($"{nameof(NamedMember<T>.Member)} missing and {nameof(NamedMember<T>.AmbiguousMembers)} invalid."); | ||
|
|
||
| throw new AmbiguousMatchException( |
There was a problem hiding this comment.
I think we need tests for these exceptional cases
There was a problem hiding this comment.
What about the ToPropertiesInsensitivelyDuplicated_WithoutAnyProjections test I have added with the third commit?
| .GroupBy(m => m.Member.Name, | ||
| (k, g) => | ||
| new NamedMember<T>(k, | ||
| g |
There was a problem hiding this comment.
Need to have a test for this behaviour
There was a problem hiding this comment.
Ok, pushed, tests heavily rewritten.
fredericDelaporte
force-pushed
the
NH-3693
branch
3 times, most recently
from
f6dced9
to
a9b6120
Compare
|
Attempt to fix the new cases for the 3 failing DB. Guessing they yield full lower case aliases instead of uppercase as I was believing. |
|
@fredericDelaporte different tests fail on different DBs |
|
It seems that Oracle and Firebird return upper case, and PostgreSQL - lowercase |
|
Yes I have seen there are troubles, I have already made some attempts but it looks like I will have to debug them with local installs. |
fredericDelaporte
force-pushed
the
NH-3693
branch
2 times, most recently
from
35b49a7
to
a84ca9d
Compare
|
Two unrelated files were added in my previous commit amendment, I have removed them. Unless told otherwise, I intend to merge this PR in the days to come then close #330. |
| /// sorts them by inheritance depth then by visibility from public to private, and takes those ranking first. | ||
| /// </summary> | ||
| [Serializable] | ||
| public class QueryAliasToObjectPropertySetter |
There was a problem hiding this comment.
Or, actually inlined (not just make a nested class, but actually inlined) into the AliasToBeanTransformer
|
|
||
| return new QueryAliasToObjectPropertySetter(fieldsByNameCaseSensitive, fieldsByNameCaseInsensitive, propertiesByNameCaseSensitive, propertiesByNameCaseInsensitive); | ||
|
|
||
| int getFieldVisibilityRank(FieldInfo field) |
There was a problem hiding this comment.
ReSharper suggests that these shall be named as usual method.
There was a problem hiding this comment.
PascalCase? Microsoft seems to mandate camelCase instead, for exhibiting their local nature.
| var currentType = objType; | ||
| var rank = 1; | ||
| // For grasping private members, we need to manually walk the hierarchy. | ||
| while (currentType != null && currentType != typeof(object)) |
There was a problem hiding this comment.
Can you move the while loop to a separate method?
|
To be honest I'm not a big fun of local functions, and would like to avoid them until necessary. |
| /// if no matching property or field was found, retry with a case insensitive match. For members having the same name, it | ||
| /// sorts them by inheritance depth then by visibility from public to private, and takes those ranking first. | ||
| /// </summary> | ||
| [Serializable] |
There was a problem hiding this comment.
If this class is Serializable we need to implement IDeserializationCallback because we have IDictionary<,> fields. Or we need to exclude these fields non serialized and add a deserialization constructor for a class to repopulate these fields
There was a problem hiding this comment.
Maybe better simply store the concrete implementation which is serializable.
I find them handy for avoiding bloating a class with helper functions useful only for a single method. But well, we have cope without them for many years and can go on without them. |
|
Bunch of changes done. |
| @@ -29,37 +31,46 @@ namespace NHibernate.Transform | |||
| /// sorts them by inheritance depth then by visibility from public to private, and takes those ranking first. | |||
| /// </remarks> | |||
| [Serializable] | |||
| public class AliasToBeanResultTransformer : AliasedTupleSubsetResultTransformer | |||
| public class AliasToBeanResultTransformer : AliasedTupleSubsetResultTransformer, IEquatable<AliasToBeanResultTransformer> | |||
There was a problem hiding this comment.
Still need IDeserializationCallback. Or [NonSerialized] + deserialization constructor.
There was a problem hiding this comment.
I will add a serialization/deserialization test. Your link on SO seems unrelated: the trouble was attempting to use the map during the deserialization process. The OP states that the map was correclty filled after, but he needed it during.
|
Serialization/deserialization test added, no troubles. Now if we consider serialization should be spatially optimized (consumed memory) rather than temporally (execution time), flagging dictionaries as non serializable and reconstructing them after deserialization would be better. (I do not tend to bother with those considerations when I do not actually understand the serialization use case for the application: hard to know which optimization to favor in such case.) |
…case, test case and fix for struct components
fredericDelaporte
force-pushed
the
NH-3693
branch
from
c121dad
to
792186f
Compare
|
Rebased and last three commits squashed in one. |
|
Thanks for the reviews. |
NH-3693 - AliasToBeanResultTransformer fails under Firebird, fix and test cases
This is based on #330, rebased, squashed in one commit for original work of @amroel, another one for the "done at the same time" fix and test cases for NH-1904 (trouble with struct having properties with same name but different case), and a last one for my proposed implementation of setter resolver (with test case for the ambiguous match case).