Skip to content

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

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/NHibernate.Test/Criteria/Lambda/LambdaFixtureBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,9 @@ private void AssertObjectsAreEqual(object expected, object actual, string name)

if ((expectedType.IsValueType)
|| (expected is System.Type)
|| (expected is string))
|| (expected is string)
|| (expected is FieldInfo)
|| (expected is PropertyInfo))
{
Assert.AreEqual(expected, actual, fieldPath);
_fieldPath.Pop();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
using System;
using System.Collections;
using System.Collections.Generic;
using NHibernate.Transform;
Expand Down Expand Up @@ -41,6 +40,43 @@ public struct TestStruct
public string Something { get; set; }
}

public class PublicPropertiesSimpleDTO
{
public object Id { get; set; }
public string Name { get; set; }
}

public class PrivateFieldsSimpleDTO
{
private object id;
private string name;

public object Id { get { return id; } }
public string Name { get { return name; } }
}

public class BasePublicPropsSimpleDTO
{
public object Id { get; set; }
}

public class PublicInheritedPropertiesSimpleDTO : BasePublicPropsSimpleDTO
{
public string Name { get; set; }
}

public class BasePrivateFieldSimpleDTO
{
private object id;
public object Id { get { return id; } }
}

public class PrivateInheritedFieldsSimpleDTO : BasePrivateFieldSimpleDTO
{
private string name;
public string Name { get { return name; } }
}

#region Overrides of TestCase

protected override IList Mappings
Expand Down Expand Up @@ -77,6 +113,98 @@ public void WorkWithOutPublicParameterLessCtor()
}
}

[Test]
public void ToPublicProperties_WithoutAnyProjections()
{
try
{
Setup();

using (ISession s = OpenSession())
{
var transformer = Transformers.AliasToBean<PublicPropertiesSimpleDTO>();
IList<PublicPropertiesSimpleDTO> l = s.CreateSQLQuery("select * from Simple")
.SetResultTransformer(transformer)
.List<PublicPropertiesSimpleDTO>();
Assert.That(l.Count, Is.EqualTo(2));
Assert.That(l, Has.All.Not.Null);
}
}
finally
{
Cleanup();
}
}

[Test]
public void ToPrivateFields_WithoutAnyProjections()
{
try
{
Setup();

using (ISession s = OpenSession())
{
var transformer = Transformers.AliasToBean<PrivateFieldsSimpleDTO>();
IList<PrivateFieldsSimpleDTO> l = s.CreateSQLQuery("select * from Simple")
.SetResultTransformer(transformer)
.List<PrivateFieldsSimpleDTO>();
Assert.That(l.Count, Is.EqualTo(2));
Assert.That(l, Has.All.Not.Null);
}
}
finally
{
Cleanup();
}
}

[Test]
public void ToInheritedPublicProperties_WithoutProjections()
{
try
{
Setup();

using (ISession s = OpenSession())
{
var transformer = Transformers.AliasToBean<PublicInheritedPropertiesSimpleDTO>();
IList<PublicInheritedPropertiesSimpleDTO> l = s.CreateSQLQuery("select * from Simple")
.SetResultTransformer(transformer)
.List<PublicInheritedPropertiesSimpleDTO>();
Assert.That(l.Count, Is.EqualTo(2));
Assert.That(l, Has.All.Not.Null);
}
}
finally
{
Cleanup();
}
}

[Test]
public void ToInheritedPrivateFields_WithoutProjections()
{
try
{
Setup();

using (ISession s = OpenSession())
{
var transformer = Transformers.AliasToBean<PrivateInheritedFieldsSimpleDTO>();
IList<PrivateInheritedFieldsSimpleDTO> l = s.CreateSQLQuery("select * from Simple")
.SetResultTransformer(transformer)
.List<PrivateInheritedFieldsSimpleDTO>();
Assert.That(l.Count, Is.EqualTo(2));
Assert.That(l, Has.All.Not.Null);
}
}
finally
{
Cleanup();
}
}

[Test]
public void WorkWithPublicParameterLessCtor()
{
Expand Down
1 change: 1 addition & 0 deletions src/NHibernate/NHibernate.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,7 @@
<Compile Include="Transform\ITupleSubsetResultTransformer.cs" />
<Compile Include="Transform\DistinctRootEntityResultTransformer.cs" />
<Compile Include="Transform\IResultTransformer.cs" />
<Compile Include="Transform\QueryAliasToObjectPropertySetter.cs" />
<Compile Include="Transform\RootEntityResultTransformer.cs" />
<Compile Include="TransientObjectException.cs" />
<Compile Include="Type\AbstractType.cs" />
Expand Down
8 changes: 0 additions & 8 deletions src/NHibernate/Properties/BasicPropertyAccessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,6 @@ internal static BasicSetter GetSetterOrNull(System.Type type, string propertyNam

BindingFlags bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly;

if (type.IsValueType)
{
// the BindingFlags.IgnoreCase is important here because if type is a struct, the GetProperty method does
// not ignore case by default. If type is a class, it _does_ ignore case... we're better off explicitly
// stating that casing should be ignored so we get the same behavior for both structs and classes
bindingFlags = bindingFlags | BindingFlags.IgnoreCase;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good for me.

Copy link
Member

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.

Copy link
Contributor Author

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.


PropertyInfo property = type.GetProperty(propertyName, bindingFlags);

if (property != null && property.CanWrite)
Expand Down
38 changes: 7 additions & 31 deletions src/NHibernate/Transform/AliasToBeanResultTransformer.cs
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
{
Expand All @@ -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)
Expand All @@ -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[]
Copy link
Member

@fredericDelaporte fredericDelaporte Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only usage of the class ChainedPropertyAccessor. With this change, this class as no more any purpose in NHibernate. This class should be marked with [Obsolete("Has no more usage and will be removed in a future version")].

(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)
Expand All @@ -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)
Expand Down
48 changes: 48 additions & 0 deletions src/NHibernate/Transform/QueryAliasToObjectPropertySetter.cs
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);
Copy link
Member

Choose a reason for hiding this comment

The 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 BindingFlags.IgnoreCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch @fredericDelaporte
it doesn't make sense and i'll remove it.


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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks a bit inefficient. Better construct two new Dictionary<string, Field/PropertyInfo>(StringComparer.OrdinalIgnoreCase) in constructor and use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredericDelaporte we cannot make it just a dictionary, see NH-1904

Copy link
Member

@fredericDelaporte fredericDelaporte Apr 11, 2017

Choose a reason for hiding this comment

The 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]);
Copy link
Member

Choose a reason for hiding this comment

The 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 PropertyNotFoundException instead I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the test cannot be simply moved up!
The thing is that if you have a readonly Property named "Anything" with a backing field named "anything" then we would throw an exception because the property cannot be written although a backing field is available.
However I can throw the same exception right there if the property is readonly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in ToInheritedPrivateFields_WithoutProjections the transformer fails to set "ID"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! How come the tests on the build server were passing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that if you have a readonly Property named "Anything" with a backing field named "anything" then we would throw an exception because the property cannot be written although a backing field is available.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, but it will change the current behaviour

Copy link
Member

@fredericDelaporte fredericDelaporte Apr 11, 2017

Choose a reason for hiding this comment

The 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.

Copy link
Member

@fredericDelaporte fredericDelaporte Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, but it will change the current behaviour

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.

}
}
}