You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "Kevin Sutter (JIRA)" <ji...@apache.org> on 2007/08/13 18:11:30 UTC
[jira] Commented: (OPENJPA-313) list of objects returned by query
partially correct
[ https://issues.apache.org/jira/browse/OPENJPA-313?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519452 ]
Kevin Sutter commented on OPENJPA-313:
--------------------------------------
Catalina,
Thanks for the patch. A couple of questions and/or comments though...
It looks like this updated code is only if we're working with an OpenJPAID oid and subs is true. I can understand the oid part, but what exactly is the subs boolean used for? I know this wasn't your original code, but a quick glance at the code shows a need for a javadoc update at least. You must have spent some time figuring out how this parameter is used. It would be helpful to put add to the javadoc for this method to explain what the subs parameter is used for (at least). Thanks.
Also, I really dislike empty catch clauses. I can't tell if this was on purpose or an oversight. It also forces me to think through the logic whether this exception clause was necessary or not. I think the empty clause is okay, but we should at least put a comment in the clause indicating why we can safely ignore this exception.
I also personally don't like the idea of comparing strings for the strategy type, but it looks like we're just following suit from the existing code. We can only change so much at a time, I suppose... :-)
Thanks again,
Kevin
> list of objects returned by query partially correct
> ---------------------------------------------------
>
> Key: OPENJPA-313
> URL: https://issues.apache.org/jira/browse/OPENJPA-313
> Project: OpenJPA
> Issue Type: Bug
> Components: jdbc
> Reporter: Catalina Wei
> Fix For: 1.0.0
>
> Attachments: jpa1.0.0.patch, OPENJPA-313.r564688.patch
>
>
> We have an object inheritance hierarchy as follows.
> FixedAnnuity extends Annuity
> EquityAnnuity extends Annuity
> Then we have following code:
> List<IAnnuity> annuities = getServerAdapter().findHolderAnnuities(holder);
> from which we expect to have a list of "Annuity" (could be Annuity, FixedAnnuity or
> EquityAnnuity) from following code
> EntityManager em = null;
> try{
> em = factory.createEntityManager();
> Query query = em.createNamedQuery("GetHolderAnnuities");
> query.setParameter("holderId", holder.getId());
> return (List<IAnnuity>) query.getResultList();
> }
> Here is the query
> <named-query name="GetHolderAnnuities">
> <query>SELECT a FROM Annuity AS a WHERE a.annuityHolderId = :holderId</query>
> </named-query>
> In the end, the list returned only the first object with the correct Object, the
> rest of the objects all casted into the basic type "Annuity"
> e.g. we have following code:
> EntityManager em = null;
> try{
> em = factory.createEntityManager();
> Query query = em.createNamedQuery("GetHolderAnnuities");
> query.setParameter("holderId", holder.getId());
> return (List<IAnnuity>) query.getResultList();
> }
>
> FixedAnnuity fixed1 = new FixedAnnuity();
> fixed1.setHolder(holder);
> EquityAnnuity equity1 = new EquityAnnuity();
> equity1.setHolder(holder);
> FixedAnnuity fixed1 = new FixedAnnuity();
> fixed2.setHolder(holder);
> EquityAnnuity equity2 = new EquityAnnuity();
> equity2.setHolder(holder);
>
> List<IAnnuity> annuities = getServerAdapter().findHolderAnnuities(holder);
>
> Only annuities.get(0) returns the correct object as FixedAnnuity, the other 3
> objects returned all returned as Annuity
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (OPENJPA-313) list of objects returned by query partially correct
Posted by catalina wei <ca...@gmail.com>.
Hi Craig,
I have added block comments to the latest OPENJPA-313.r564688.patch (dated
8/13).
I have no authority to delete my previously attached patches.
Catalina
On 8/13/07, Craig L Russell <Cr...@sun.com> wrote:
>
> Hi Catalina,
>
> On Aug 13, 2007, at 6:20 PM, catalina wei wrote:
>
> > The method I am changing is a private method, so no javadoc is
> > required.
>
> I'm not sure what this statement means. Javadoc is not solely for the
> purpose of documenting public methods. It's a way of helping
> developers who in future will have to look at the code and try to
> figure out if the behavior they're seeing is correct or not in context.
>
> I'd really like to see the comments below incorporated into the code.
> Whether it's in javadoc or comments contained in the body of the
> method is a matter of style, but let's make it easier for future
> developers.
>
> Regards,
>
> Craig
> >
> > subs flag indicates whether a relation is non-polymorphic relation
> > or not.
> > In this test scenario, we are dealing with a polymorphic relation,
> > so the
> > type should be determined by the discriminator value.
> >
> > The discriminator value can be null in the database because this
> > column is
> > nullable (our mapping tool does not enforce it as 'not null'
> > column, so it
> > is possible that other non-jpa application insert a row with null
> > discriminator value ). We do not want a null discriminator value
> > end up with
> > an Exception. So the empty catch block is simply ignore
> > potential data related error in fetching the discriminator value.
> > Essentially, if the discriminator is not present, we want the type
> > initially
> > set in oid to be used
> > (which is the ClassMapping cls passed in to the getObjectId method).
> > Otherwise set the correct type in the oid based on the
> > discriminator value.
> >
> > Catalina
>
> Craig Russell
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
> 408 276-5638 mailto:Craig.Russell@sun.com
> P.S. A good JDO? O, Gasp!
>
>
>
Re: [jira] Commented: (OPENJPA-313) list of objects returned by query
partially correct
Posted by Craig L Russell <Cr...@Sun.COM>.
Hi Catalina,
On Aug 13, 2007, at 6:20 PM, catalina wei wrote:
> The method I am changing is a private method, so no javadoc is
> required.
I'm not sure what this statement means. Javadoc is not solely for the
purpose of documenting public methods. It's a way of helping
developers who in future will have to look at the code and try to
figure out if the behavior they're seeing is correct or not in context.
I'd really like to see the comments below incorporated into the code.
Whether it's in javadoc or comments contained in the body of the
method is a matter of style, but let's make it easier for future
developers.
Regards,
Craig
>
> subs flag indicates whether a relation is non-polymorphic relation
> or not.
> In this test scenario, we are dealing with a polymorphic relation,
> so the
> type should be determined by the discriminator value.
>
> The discriminator value can be null in the database because this
> column is
> nullable (our mapping tool does not enforce it as 'not null'
> column, so it
> is possible that other non-jpa application insert a row with null
> discriminator value ). We do not want a null discriminator value
> end up with
> an Exception. So the empty catch block is simply ignore
> potential data related error in fetching the discriminator value.
> Essentially, if the discriminator is not present, we want the type
> initially
> set in oid to be used
> (which is the ClassMapping cls passed in to the getObjectId method).
> Otherwise set the correct type in the oid based on the
> discriminator value.
>
> Catalina
Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:Craig.Russell@sun.com
P.S. A good JDO? O, Gasp!
Re: [jira] Commented: (OPENJPA-313) list of objects returned by query partially correct
Posted by catalina wei <ca...@gmail.com>.
On 8/13/07, Kevin Sutter (JIRA) <ji...@apache.org> wrote:
>
>
> [
> https://issues.apache.org/jira/browse/OPENJPA-313?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519452]
>
> Kevin Sutter commented on OPENJPA-313:
> --------------------------------------
>
> Catalina,
> Thanks for the patch. A couple of questions and/or comments though...
>
> It looks like this updated code is only if we're working with an OpenJPAID
> oid and subs is true. I can understand the oid part, but what exactly is
> the subs boolean used for? I know this wasn't your original code, but a
> quick glance at the code shows a need for a javadoc update at least. You
> must have spent some time figuring out how this parameter is used. It would
> be helpful to put add to the javadoc for this method to explain what the
> subs parameter is used for (at least). Thanks.
The method I am changing is a private method, so no javadoc is required.
subs flag indicates whether a relation is non-polymorphic relation or not.
In this test scenario, we are dealing with a polymorphic relation, so the
type should be determined by the discriminator value.
The discriminator value can be null in the database because this column is
nullable (our mapping tool does not enforce it as 'not null' column, so it
is possible that other non-jpa application insert a row with null
discriminator value ). We do not want a null discriminator value end up with
an Exception. So the empty catch block is simply ignore
potential data related error in fetching the discriminator value.
Essentially, if the discriminator is not present, we want the type initially
set in oid to be used
(which is the ClassMapping cls passed in to the getObjectId method).
Otherwise set the correct type in the oid based on the discriminator value.
Catalina
Also, I really dislike empty catch clauses. I can't tell if this was on
> purpose or an oversight. It also forces me to think through the logic
> whether this exception clause was necessary or not. I think the empty
> clause is okay, but we should at least put a comment in the clause
> indicating why we can safely ignore this exception.
>
> I also personally don't like the idea of comparing strings for the
> strategy type, but it looks like we're just following suit from the existing
> code. We can only change so much at a time, I suppose... :-)
>
> Thanks again,
> Kevin
>
> > list of objects returned by query partially correct
> > ---------------------------------------------------
> >
> > Key: OPENJPA-313
> > URL: https://issues.apache.org/jira/browse/OPENJPA-313
> > Project: OpenJPA
> > Issue Type: Bug
> > Components: jdbc
> > Reporter: Catalina Wei
> > Fix For: 1.0.0
> >
> > Attachments: jpa1.0.0.patch, OPENJPA-313.r564688.patch
> >
> >
> > We have an object inheritance hierarchy as follows.
> > FixedAnnuity extends Annuity
> > EquityAnnuity extends Annuity
> > Then we have following code:
> > List<IAnnuity> annuities =
> getServerAdapter().findHolderAnnuities(holder);
> > from which we expect to have a list of "Annuity" (could be Annuity,
> FixedAnnuity or
> > EquityAnnuity) from following code
> > EntityManager em = null;
> > try{
> > em = factory.createEntityManager();
> > Query query = em.createNamedQuery("GetHolderAnnuities");
> > query.setParameter("holderId", holder.getId());
> > return (List<IAnnuity>) query.getResultList();
> > }
> > Here is the query
> > <named-query name="GetHolderAnnuities">
> > <query>SELECT a FROM Annuity AS a WHERE a.annuityHolderId =
> :holderId</query>
> > </named-query>
> > In the end, the list returned only the first object with the correct
> Object, the
> > rest of the objects all casted into the basic type "Annuity"
> > e.g. we have following code:
> > EntityManager em = null;
> > try{
> > em = factory.createEntityManager();
> > Query query = em.createNamedQuery("GetHolderAnnuities");
> > query.setParameter("holderId", holder.getId());
> > return (List<IAnnuity>) query.getResultList();
> > }
> >
> > FixedAnnuity fixed1 = new FixedAnnuity();
> > fixed1.setHolder(holder);
> > EquityAnnuity equity1 = new EquityAnnuity();
> > equity1.setHolder(holder);
> > FixedAnnuity fixed1 = new FixedAnnuity();
> > fixed2.setHolder(holder);
> > EquityAnnuity equity2 = new EquityAnnuity();
> > equity2.setHolder(holder);
> >
> > List<IAnnuity> annuities =
> getServerAdapter().findHolderAnnuities(holder);
> >
> > Only annuities.get(0) returns the correct object as FixedAnnuity, the
> other 3
> > objects returned all returned as Annuity
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>