Issue 93: Secondary constructor in custom type
Marco Rico-Gomez
I tried myself on issue #93. (https://github.com/max-l/Squeryl/issues#issue/93)
It think it could be solved by changing the function
_createCustomTypeFactory in FieldMetaData as already mentioned by
Enrique (http://groups.google.com/group/squeryl/msg/86aac6ca9afa7077)
The current function takes the first one-arg constructor in the field
class for which it can create a default value
and returns a closure for it. This algorithm fails if the field's
class has multiple one-arg constructors.
I tried to rewrite the function so it is able to handle classes with
multiple one-arg constructors.
In my solution it creates a partial function for each one-arg
constructor for which a default value can be created.
Each partial function is only defined if the field's value is null or
the associated constructor can
be called with the given value.
The new _createCustomTypeFactory returns a function which selects and
calls the best constructor for the given type.
If no constructor matches a RuntimeException will be thrown.
The changes can be found here:
https://github.com/mrico/Squeryl/commit/a4b6ecad8d9780a79dd258d4311530ad04662342
To demonstrate and test the changes I added an additional constructor
to the custom type Age in the TestCustomTypesMode test case.
To run the tests checkout the branch mr_issue_93 in
https://github.com/mrico/Squery and exec sbt test
What do you think? Could this solution have some negative impacts?
--
\ Marco
--
Twitter: @mricog
Website: http://mrico.eu
Maxime Lévesque
It looks good, the logic is sound,
I have a question though, how would this one behave :
class Age(v: Int) extends IntField(v) with Domain[Int] {
def this(s: String) = this(Int.parse(s))
def this(d: Date) = this(....get an int out of a date somehow...)
}
if the algorithm chooses the string or date constructor over the Int ?
This got me thinking that type of the 'value' field for custom types is *not* erased,
(I forgot which exact reflection trick allows you to get it, but it is available),
it could be the preffered way to choose the "right" constructor.
//ancestor class has a 'value' field who's type *should* be reflectable :
class Age(v: Int) extends IntField(v) with Domain[Int] {
def validate(a: Int) = assert(a > 0, "age must be positive, got " + a)
def label = "age"
}
what do you think ?
Marco Rico-Gomez
>
> It looks good, the logic is sound,
>
> I have a question though, how would this one behave :
>
> class Age(v: Int) extends IntField(v) with Domain[Int] {
> def this(s: String) = this(Int.parse(s))
> def this(d: Date) = this(....get an int out of a date somehow...)
> }
>
> if the algorithm chooses the string or date constructor over the Int ?
In the tests the algorithm uses the Int constructor. The only case I
could imagine where one of the other constructors could be used is in
case of 'null' values.
>
> This got me thinking that type of the 'value' field for custom types is
> *not* erased,
> (I forgot which exact reflection trick allows you to get it, but it is
> available),
> it could be the preffered way to choose the "right" constructor.
Agreed, this sounds like a cleaner solution. I tried to implement it.
To get access to the value field I called
typeOfField.getMethod("value"). Now the problem is that the return
type of the method 'value' is for some reason java.lang.Object.
I used javap to disassamble the Age class and got the following result
for the method value, what confuses me a bit:
--
public java.lang.Object value();
--
When I disassamble DoubleField I get the following result:
--
public double value();
--
Any ideas?
Marco Rico-Gomez
Now the algorithm runs through the field's class hierarchy and looks for the first method which is called "value" and doesn't return java.lang.Object.
If such a method was found it looks for a constructor which has a parameter with the same type than the return type of the 'value' method. Finally it returns the closure.
To ensure that every CustomType implementation has a 'value' field I added an abstract field to the CustomType trait. Do you think it is worth?
During my tests I run into a bug in 'FieldMetaData.set'. The behavior of Option[CustomType] with null values is strange because it returns something like Some(defaultValue) instead of None. This bug exists in the master branch too. To reproduce the bug just add the following line to 'TestCustomTypesMode.testAll':
--
validateQuery('simpleSelect2, patients.where(_.age < 40), (p:Patient)=>p.id.value, List(raoulEspinoza.id.value))
--
The problem is that the set method calls the customTypeFactory with the null value. The factory then creates an instance of the CustomType with a defaultValue. So the mapped value is Some(defaultValue) instead of None. I aligned the 'FieldMetaData.set' function accordingly so a customTypeFactory will not be called with null values.
(Please note that if you run the test you'll get an Exception instead of the explained behavior. This is because the validation rule 'age must be greater than zero' raises an exception)
Code can be found here: https://github.com/mrico/Squeryl/commit/1c44f7ae99ee8723882c0d6afcdd3ddf97162063
What do you think?
Maxime Lévesque
To ensure that every CustomType implementation has a 'value' field I added an abstract field to the CustomType trait. Do you think it is worth?
Yes I think it is worth adding, I'm thinking, it might be a good ideal also to test the type with FieldMetaData.FieldMetaData.isSupportedFieldType(theClassObject)
During my tests I run into a bug in 'FieldMetaData.set'. The behavior of Option[CustomType] with null values is strange because it returns something like Some(defaultValue) instead of None. This bug exists in the master branch too. To reproduce the bug just add the following line to 'TestCustomTypesMode.testAll':
--
validateQuery('simpleSelect2, patients.where(_.age < 40), (p:Patient)=>p.id.value, List(raoulEspinoza.id.value))
I added some logging, in the soon to be obsoleted code, here's the output :
Constructor public org.squeryl.tests.customtypes.FirstName(java.lang.String) succeeded whith param
const arg is of type class java.lang.String, was given : class java.lang.String
constructed a class org.squeryl.tests.customtypes.FirstName
Constructor public org.squeryl.customtypes.IntField(int) succeeded whith param 0
const arg is of type int, was given : class java.lang.Integer
constructed a class org.squeryl.customtypes.IntField
Constructor public org.squeryl.customtypes.IntField(int) succeeded whith param 0
const arg is of type int, was given : class java.lang.Integer
constructed a class org.squeryl.customtypes.IntField
Constructor public org.squeryl.tests.customtypes.Info(java.lang.String) succeeded whith param
const arg is of type class java.lang.String, was given : class java.lang.String
constructed a class org.squeryl.tests.customtypes.Info
Constructor public org.squeryl.customtypes.IntField(int) succeeded whith param 0
const arg is of type int, was given : class java.lang.Integer
constructed a class org.squeryl.customtypes.IntField
Caused by: java.lang.RuntimeException: Constructor public org.squeryl.tests.customtypes.WeightInKilograms(double) failed whith param 0.0
const arg is of type double, was given : class java.lang.Double
java.lang.reflect.InvocationTargetException
at scala.Predef$.error(Predef.scala:58)
at org.squeryl.internals.FieldMetaData$$anonfun$org$squeryl$internals$FieldMetaData$$_createCustomTypeFactory$2$$anonfun$apply$1.apply(FieldMetaData.scala:492)
One difference that I observe between the case that fails with those that don't
is that for primitive types, the constructor from the ancestor (direct descendants from CustomType) class work,
while WeightInKilograms doesn't, and happens to be an indirect descendant of
the java specs seem to say that with reflection, a method that takes a int can be fed a java.lang.Int
(how could it be otherwise, since there is no way to get a reference of java.lang.Object on a 'int' ...
String constructors don't have this problem since they don't have this primitive vs boxed dual
representation.
--
The problem is that the set method calls the customTypeFactory with the null value. The factory then creates an instance of the CustomType with a defaultValue. So the mapped value is Some(defaultValue) instead of None. I aligned the 'FieldMetaData.set' function accordingly so a customTypeFactory will not be called with null values.
Oh, if I understand avoiding creating Some(defaultValue) would prevent the problem I just described ?
Marco Rico-Gomez
On 27.01.2011, at 01:28, Maxime Lévesque wrote:
>
>
> To ensure that every CustomType implementation has a 'value' field I added an abstract field to the CustomType trait. Do you think it is worth?
>
>
> Yes I think it is worth adding, I'm thinking, it might be a good ideal also to test the type with FieldMetaData.FieldMetaData.isSupportedFieldType(theClassObject)
Ok.
>
>
> During my tests I run into a bug in 'FieldMetaData.set'. The behavior of Option[CustomType] with null values is strange because it returns something like Some(defaultValue) instead of None. This bug exists in the master branch too. To reproduce the bug just add the following line to 'TestCustomTypesMode.testAll':
> --
> validateQuery('simpleSelect2, patients.where(_.age < 40), (p:Patient)=>p.id.value, List(raoulEspinoza.id.value))
>
> I added some logging, in the soon to be obsoleted code, here's the output :
>
>
> Constructor public org.squeryl.tests.customtypes.FirstName(java.lang.String) succeeded whith param
> const arg is of type class java.lang.String, was given : class java.lang.String
> constructed a class org.squeryl.tests.customtypes.FirstName
>
> Constructor public org.squeryl.customtypes.IntField(int) succeeded whith param 0
> const arg is of type int, was given : class java.lang.Integer
> constructed a class org.squeryl.customtypes.IntField
>
> Constructor public org.squeryl.customtypes.IntField(int) succeeded whith param 0
> const arg is of type int, was given : class java.lang.Integer
> constructed a class org.squeryl.customtypes.IntField
>
> Constructor public org.squeryl.tests.customtypes.Info(java.lang.String) succeeded whith param
> const arg is of type class java.lang.String, was given : class java.lang.String
> constructed a class org.squeryl.tests.customtypes.Info
>
> Constructor public org.squeryl.customtypes.IntField(int) succeeded whith param 0
> const arg is of type int, was given : class java.lang.Integer
> constructed a class org.squeryl.customtypes.IntField
>
>
> Caused by: java.lang.RuntimeException: Constructor public org.squeryl.tests.customtypes.WeightInKilograms(double) failed whith param 0.0
> const arg is of type double, was given : class java.lang.Double
> java.lang.reflect.InvocationTargetException
> at scala.Predef$.error(Predef.scala:58)
> at org.squeryl.internals.FieldMetaData$$anonfun$org$squeryl$internals$FieldMetaData$$_createCustomTypeFactory$2$$anonfun$apply$1.apply(FieldMetaData.scala:492)
>
This is the 'null' case. In the 'master' branch the customTypeFactory is called with a null value and creates an instance of the CustomType with a defaultValue (here: 0.0). Now a InvocationTargetException is raised which is caused by the assertion 'weight must be positive, got 0.0'. Unfortunately the target exception is not visible in the stack-trace. This is why I catch the InvocationTargetException in the 'invoke' helper method and re-throw the target exception.
AFAIK the customTypeFactory shouldn't be called at all with null values from 'FieldMetaData.set', because the result should be None not Some(new CustomType(defaultValue)).
>
> One difference that I observe between the case that fails with those that don't
> is that for primitive types, the constructor from the ancestor (direct descendants from CustomType) class work,
> while WeightInKilograms doesn't, and happens to be an indirect descendant of
>
> the java specs seem to say that with reflection, a method that takes a int can be fed a java.lang.Int
> (how could it be otherwise, since there is no way to get a reference of java.lang.Object on a 'int' ...
>
> String constructors don't have this problem since they don't have this primitive vs boxed dual
> representation.
>
>
> --
> The problem is that the set method calls the customTypeFactory with the null value. The factory then creates an instance of the CustomType with a defaultValue. So the mapped value is Some(defaultValue) instead of None. I aligned the 'FieldMetaData.set' function accordingly so a customTypeFactory will not be called with null values.
>
>
> Oh, if I understand avoiding creating Some(defaultValue) would prevent the problem I just described ?
Yes.
Maxime Lévesque
This is the 'null' case. In the 'master' branch the customTypeFactory is called with a null value and creates an instance of the CustomType with a defaultValue (here: 0.0). Now a InvocationTargetException is raised which is caused by the assertion 'weight must be positive, got 0.0'. Unfortunately the target exception is not visible in the stack-trace. This is why I catch the InvocationTargetException in the 'invoke' helper method and re-throw the target exception.
AFAIK the customTypeFactory shouldn't be called at all with null values from 'FieldMetaData.set', because the result should be None not Some(new CustomType(defaultValue)).
Cool, so it looks like your patch will solve this problem that is currently in master, as well as issue 93, thanks for your effort !