openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Patrick Linskey" <plins...@gmail.com>
Subject Re: svn commit: r573750 - in /openjpa/trunk: openjpa-kernel/src/main/java/org/apache/openjpa/kernel/ openjpa-kernel/src/main/java/org/apache/openjpa/meta/ openjpa-persistence/src/main/java/org/apache/openjpa/persistence/
Date Sat, 08 Sep 2007 19:44:44 GMT
Some comments about the commit:

> +    public boolean setContainedBy(FetchGroup parent) {
> +    public String[] getContainedBy() {

- Personally, I like to add @since tags to any new public methods. I
think I add them to protected methods sometimes too. I don't think
that we have a hard-and-fast policy about this, especially for
internally-facing packages, but I'm hoping to sway as many people as
possible to my way of thinking.

- Why does setContainedBy() return a boolean? It doesn't look like we
use the return value; seems simpler to just leave it out.
Additionally, if we do need it, we should document what it means.

- Why does getContainedBy() return an array? Creating an array is
slow, and they're more difficult to work with than lists usually. If
you're just trying to protect the internal representation, it'd be
better to return a Collection, and make the body just do 'return
Collections.unmodifiableSet(...)' instead of doing the arraycopy.

-Patrick

> +    public boolean setContainedBy(FetchGroup parent) {
> +       parent.addDeclaredInclude(this.getName());
> +       if (_containedBy==null)
> +               _containedBy = new HashSet();
> +       return _containedBy.add(parent.getName());
> +    }
> +
> +    /**
> +     * Gets the name of the fetch groups in which this receiver has been
> +     * included.
> +     *
> +     * @see #setContainedBy(FetchGroup)
> +     */
> +    public String[] getContainedBy() {


On 9/7/07, ppoddar@apache.org <ppoddar@apache.org> wrote:
> Author: ppoddar
> Date: Fri Sep  7 16:08:02 2007
> New Revision: 573750
>
> URL: http://svn.apache.org/viewvc?rev=573750&view=rev
> Log:
> Fix for FetchGroup inclusion and recursion depth calculation.
>
> Modified:
>     openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java
>     openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/meta/FetchGroup.java
>     openjpa/trunk/openjpa-persistence/src/main/java/org/apache/openjpa/persistence/AnnotationPersistenceMetaDataParser.java
>
> Modified: openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java
> URL: http://svn.apache.org/viewvc/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java?rev=573750&r1=573749&r2=573750&view=diff
> ==============================================================================
> --- openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java
(original)
> +++ openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/FetchConfigurationImpl.java
Fri Sep  7 16:08:02 2007
> @@ -604,6 +604,8 @@
>          int cur;
>          for (int i = 0; max != FetchGroup.DEPTH_INFINITE
>              && i < groups.length; i++) {
> +            // ignore custom groups that are inactive in this configuration
> +            if (!this.hasFetchGroup(groups[i])) continue;
>              cur = meta.getFetchGroup(groups[i]).getRecursionDepth(fm);
>              if (cur == FetchGroup.DEPTH_INFINITE || cur > max)
>                  max = cur;
> @@ -625,7 +627,7 @@
>              return avail;
>          return Math.min(max, avail);
>      }
> -
> +
>      /**
>       * Return the relation type of the given field.
>       */
>
> Modified: openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/meta/FetchGroup.java
> URL: http://svn.apache.org/viewvc/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/meta/FetchGroup.java?rev=573750&r1=573749&r2=573750&view=diff
> ==============================================================================
> --- openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/meta/FetchGroup.java
(original)
> +++ openjpa/trunk/openjpa-kernel/src/main/java/org/apache/openjpa/meta/FetchGroup.java
Fri Sep  7 16:08:02 2007
> @@ -21,9 +21,11 @@
>  import java.io.Serializable;
>  import java.util.ArrayList;
>  import java.util.HashMap;
> +import java.util.HashSet;
>  import java.util.Iterator;
>  import java.util.List;
>  import java.util.Map;
> +import java.util.Set;
>
>  import org.apache.commons.lang.StringUtils;
>  import org.apache.commons.lang.ObjectUtils;
> @@ -74,6 +76,7 @@
>      private final ClassMetaData _meta;
>      private final boolean _readOnly;
>      private List _includes;
> +    private Set  _containedBy;
>      private Map _depths;
>      private Boolean _postLoad;
>
> @@ -171,6 +174,32 @@
>              }
>          }
>          return false;
> +    }
> +
> +    /**
> +     * Sets this receiver as one of the included fetch groups of the given
> +     * parent.
> +     * The parent fecth grop must include this receiver before this call.
> +     *
> +     * @see #includes(String, boolean)
> +     * @see #addDeclaredInclude(String)
> +     */
> +    public boolean setContainedBy(FetchGroup parent) {
> +       parent.addDeclaredInclude(this.getName());
> +       if (_containedBy==null)
> +               _containedBy = new HashSet();
> +       return _containedBy.add(parent.getName());
> +    }
> +
> +    /**
> +     * Gets the name of the fetch groups in which this receiver has been
> +     * included.
> +     *
> +     * @see #setContainedBy(FetchGroup)
> +     */
> +    public String[] getContainedBy() {
> +       return (_containedBy == null) ? new String[0]
> +            : (String[]) _containedBy.toArray(new String[_containedBy.size()]);
>      }
>
>      /**
>
> Modified: openjpa/trunk/openjpa-persistence/src/main/java/org/apache/openjpa/persistence/AnnotationPersistenceMetaDataParser.java
> URL: http://svn.apache.org/viewvc/openjpa/trunk/openjpa-persistence/src/main/java/org/apache/openjpa/persistence/AnnotationPersistenceMetaDataParser.java?rev=573750&r1=573749&r2=573750&view=diff
> ==============================================================================
> --- openjpa/trunk/openjpa-persistence/src/main/java/org/apache/openjpa/persistence/AnnotationPersistenceMetaDataParser.java
(original)
> +++ openjpa/trunk/openjpa-persistence/src/main/java/org/apache/openjpa/persistence/AnnotationPersistenceMetaDataParser.java
Fri Sep  7 16:08:02 2007
> @@ -875,6 +875,14 @@
>
>      /**
>       * Create fetch groups.
> +     * If FetchGroup A includes FetchGroup B, then a bi-link is set between
> +     * A and B. Both A and B must be declared in the same Class.
> +     * <br>
> +     * Call {@link #parseFetchAttribute(ClassMetaData,
> +     * org.apache.openjpa.meta.FetchGroup, FetchAttribute) only after the
> +     * bi-links have been established, because a field f will not only add the
> +     * fetch group A which explictly includes f to its custom fetch groups but
> +     * also will also add any fetch group B that includes A.
>       */
>      private void parseFetchGroups(ClassMetaData meta, FetchGroup... groups) {
>          org.apache.openjpa.meta.FetchGroup fg;
> @@ -885,12 +893,25 @@
>              fg = meta.addDeclaredFetchGroup(group.name());
>              if (group.postLoad())
>                  fg.setPostLoad(true);
> -            for (String s : group.fetchGroups())
> +            for (String s : group.fetchGroups()) {
>                  fg.addDeclaredInclude(s);
> +            }
> +        }
> +
> +        for (FetchGroup group:groups) {
> +               fg = meta.getFetchGroup(group.name());
> +               String[] includedFetchGropNames = fg.getDeclaredIncludes();
> +               for (String includedFectchGroupName:includedFetchGropNames)
> +                   meta.getFetchGroup(includedFectchGroupName).setContainedBy(fg);
> +        }
> +
> +        for (FetchGroup group : groups) {
> +            fg = meta.getFetchGroup(group.name());
>              for (FetchAttribute attr : group.attributes())
>                  parseFetchAttribute(meta, fg, attr);
>          }
>      }
> +
>
>      /**
>       * Set a field's fetch group.
> @@ -904,6 +925,9 @@
>                  meta, attr.name()));
>
>          field.setInFetchGroup(fg.getName(), true);
> +        String[] parentFetchGroups = fg.getContainedBy();
> +        for (String parentFetchGroup:parentFetchGroups)
> +               field.setInFetchGroup(parentFetchGroup, true);
>          if (attr.recursionDepth() != Integer.MIN_VALUE)
>              fg.setRecursionDepth(field, attr.recursionDepth());
>      }
>
>
>


-- 
Patrick Linskey
202 669 5907

Mime
View raw message