Re: TRQS321: Criteria Serialization

[Scott Eade <se...@backstagetech.com.au>]

Thomas Fischer wrote:

>I took a quick glance at the issue, and when I uncommented the test case
>you checked in and resolved an issue of visibility (use
>"Criteria.getNewCriterion()" instead of "new Criteria.Criterion"), the test
>case did not produce an error.
>  
>
This is interesting.  What JVM are you using?  I am using J2SE 1.4.2_06
from Sun for Win32.

>Then I looked what the test case does, and it uses Criteria.equals(Object)
>to test whether the two criteria are equal. This is not enough, since
>Criteria.equals does not compare aliases and asColumns. There are two
>possibilities to remedy this:
>- The first is to change criteria.equals, which I did not want to do
>between release candidates, but if somebody else is of another opinion
>here,  we need to discuss it.
>  
>
If equals() is performing its function correctly then this is IMO a bug
and most bugs warrant fixing - even between release candidates (though
preferably not between the last rc and the final release).
The test case is cloning the Criteria object using serialization - it
would only pick up this problem if the Criteria in use actually used
aliases or asColumns which it does not.
As I said earlier, this has been broken (at least for me) for a while, I
wouldn't hold off 3.2 for this issue as nobody spoke up earlier or felt
that it was worth investing their own time to fix (me included).

>- the second one is to write a custom comparison for the test case and
>remove it once Criteria.equals() is fixed. I admit I was too lazy to do it.
>  
>
I certainly wouldn't bother doing this.  Note however that the problem
that is encountered is nothing to do with equals(), it is that the
"value" of the Criterion is incorrectly deserialized - the following
assertion (replace the last line in the test case) fails for me:
        assertEquals(c.getValue("Author.NAME"),
cClone.getValue("Author.NAME"));
        junit.framework.AssertionFailedError: expected:<author> but
was:<Author.NAME='author'>

>So the question is how we should proceed. If you wish, I can check in the
>modified test case which runs for me, and then we can check whether it runs
>for you also.
>Then, I'm going to look at the old mails where the problem is described and
>see whether I can reproduce it. Maybe the problem has dissolved into thin
>air ??? (it is time for another miracle :-) )
>  
>
According to Bloch (Effective Java Item 54) the default serialized form
of an inner class is ill-defined.  This could lead to varying behaviour
between different JVMs.  The answer may be to add custom serialization
code to Criterion.

Scott

-- 
Scott Eade
Backstage Technologies Pty. Ltd.
http://www.backstagetech.com.au


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org

Re: TRQS321: Criteria Serialization

[Thomas Fischer <fi...@seitenbau.net>]

Sorry for my noise. I forgot to add to my last message that the uncommented
test case which is in svn works for jvm 1.5.0_01 on linux.

      Thomas

> > >> I took a quick glance at the issue, and when I uncommented the test
> case
> > >> you checked in and resolved an issue of visibility (use
> > >> "Criteria.getNewCriterion()" instead of "new Criteria.Criterion"),
the
> test
> > >> case did not produce an error.
> > >>
> > >>
> > > This is interesting.  What JVM are you using?  I am using J2SE 1.4.2
_06
> > > from Sun for Win32.


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org

Re: TRQS321: Criteria Serialization

[Thomas Fischer <fi...@seitenbau.net>]

Scott,

I did not see you already provided a patch in scarab. I will look at it
today evening. If noone objects, I will include it into the release.

   Thomas



Thomas Fischer <tf...@apache.org> schrieb am 10.11.2005 09:19:06:

>
>
> On Thu, 10 Nov 2005, Scott Eade wrote:
>
> > Thomas Fischer wrote:
> >
> >> I took a quick glance at the issue, and when I uncommented the test
case
> >> you checked in and resolved an issue of visibility (use
> >> "Criteria.getNewCriterion()" instead of "new Criteria.Criterion"), the
test
> >> case did not produce an error.
> >>
> >>
> > This is interesting.  What JVM are you using?  I am using J2SE 1.4.2_06
> > from Sun for Win32.
>
> I could reproduce the error at a second machine. It uses 1.4.2-05 for
> linux.
> >
> >> Then I looked what the test case does, and it uses
Criteria.equals(Object)
> >> to test whether the two criteria are equal. This is not enough, since
> >> Criteria.equals does not compare aliases and asColumns. There are two
> >> possibilities to remedy this:
> >> - The first is to change criteria.equals, which I did not want to do
> >> between release candidates, but if somebody else is of another opinion
> >> here,  we need to discuss it.
> >>
> >>
> > If equals() is performing its function correctly then this is IMO a bug
> > and most bugs warrant fixing - even between release candidates (though
> > preferably not between the last rc and the final release).
>
> ok, now that I can reproduce the error, I see that Criteria.equals() is
> not the problem (it is not strict enough in my opinion, but this has
> nothing to do with the serialisation problem.)
>
> > ...
> > As I said earlier, this has been broken (at least for me) for a while,
I
> > wouldn't hold off 3.2 for this issue as nobody spoke up earlier or felt
> > that it was worth investing their own time to fix (me included).
>
> If we want to solve this problem, we can as easily fix it before the
> release as after. So let's do it now, if we can.
>
> > According to Bloch (Effective Java Item 54) the default serialized form
> > of an inner class is ill-defined.  This could lead to varying behaviour
> > between different JVMs.  The answer may be to add custom serialization
> > code to Criterion.
>
> I could not get hold of Bloch's book in short time, but according to what

> I have found on the internet, the problem with inner classes only occurs
> for non-static inner classes. So I thought maybe one can fix this by
> making Criterion a static inner class (diff appended), but this does not
> solve the problem (same behaviour as before).
>
> I am puzzled at the moment and need to dissect the problem further to
> understand it. Not sure whether it will happen before the week-end,
though
>
>     Thomas[Anhang "patch.txt" gelöscht von Thomas
> Fischer/kn/seitenbau]
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
> For additional commands, e-mail: torque-dev-help@db.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org

Re: TRQS321: Criteria Serialization

[Thomas Fischer <tf...@apache.org>]

On Thu, 10 Nov 2005, Scott Eade wrote:

> Thomas Fischer wrote:
>
>> I took a quick glance at the issue, and when I uncommented the test case
>> you checked in and resolved an issue of visibility (use
>> "Criteria.getNewCriterion()" instead of "new Criteria.Criterion"), the test
>> case did not produce an error.
>>
>>
> This is interesting.  What JVM are you using?  I am using J2SE 1.4.2_06
> from Sun for Win32.

I could reproduce the error at a second machine. It uses 1.4.2-05 for 
linux.
>
>> Then I looked what the test case does, and it uses Criteria.equals(Object)
>> to test whether the two criteria are equal. This is not enough, since
>> Criteria.equals does not compare aliases and asColumns. There are two
>> possibilities to remedy this:
>> - The first is to change criteria.equals, which I did not want to do
>> between release candidates, but if somebody else is of another opinion
>> here,  we need to discuss it.
>>
>>
> If equals() is performing its function correctly then this is IMO a bug
> and most bugs warrant fixing - even between release candidates (though
> preferably not between the last rc and the final release).

ok, now that I can reproduce the error, I see that Criteria.equals() is 
not the problem (it is not strict enough in my opinion, but this has 
nothing to do with the serialisation problem.)

> ...
> As I said earlier, this has been broken (at least for me) for a while, I
> wouldn't hold off 3.2 for this issue as nobody spoke up earlier or felt
> that it was worth investing their own time to fix (me included).

If we want to solve this problem, we can as easily fix it before the 
release as after. So let's do it now, if we can.

> According to Bloch (Effective Java Item 54) the default serialized form
> of an inner class is ill-defined.  This could lead to varying behaviour
> between different JVMs.  The answer may be to add custom serialization
> code to Criterion.

I could not get hold of Bloch's book in short time, but according to what 
I have found on the internet, the problem with inner classes only occurs 
for non-static inner classes. So I thought maybe one can fix this by 
making Criterion a static inner class (diff appended), but this does not 
solve the problem (same behaviour as before).

I am puzzled at the moment and need to dissect the problem further to 
understand it. Not sure whether it will happen before the week-end, though

    Thomas

Re: TRQS321: Criteria Serialization

[Scott Eade <se...@backstagetech.com.au>]

Thomas,

The Criteria serialization fix patch has been attached to the Scarab
issue at http://issues.apache.org/scarab/issues/id/TRQS321

If 3.2-rc3 hasn't been cut yet then this can be included at your
option.  IMO it should be included as it fixes bugs in equals() and
serialization, but as release manager the call is yours to make.

Scott

-- 
Scott Eade
Backstage Technologies Pty. Ltd.
http://www.backstagetech.com.au


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org

Re: TRQS321: Criteria Serialization

[Scott Eade <se...@backstagetech.com.au>]

Thomas,

I have sorted this out here - I'm just in the process of tidying up the
code.  Given the nature of the problem I am quite surprised that the
commented test case actually passes for you.

As we are in the throws of voting on an RC with a view to releasing RSN
I will post a patch rather than committing the changes.  Given that
Criteria is actually quite broken and that we haven't yet cut rc3 you
could slip this in if you want to.  I will leave the call to you since
you are managing the release.

Scott

-- 
Scott Eade
Backstage Technologies Pty. Ltd.
http://www.backstagetech.com.au


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org