NH-3693 - AliasToBeanResultTransformer fails under Firebird #330

amroel · 2014-09-15T07:57:41Z

https://nhibernate.jira.com/browse/NH-3693
Added additional case-insensitive field/property matching

hazzik · 2014-11-15T03:50:02Z

Also fixes Oracle

hazzik · 2014-11-15T04:00:38Z

But new tests fail http://teamcity.codebetter.com/viewLog.html?buildId=131083&tab=buildResultsDiv&buildTypeId=bt873

amroel · 2014-11-15T07:33:34Z

seems like oracle returns decimal type for the id.

hazzik · 2014-11-15T07:37:49Z

@amroel, yes, it allways does this

hazzik · 2014-11-20T08:08:34Z

src/NHibernate/Properties/BasicPropertyAccessor.cs

+			// This part of the code has beed changed twice to fix the following Issues in order: NH-1728 then NH-1904
+			// As it stands now the implementation prevents AliasToBeanTransformer from finding the correct property 
+			// on a class if the dialect returns column names in a different case than expected.
+
 			BindingFlags bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly;

 			if (type.IsValueType)


Why can not we just remove this check?

Because it was added to fix one or both of the mentioned issues in the
comment I added to the code and I didn't want to modify existing code if
there are alternatives.

hazzik · 2015-04-14T05:50:21Z

@amroel what if we change the way AliasToBeanResultTransformer works to following: get all properties and try to find proper alias?

amroel · 2015-04-27T09:06:00Z

@hazzik I'm very sorry for my late response.
I'm not sure if I understand you correctly!
We are trying to get all properties and match them with their correspondent aliases aren't we?
The "matching" part is what's causing trouble.

hazzik · 2015-04-27T19:48:53Z

No, currently we get all aliases (fields from results) and try to match them to properties.

amroel · 2015-04-29T07:13:32Z

I can't see why this would make a difference.
No matter what we do we still have to match using some sort of a "strategy".
I mean given we first fetch all properties you still can't simply go ahead and do:

from prop in allProperties
where prop.Name == alias
select prop;

right?

hazzik · 2015-04-29T08:08:13Z

Yes we can, I mean we can use ignore case comparer:

from prop in allProperties
where string.Equals(prop.Name, alias, StringComparision.OrdinalIgnoreCase);
select prop;

But we will not have the issues similar to NH-1728 and NH-1904

hazzik · 2015-04-29T08:14:33Z

@amroel do you know if the same problem applies to Hibernate?

amroel · 2015-04-29T08:59:44Z

No, I don't.
But from what I see here:
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/property/BasicPropertyAccessor.java
in setterMethod() they decapitalize and match:

            if ( method.getParameterTypes().length == 1 && methodName.startsWith( "set" ) ) {
                String testStdMethod = Introspector.decapitalize( methodName.substring( 3 ) );
                String testOldMethod = methodName.substring( 3 );
                if ( testStdMethod.equals( propertyName ) || testOldMethod.equals( propertyName ) ) {
                    potentialSetter = method;
                    if ( returnType == null || method.getParameterTypes()[0].equals( returnType ) ) {
                        return potentialSetter;
                    }
                }
            }

Which is somewhat closer to string.Equals(prop.Name, alias, StringComparision.OrdinalIgnoreCase).
So I would say, No, the problem doesn't apply to Hibernate!

hazzik · 2015-04-29T09:05:53Z

@amroel I mean the problem with that Firebird returns aliaces in all caps and Hibernate does comparision with case sensitivity.

So, alias "SOMETHING" will not match "something", or "FOOBAR" will not match "fooBar"

amroel · 2015-04-29T09:32:36Z

@hazzik I don't know.

amroel · 2015-04-29T18:37:41Z

@hazzik how does this looks like?

hazzik · 2015-09-11T02:15:04Z

@amroel it contains a breaking change. Previously it was able to find properties in base classes, not it only looks for one level properties.

amroel · 2015-09-11T11:23:10Z

@hazzik I made 2 additional confirmation tests and removed the 'DeclaredOnly' flag.

fredericDelaporte

Hi Amroel,
I have some concerns about your PR. May you check following comments please?

fredericDelaporte · 2017-04-10T14:55:32Z

src/NHibernate/Properties/BasicPropertyAccessor.cs

-				// 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;
-			}


Why this change?

#330 (diff)

Ok, good for me.

This does fix NH-1904 for the struct case.

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.

fredericDelaporte · 2017-04-10T15:03:31Z

src/NHibernate/Transform/QueryAliasToObjectPropertySetter.cs

+				BindingFlags.NonPublic |
+				BindingFlags.IgnoreCase;
+			var fields = objType.GetFields(bindingFlags);
+			var properties = objType.GetProperties(bindingFlags);


When getting fields or properties without filtering by a name, what is the point of BindingFlags.IgnoreCase?

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

fredericDelaporte · 2017-04-10T15:19:15Z

src/NHibernate/Transform/QueryAliasToObjectPropertySetter.cs

+		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));


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

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

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.

fredericDelaporte · 2017-04-10T15:19:20Z

src/NHibernate/Transform/QueryAliasToObjectPropertySetter.cs

+				return;
+			}
+			if (property != null && property.CanWrite)
+				property.SetValue(resultObj, value, new object[0]);


Silently ignoring the value to set if the property cannot be written? This test should be moved up for throwing PropertyNotFoundException instead I think.

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.

Actually in ToInheritedPrivateFields_WithoutProjections the transformer fails to set "ID"

Because GetFields does not return inherited fields

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

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.

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.

It makes sense, but it will change the current behaviour

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.

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.

hazzik · 2017-04-11T08:46:44Z

I have a reworked version somewhere.

hazzik · 2017-04-11T09:24:25Z

@amroel can you please enable changes by contributors?

amroel · 2017-04-11T10:01:14Z

@hazzik
done. Edits from maintainers are enabled

hazzik · 2017-04-11T10:17:05Z

I'm thinking about going back to your original implementation after all, because supporting fields would be a painful task.

amroel · 2017-04-11T12:10:23Z

@fredericDelaporte
take a look at my original implementation: 42c5b85

fredericDelaporte · 2017-04-11T17:18:37Z

Looking at all that and Msdn for properties and fields, the not included fields from base classes are only private ones, and this holds true for properties too.
So for consistency we should seek on base types for both I think. That is what master implementation does by the way.

For minimizing behavior changes in regard to current master implementation while supporting case insensitive lookup as a fallback, what about following version of QueryAliasToObjectPropertySetter class?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;

namespace NHibernate.Transform
{
	[Serializable]
	public class QueryAliasToObjectPropertySetter
	{
		private readonly IDictionary<string, FieldInfo> _fieldsByNameCaseSensitive;
		private readonly IDictionary<string, FieldInfo> _fieldsByNameCaseInsensitive;
		private readonly IDictionary<string, PropertyInfo> _propertiesByNameCaseSensitive;
		private readonly IDictionary<string, PropertyInfo> _propertiesByNameCaseInsensitive;

		private QueryAliasToObjectPropertySetter(IDictionary<string, FieldInfo> fieldsByNameCaseSensitive,
			IDictionary<string, FieldInfo> fieldsByNameCaseInsensitive,
			IDictionary<string, PropertyInfo> propertiesByNameCaseSensitive,
			IDictionary<string, PropertyInfo> propertiesByNameCaseInsensitive)
		{
			_fieldsByNameCaseSensitive = fieldsByNameCaseSensitive;
			_fieldsByNameCaseInsensitive = fieldsByNameCaseInsensitive;
			_propertiesByNameCaseSensitive = propertiesByNameCaseSensitive;
			_propertiesByNameCaseInsensitive = propertiesByNameCaseInsensitive;
		}

		public static QueryAliasToObjectPropertySetter MakeFor(System.Type objType)
		{
			var bindingFlags = BindingFlags.Instance |
				BindingFlags.Public |
				BindingFlags.NonPublic |
				BindingFlags.DeclaredOnly;
			// Introducing C#7 tuples would avoid those hacks but well, are we willing to introduce the tuple dependency?
			var fields = (new[] { new { field = default(FieldInfo), visibilityRank = 0, hierarchyRank = 0 } }).Where(f => false).ToList();
			var properties = (new[] { new { property = default(PropertyInfo), visibilityRank = 0, hierarchyRank = 0 } }).Where(f => false).ToList();
			var currentType = objType;
			var rank = 1;
			// For grasping private members, we need to manually walk the hierarchy.
			while (currentType != null && currentType != typeof(object))
			{
				fields.AddRange(
					currentType
						.GetFields(bindingFlags)
						.Select(f => new { field = f, visibilityRank = getFieldVisibilityRank(f), hierarchyRank = rank }));
				properties.AddRange(
					currentType
						.GetProperties(bindingFlags)
						.Where(p => p.CanWrite)
						.Select(p => new { property = p, visibilityRank = getPropertyVisibilityRank(p), hierarchyRank = rank }));
				currentType = currentType.BaseType;
				rank++;
			}

			var fieldsByNameCaseSensitive = fields
				.GroupBy(f => f.field.Name)
				.ToDictionary(g => g.Key,
					g => g.OrderBy(f => f.hierarchyRank).Select(f => f.field).First());
			var fieldsByNameCaseInsensitive = fields
				.GroupBy(f => f.field.Name, StringComparer.OrdinalIgnoreCase)
				.ToDictionary(g => g.Key,
					g => g.OrderBy(f => f.hierarchyRank).ThenBy(f => f.visibilityRank).Select(f => f.field).First(),
					StringComparer.OrdinalIgnoreCase);

			var propertiesByNameCaseSensitive = properties
				.GroupBy(p => p.property.Name)
				.ToDictionary(g => g.Key,
					g => g.OrderBy(p => p.hierarchyRank).Select(p => p.property).First());
			var propertiesByNameCaseInsensitive = properties
				.GroupBy(p => p.property.Name, StringComparer.OrdinalIgnoreCase)
				.ToDictionary(g => g.Key,
					g => g.OrderBy(p => p.hierarchyRank).ThenBy(p => p.visibilityRank).Select(p => p.property).First(),
					StringComparer.OrdinalIgnoreCase);

			return new QueryAliasToObjectPropertySetter(fieldsByNameCaseSensitive, fieldsByNameCaseInsensitive, propertiesByNameCaseSensitive, propertiesByNameCaseInsensitive);

			int getFieldVisibilityRank(FieldInfo field)
			{
				if (field.IsPublic)
					return 1;
				if (field.IsFamilyOrAssembly)
					return 2;
				if (field.IsFamily)
					return 3;
				// Inexistent case but well...
				if (field.IsFamilyAndAssembly)
					return 4;
				//if (field.IsPrivate)
				return 5;
			}

			int getPropertyVisibilityRank(PropertyInfo field)
			{
				var setter = field.SetMethod;
				if (setter.IsPublic)
					return 1;
				if (setter.IsFamilyOrAssembly)
					return 2;
				if (setter.IsFamily)
					return 3;
				// Inexistent case but well...
				if (setter.IsFamilyAndAssembly)
					return 4;
				//if (setter.IsPrivate)
				return 5;
			}
		}

		public void SetProperty(string alias, object value, object resultObj)
		{
			if (_propertiesByNameCaseSensitive.TryGetValue(alias, out var property))
			{
				property.SetValue(resultObj, value, new object[0]);
				return;
			}
			if (_fieldsByNameCaseSensitive.TryGetValue(alias, out var field))
			{
				field.SetValue(resultObj, value);
				return;
			}
			if (_propertiesByNameCaseInsensitive.TryGetValue(alias, out property))
			{
				property.SetValue(resultObj, value, new object[0]);
				return;
			}
			if (_fieldsByNameCaseInsensitive.TryGetValue(alias, out field))
			{
				field.SetValue(resultObj, value);
				return;
			}

			throw new PropertyNotFoundException(resultObj.GetType(), alias, "setter");
		}
	}
}

All tests of the branch are good with that.

I may push it to this PR if you wish.

I think doing all that is still better than resolving the property/field at each value to set for each row to transform.

amroel · 2017-04-12T07:19:09Z

@fredericDelaporte, @hazzik
I was looking at my original implementation and thinking: why do we even care about case sensitivity at all in this context?
Why not just replace

propertyAccessor = new ChainedPropertyAccessor(new[]
	                            	{
	                            		PropertyAccessorFactory.GetPropertyAccessor(null),
	                            		PropertyAccessorFactory.GetPropertyAccessor("field")
	                            	});

with

propertyAccessor = new ChainedPropertyAccessor(new[]
	                            	{
	                            		new CaseInsensitivePropertyAccessor(),
	                            		new CaseInsensitiveFieldAccessor()
	                            	});

?
To be more efficient both CaseInsenitivePropertyAccesor and CaseInsenitiveFieldAccesor could cache the resolved properties/fields.
What I like about this is that it would resolve only those fields/properties which are effectively used by a query instead of resolving everything all at once upfront regardless of whether they're used by the involved query or not.

This would also leave place for "fixing" NH-1904 independently but of course this also applies to the version suggested by @fredericDelaporte

What do you guys think?

fredericDelaporte · 2017-04-12T09:32:58Z

Even if this is the current way master works (but case sensitive), I highly dislike it. True, it does not resolve properties or fields which do not correspond to an alias. But is really a bean supposed to have many of them? The usual, most frequent case should be "no".

And it has a big drawback: it does reflect the property or field for each alias for each row to transform! Take a 10 rows result with 5 aliases and a bean without inheritance and 10 fields/properties => 50 reflection operations at best (100 at worst with your last proposal, 200 with your first proposal).

While my proposal will do only 2 reflection call, sorting among 10 entries at worst once upfront (O(n log(n))), then will hydrate the bean with at best 50, at worst 100 O(1) dictionary lookups (if giving up case sensitive matching as your last proposal).

Trading 48 (best, 98 worst) reflection calls for 1 small sort and 50 (100) dictionary lookups should be quite faster. Reflection is damn slow.

It can be objected that my proposal 2 reflection calls are heavier than the ones done in current master. Maybe, but not worst overall than by a factor of the number of fields/properties. So those 2 reflection calls are at worst as costly than 10 reflection calls of current master implementation with above example. This means still a gain of 40 reflection calls.

Adding inheritance in the mix will just scale those numbers linearly for both proposal (worst case at least), since current implementation do the same as my proposal (indeed, I have copied the principle) about inherited types.

And since all the upfront cost occurs at bean transformer instanciation, it can even be saved and reused for later queries call. (Just store the output of Transformers.AliasToBean<BeanClass>(); we could even introduce here an improvement for caching them.)

About giving up case sensitive match completely, I am not for it. That will be a breaking change in case a bean was having two properties differing only by case, while using a DB preserving the aliases case.

…case, test case for struct components

fredericDelaporte · 2017-04-12T10:47:27Z

Since this PR currently fixes NH-1904 for the struct case by the way, but without a test case, I have added one.

amroel · 2017-04-12T10:48:13Z

I am fully aware of the benefits of your version @fredericDelaporte
What I was trying to say or ask is the following:
Are we trying to map a class to a query or do we have a query and would like to find matching class-properties?
The latter case does clearly imply that you may have a class with a bunch of properties that have nothing in common with the query!
But of course I have no Idea how often that would or not be the case.

I'm perfectly fine with your version of QueryAliasToObjectPropertySetter.
@hazzik ?

fredericDelaporte · 2017-04-12T10:59:49Z

With a library, there is always the trouble of intended use vs actual use. The later is most likely to be wider. Either intended use is clearly documented and then we are quite legit about breaking some unintended actual use cases, or we have to take care avoiding breaking any potential actual uses we have not forecast.

I believe there is nowhere the documentation states the bean has to closely match the query and that additional fields/properties are unsupported.

Going fully case insensitive is going to break some potential use cases. Since this transformer is here for quite a long time, it is likely such use cases exist. This is why I am against this change, as stated in my last paragraph of my previous long comment.

amroel · 2017-04-12T11:34:13Z

I believe there is nowhere the documentation states the bean has to closely match the query and that additional fields/properties are unsupported.

That is precisely the reason for my question class->query or query->class?
Resolving all class-properties upfront assumes a close matching to the query.
This is of course perfectly fine if this is the majority of the use cases.
My own personal coding style would say: yes, this is and should be the usual use case.

I just don't know if some weirdos would create a huge class as a sort of a "global data bag" and use it for more than 10 different queries!

What about constructing your version of QueryAliasToObjectPropertySetter the first time TransformTuple is called and pass it the aliases?

fredericDelaporte · 2017-04-12T11:47:22Z

There is another difference between 42c5b85 and my proposal, in favor of 42c5b85 this time I think.

This difference is about the behavior when we have to fallback to case insensitive matching, while two properties declared in the same type insensitively match: 42c5b85 will throw an AmbiguousMatchException, while my proposal will set one of them "at random" (if they have the same accessibility). My proposal can easily be adjusted to sort them by name in a sensitive way and so avoid the "random" aspect. But we may still prefer to raise an exception instead in that case, which will then require more changes.

fredericDelaporte · 2017-04-12T11:55:45Z

What about constructing your version of QueryAliasToObjectPropertySetter the first time TransformTuple is called and pass it the aliases?

Would break even more cases. People are defining beans sometime a bit wider than queries just for reusing them among similar queries. Reusing the result transformer among those queries would then break.

Why going narrower with our supported use cases? It does not look to me it really simplifies much the code base.

fredericDelaporte · 2017-04-12T12:02:43Z

src/NHibernate/Transform/AliasToBeanResultTransformer.cs

 			}

-			propertyAccessor =
-				new ChainedPropertyAccessor(new[]


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

amroel · 2017-04-12T12:32:24Z

Would break even more cases. People are defining beans sometime a bit wider than queries just for reusing them among similar queries. Reusing the result transformer among those queries would then break.

I'm not sure I understood. Can you please elaborate a bit more on this?
As I see it, the only place where the "accessor" is used is in TransformTuple. The behavior to anyone using this class wouldn't be changed at all.

Why going narrower with our supported use cases? It does not look to me it really simplifies much the code base.

I don't want to narrow the supported use cases at all! I only want to make sure that AliasToBeanResultTransformer do what its name promises to do. From an Alias to a Bean(property).

fredericDelaporte · 2017-04-12T13:11:29Z

What about constructing your version of QueryAliasToObjectPropertySetter the first time TransformTuple is called and pass it the aliases?

I understand this as "lazily construct the QueryAliasToObjectPropertySetter at first TransformTuple call specifically for the provided aliases list, then reuse it on subsequent calls". In such case, reusing the transformer for another query having an additional alias will cause this additional alias to fail. Yes, a transformer can be reused among different queries.

amroel · 2017-04-12T13:33:18Z

Ok, I understand now.

fredericDelaporte · 2017-04-12T16:31:53Z

So I have done #597 based on your work, for testing my proposal, adjusted to handle the ambiguous match case with an explicit failure.

amroel · 2017-04-13T08:43:17Z

Thanks for your time and effort @fredericDelaporte

hazzik · 2017-04-23T05:49:48Z

Replaced by #597

hazzik force-pushed the master branch from eb44257 to 4b88cc1 Compare September 21, 2014 13:52

hazzik added the milestone ??? label Sep 21, 2014

hazzik added this to the 4.1.0 milestone Nov 15, 2014

hazzik removed the milestone ??? label Nov 15, 2014

hazzik reviewed Nov 20, 2014
View reviewed changes

hazzik added the to review label Dec 2, 2014

amroel force-pushed the NH-3693 branch 4 times, most recently from 09adcfd to 0dcdde8 Compare February 25, 2015 10:45

amroel force-pushed the NH-3693 branch from 0dcdde8 to f47ceb1 Compare April 29, 2015 18:36

amroel force-pushed the NH-3693 branch from f47ceb1 to fab992c Compare September 11, 2015 11:07

hazzik force-pushed the master branch from e20411b to 9b7a5a3 Compare October 26, 2016 01:26

oskarb added this to the 5.0.0 milestone Nov 20, 2016

fredericDelaporte requested changes Apr 10, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into amroel-NH-3693-2

1d949eb

Enforce Id property check

8dfccf4

hazzik added needs work and removed to review labels Apr 11, 2017

NH-1904 - public properties cannot have the same name with different …

5d8f2dc

…case, test case for struct components

fredericDelaporte reviewed Apr 12, 2017

View reviewed changes

fredericDelaporte mentioned this pull request Apr 12, 2017

NH-3693 - AliasToBeanResultTransformer fails under Firebird #597

Merged

hazzik closed this Apr 23, 2017

hazzik removed this from the 5.0.0 milestone Apr 23, 2017

NH-3693 - AliasToBeanResultTransformer fails under Firebird #330

NH-3693 - AliasToBeanResultTransformer fails under Firebird #330

Uh oh!

Conversation

amroel commented Sep 15, 2014

Uh oh!

hazzik commented Nov 15, 2014

Uh oh!

hazzik commented Nov 15, 2014

Uh oh!

amroel commented Nov 15, 2014

Uh oh!

hazzik commented Nov 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Apr 14, 2015

Uh oh!

amroel commented Apr 27, 2015

Uh oh!

hazzik commented Apr 27, 2015 via email

Uh oh!

amroel commented Apr 29, 2015

Uh oh!

hazzik commented Apr 29, 2015

Uh oh!

hazzik commented Apr 29, 2015

Uh oh!

amroel commented Apr 29, 2015

Uh oh!

hazzik commented Apr 29, 2015

Uh oh!

amroel commented Apr 29, 2015

Uh oh!

amroel commented Apr 29, 2015

Uh oh!

hazzik commented Sep 11, 2015

Uh oh!

amroel commented Sep 11, 2015

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fredericDelaporte Apr 11, 2017 •

edited

Loading

fredericDelaporte Apr 11, 2017 •

edited

Loading

fredericDelaporte Apr 11, 2017 •

edited

Loading

fredericDelaporte commented Apr 11, 2017 •

edited

Loading

fredericDelaporte commented Apr 12, 2017 •

edited

Loading

amroel commented Apr 12, 2017 •

edited

Loading

fredericDelaporte commented Apr 12, 2017 •

edited

Loading