-
Notifications
You must be signed in to change notification settings - Fork 933
NH-3693 - AliasToBeanResultTransformer fails under Firebird #330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
42c5b85
af58562
3409f8f
51e454d
5a7529f
a55a9fe
9bd8d1a
0d628f3
7e61b14
b20526a
c62df4a
1d949eb
8dfccf4
5d8f2dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
using System; | ||
using System.Collections; | ||
using System.Reflection; | ||
using NHibernate.Properties; | ||
|
||
namespace NHibernate.Transform | ||
{ | ||
|
@@ -27,10 +26,9 @@ namespace NHibernate.Transform | |
[Serializable] | ||
public class AliasToBeanResultTransformer : AliasedTupleSubsetResultTransformer | ||
{ | ||
private readonly QueryAliasToObjectPropertySetter _propertySetter; | ||
private const BindingFlags flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance; | ||
private readonly System.Type resultClass; | ||
private ISetter[] setters; | ||
private readonly IPropertyAccessor propertyAccessor; | ||
private readonly ConstructorInfo constructor; | ||
|
||
public AliasToBeanResultTransformer(System.Type resultClass) | ||
|
@@ -48,22 +46,17 @@ public AliasToBeanResultTransformer(System.Type resultClass) | |
if (constructor == null && resultClass.IsClass) | ||
{ | ||
throw new ArgumentException("The target class of a AliasToBeanResultTransformer need a parameter-less constructor", | ||
"resultClass"); | ||
"resultClass"); | ||
} | ||
|
||
propertyAccessor = | ||
new ChainedPropertyAccessor(new[] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the only usage of the class (Of course, if we keep current changes, not reverting back to 42c5b85.) |
||
{ | ||
PropertyAccessorFactory.GetPropertyAccessor(null), | ||
PropertyAccessorFactory.GetPropertyAccessor("field") | ||
}); | ||
_propertySetter = QueryAliasToObjectPropertySetter.MakeFor(resultClass); | ||
} | ||
|
||
|
||
public override bool IsTransformedValueATupleElement(String[] aliases, int tupleLength) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
|
||
public override object TransformTuple(object[] tuple, String[] aliases) | ||
|
@@ -76,30 +69,13 @@ public override object TransformTuple(object[] tuple, String[] aliases) | |
|
||
try | ||
{ | ||
if (setters == null) | ||
{ | ||
setters = new ISetter[aliases.Length]; | ||
for (int i = 0; i < aliases.Length; i++) | ||
{ | ||
string alias = aliases[i]; | ||
if (alias != null) | ||
{ | ||
setters[i] = propertyAccessor.GetSetter(resultClass, alias); | ||
} | ||
} | ||
} | ||
|
||
// if resultClass is not a class but a value type, we need to use Activator.CreateInstance | ||
result = resultClass.IsClass | ||
? constructor.Invoke(null) | ||
: Cfg.Environment.BytecodeProvider.ObjectsFactory.CreateInstance(resultClass, true); | ||
? constructor.Invoke(null) | ||
: Cfg.Environment.BytecodeProvider.ObjectsFactory.CreateInstance(resultClass, true); | ||
|
||
for (int i = 0; i < aliases.Length; i++) | ||
{ | ||
if (setters[i] != null) | ||
{ | ||
setters[i].Set(result, tuple[i]); | ||
} | ||
_propertySetter.SetProperty(aliases[i], tuple[i], result); | ||
} | ||
} | ||
catch (InstantiationException e) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Reflection; | ||
|
||
namespace NHibernate.Transform | ||
{ | ||
[Serializable] | ||
public class QueryAliasToObjectPropertySetter | ||
{ | ||
private readonly IEnumerable<FieldInfo> _fields; | ||
private readonly IEnumerable<PropertyInfo> _properties; | ||
|
||
private QueryAliasToObjectPropertySetter(FieldInfo[] fields, PropertyInfo[] properties) | ||
{ | ||
_fields = fields; | ||
_properties = properties; | ||
} | ||
|
||
public static QueryAliasToObjectPropertySetter MakeFor(System.Type objType) | ||
{ | ||
var bindingFlags = BindingFlags.Instance | | ||
BindingFlags.Public | | ||
BindingFlags.NonPublic | | ||
BindingFlags.IgnoreCase; | ||
var fields = objType.GetFields(bindingFlags); | ||
var properties = objType.GetProperties(bindingFlags); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When getting fields or properties without filtering by a name, what is the point of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch @fredericDelaporte |
||
|
||
return new QueryAliasToObjectPropertySetter(fields, properties); | ||
} | ||
|
||
public void SetProperty(string alias, object value, object resultObj) | ||
{ | ||
var property = _properties.SingleOrDefault(prop => string.Equals(prop.Name, alias, StringComparison.OrdinalIgnoreCase)); | ||
var field = _fields.SingleOrDefault(prop => string.Equals(prop.Name, alias, StringComparison.OrdinalIgnoreCase)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks a bit inefficient. Better construct two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fredericDelaporte we cannot make it just a dictionary, see NH-1904 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, thought this is bad practice to have member names differing only by the case, we have to support it. The current field/property resolution process does not really support it either, it will take the first that comes, without having any clue whether that is the more sensible one to take or not. So we can fetch the dictionary with a similar logic, grouping fields/properties by name case insensitively and just putting the first in (while filtering out read only properties by the way, in order to avoid doing that at each field set). A more sensible algorithm would be to: 1. filter out readonly properties - 2. sort by accessibility then by declaring type inheritance depth (at least, is it inherited or not). 3. take the first (may still have many with same ordering rank). We could go further by maintaining a case sensitive dictionary too, resolving by it first then fallbacking on the case insensitive one. But that would be added complexity and added behavior discrepancies between DB altering aliases case and others. |
||
if (field == null && property == null) | ||
throw new PropertyNotFoundException(resultObj.GetType(), alias, "setter"); | ||
|
||
if (field != null) | ||
{ | ||
field.SetValue(resultObj, value); | ||
return; | ||
} | ||
if (property != null && property.CanWrite) | ||
property.SetValue(resultObj, value, new object[0]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silently ignoring the value to set if the property cannot be written? This test should be moved up for throwing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the test cannot be simply moved up! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting! How come the tests on the build server were passing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was thinking about writting: if (field == null && (property == null || !property.CanWrite))
throw new PropertyNotFoundException(resultObj.GetType(), alias, "setter"); That should take care of the "field available" case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way this implementation favor setting fields over setting properties. I would tend to do the opposite. If there is a property and a field of the same name, they are more chances that the property is publicly exposed while the field is not, than the other way round. If the property can be set, it would then be safer, since nothing guarantees us that the field is indeed a backing field for the property. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense, but it will change the current behaviour There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what I propose for "more sensible" filed or property resolution in the comment about dictionary is also quite a change. But relying on .Net Framework fields and properties reflection order is not very satisfactory, since this order is not at all guaranteed and may change form a framework version to another. So maybe for sticking to current behavior as much as possible should we still favor fields other properties, but add some more sensible algorithm for selecting a field among candidates fields with the same name, and same for properties. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If I read correctly the code, we have in master: propertyAccessor =
new ChainedPropertyAccessor(new[]
{
PropertyAccessorFactory.GetPropertyAccessor(null),
PropertyAccessorFactory.GetPropertyAccessor("field")
}); Which does indeed already favor properties over fields, doesn't it? So it looks to me this is current PR which already change behavior by favoring fields other properties. |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#330 (diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does fix NH-1904 for the struct case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but if we're going towards QueryAliasToObjectPropertySetter then I would suggest a separate PR for this then. I only did it here because it "was" used in AliasToBeanResultTransformer.