persistence@glassfish.java.net

Re: Code review for issue 1324: toplink.refresh query hint does not cascade

From: Wonseok Kim <guruwons_at_gmail.com>
Date: Fri, 22 Jun 2007 23:37:52 +0900

Hi Chris,

I have an idea of new query hint to modify cascade policy. This enables
application use dynamic refresh policy such as no-cascading or cascade-all
in addition to default cascade-by-mapping. With this hint application can
refresh related entities or do not cascade at all when required regardless
of static "cascade=REFRESH" option. This could be done with TopLink
customizer or TopLink API, but query hint would be more convenient.

When I look into DatabaseQuery several cascade policies are defined like
below, but I'm not sure we should support all this policies. For refresh
read query NoCascading, CascadeAllParts and CascadeByMapping seem to be
meaningful.
-----
    /* Used as default for read, means shallow write for modify. */
    public static final int NoCascading = 1;

    /* Used as default for write, used for refreshing to refresh the whole
object. */
    public static final int CascadePrivateParts = 2;

    /* Currently not supported, used for deep write/refreshes/reads in the
future. */
    public static final int CascadeAllParts = 3;

    /* Used by the unit of work. */
    public static final int CascadeDependentParts = 4;

    /* Used by aggregate Collections: As aggregates delete at update time,
cascaded deletes
     * must know to stop when entering postDelete for a particular mapping.
Only used by the
     * aggregate collection when update is occuring in a UnitOfWork
     * CR 2811
     */
    public static final int CascadeAggregateDelete = 5;

    /*
     * Used when refreshing should check the mappings to determin if a
particular
     * mapping should be cascaded.
     */
    public static final int CascadeByMapping = 6;
-----
I'm curious this policy could be exploited for queries other than read query
with refresh hint.
Depending on the usability of the cascade we could could either "
toplink.cascade-policy" or "toplink.refresh.cascade-policy" as a hint name.

What do you think?

Regards,
Wonseok


On 6/22/07, Christopher Delahunt <christopher.delahunt_at_oracle.com> wrote:
>
> Looks good.
>
> Thanks Wonseok for getting this in.
>
>
> Best Regards,
> Chris
>
> ----- Original Message -----
> *From:* Wonseok Kim <guruwons_at_gmail.com>
> *To:* Christopher Delahunt <christopher.delahunt_at_oracle.com>
> *Sent:* Thursday, June 21, 2007 11:06 AM
> *Subject:* Re: Code review for issue 1324: toplink.refresh query hint does
> not cascade
>
> OK, then I will apply this to EJBQueryImpl.buildEJBQLDatabaseQuery as
> below diff.
> What do you think?
>
> EJBQueryImpl.java
> ===================================================================
> RCS file:
> /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java,v
> retrieving revision 1.39
> diff -u -r1.39 EJBQueryImpl.java
> ---
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java
> 22 May 2007 23:54:23 -0000 1.39
> +++
> src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java
> 21 Jun 2007 15:04:01 -0000
> @@ -221,7 +221,13 @@
> parseTree.addParametersToQuery(databaseQuery);
>
> ((EJBQLCallQueryMechanism)databaseQuery.getQueryMechanism()).getEJBQLCall().setIsParsed(true);
>
> databaseQuery.setFlushOnExecute(flushOnExecute);
> -
> +
> + //GF#1324 toplink.refresh query hint does not cascade
> + //cascade by mapping as default for read query
> + if(databaseQuery.isReadQuery ()) {
> + databaseQuery.cascadeByMapping();
> + }
> +
> // apply any query hints
> applyHints(hints, databaseQuery);
>
> Regards,
> Wonseok
>
> On 6/21/07, Christopher Delahunt <christopher.delahunt_at_oracle.com> wrote:
> >
> > Hi Wonseok,
> >
> > Someone mentioned to me that we will be/are reusing the parser for other
> > things in the future (cmp2 support etc) so its better if the
> > cascadeByMapping is only in the JPA classes, rather than in the select
> > node. buildEJBQLDatabaseQuery is probably the best spot, with a check if
> > the query is a read query.
> >
> >
> > Best Regards,
> > Chris
> >
> > ----- Original Message -----
> > *From:* Wonseok Kim <guruwons_at_gmail.com>
> > *To:* persistence_at_glassfish.dev.java.net
> > *Sent:* Thursday, June 21, 2007 10:20 AM
> > *Subject:* Re: Code review for issue 1324: toplink.refresh query hint
> > does not cascade
> >
> > Hi Chris,
> >
> > I applied cascadeByMapping() as default like you mentioned when creating
> > ReportQuery in SelectNode. I think this cascade policy should be applied
> > only to Read query. Please review again the attached fix.
> >
> > Thanks,
> > Wonseok
> >
> > On 6/21/07, Christopher Delahunt <christopher.delahunt_at_oracle.com>
> > wrote:
> > >
> > > Hello Wonseok,
> > >
> > > My concern is that by setting the cascadeByMapping option with the
> > > hint, it will override any settings users might have set manually (through
> > > customizers or direct toplink apis) on the query still wishing to set the
> > > query to refresh dynamically. It might be better to always have the
> > > JPQL queries use the cascadeByMapping in the create
> > > or buildEJBQLDatabaseQuery method, as this will ensure that all refresh
> > > queries use this option unless a user specifically changes it.
> > >
> > > Best Regards,
> > > Chris
> > >
> > >
> > >
> > > ----- Original Message -----
> > > *From:* Wonseok Kim <guruwons_at_gmail.com>
> > > *To:* persistence_at_glassfish.dev.java.net
> > > *Sent:* Wednesday, June 20, 2007 11:15 AM
> > > *Subject:* Code review for issue 1324: toplink.refresh query hint does
> > > not cascade
> > >
> > > Hi Tom, Chris
> > >
> > > I'm fixing the following issue. Please review the attached fix and let
> > > me know if you have any concern.
> > > https://glassfish.dev.java.net/issues/show_bug.cgi?id=1324
> > > <https://glassfish.dev.java.net/issues/show_bug.cgi?id=1324+>
> > >
> > > I confirmed cascade refresh works as expected in my manual local
> > > test(modified data manually between transactions and confirmed the result of
> > > the query with refresh hint).
> > > Also, I added some assertion code in entity-persistence-tests to check
> > > the DatabaseQuery's cascade policy.
> > >
> > > Thanks,
> > > Wonseok
> > >
> > >
> >
>