openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Dick" <michael.d.d...@gmail.com>
Subject Re: svn commit: r525006 - in /incubator/openjpa/trunk: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/ openjpa-persistence-jdbc/src/main/java/org/apache/openjpa/persistence/jdbc/
Date Tue, 03 Apr 2007 18:54:49 GMT
Hi Abe,

The testing I've done was to take a rather simple entity hierarchy and use
the mapping tool to generate sql. Most of the verification was manual
inspection of the sql file. I didn't decide on the best way to test it in a
unit test  - I suppose generating the file and grepping for the expected
results or generating the tables and using native queries to check that they
exist would work. Both approaches seem a bit kludgy to me - maybe there are
better options that haven't occurred to me yet. I'll attach the tests that I
have to the JIRA report rather than cluttering up everyone's inbox.

There are also similar tests in our FVT bucket, which is where we found the
problem. I know there's more cleanup to be done here, but I thought it was
worth getting a first stab at it done in time for 0.9.7.

I've attached a patch with some of the changes you noted. It probably won't
save you much time, but it can't hurt.

Thank you for taking a look at the changes.

On 4/3/07, Abe White <awhite@bea.com> wrote:
>
> I see several possible problems with this commit.  Are there tests
> for this functionality checked in?
>
> 1. The property and logic for using the DefaultSchemaName are defined
> in MappingInfo, but the default schema name is only ever set into
> ClassMappingInfos.  Not FieldMappingInfos, DiscriminatorMappingInfos,
> etc.


Definitely oversight on my part.

2.  MappingInfo uses the default schema name sometimes (line 445) but
> ignores it other times (line 469).


Another good catch - I missed the else clause, but it doesn't seem to
resolve #3.

3. It should not be necessary to prepend the schema name to the table
> name in PersistenceMappingDefaults.  The schema is already known.
> Was that code added based on any testing?  If so, perhaps it is
> because of problem #2 above.


I didn't have much luck without the changes in PersistenceMappingDefaults -
there is a schema object, but the name is null.  There might be something
else that I missed though.

4. The XMLPersistenceMappingParser should not override the endClass
> (String) method.  It already overrides the endClassMapping
> (ClassMetaData) method, which is a much more efficient place to set
> the needed info.  You could also do it in the startClassMapping method.


Agreed, I should have moved it to one of the endClassMapping method.


I'm happy to fix things myself, but I don't see any tests in the
> commit to verify my fixes.
>
> On Apr 2, 2007, at 9:48 PM, mikedd@apache.org wrote:
>
> > Author: mikedd
> > Date: Mon Apr  2 19:48:10 2007
> > New Revision: 525006
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=525006
> > Log:
> > OpenJPA-179 store defaultSchemaName in ClassMapping
> >
> > Modified:
> >     incubator/openjpa/trunk/openjpa-jdbc/src/main/java/org/apache/
> > openjpa/jdbc/meta/MappingInfo.java
> >     incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/java/
> > org/apache/openjpa/persistence/jdbc/PersistenceMappingDefaults.java
> >     incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/java/
> > org/apache/openjpa/persistence/jdbc/XMLPersistenceMappingParser.java
> >
> > Modified: incubator/openjpa/trunk/openjpa-jdbc/src/main/java/org/
> > apache/openjpa/jdbc/meta/MappingInfo.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > jdbc/src/main/java/org/apache/openjpa/jdbc/meta/MappingInfo.java?
> > view=diff&rev=525006&r1=525005&r2=525006
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-jdbc/src/main/java/org/apache/
> > openjpa/jdbc/meta/MappingInfo.java (original)
> > +++ incubator/openjpa/trunk/openjpa-jdbc/src/main/java/org/apache/
> > openjpa/jdbc/meta/MappingInfo.java Mon Apr  2 19:48:10 2007
> > @@ -21,6 +21,7 @@
> >  import java.util.Collections;
> >  import java.util.List;
> >
> > +import org.apache.commons.lang.StringUtils;
> >  import org.apache.openjpa.jdbc.conf.JDBCConfiguration;
> >  import org.apache.openjpa.jdbc.schema.Column;
> >  import org.apache.openjpa.jdbc.schema.ColumnIO;
> > @@ -69,6 +70,7 @@
> >      private boolean _canFK = true;
> >      private int _join = JOIN_NONE;
> >      private ColumnIO _io = null;
> > +    private String _defaultSchemaName = null;
> >
> >      /**
> >       * Mapping strategy name.
> > @@ -439,6 +441,9 @@
> >              if (schema == null) {
> >                  schemaName = Schemas.getNewTableSchema
> > ((JDBCConfiguration)
> >                      repos.getConfiguration());
> > +                if(StringUtils.isEmpty(schemaName)) {
> > +                   schemaName = _defaultSchemaName;
> > +                }
> >                  schema = group.getSchema(schemaName);
> >                  if (schema == null)
> >                      schema = group.addSchema(schemaName);
> > @@ -1764,4 +1769,12 @@
> >          public void populate(Table local, Table foreign, Column col,
> >              Object target, boolean inverse, int pos, int cols);
> >       }
> > +
> > +    public String getDefaultSchemaName() {
> > +        return _defaultSchemaName;
> > +    }
> > +
> > +    public void setDefaultSchemaName(String schemaName) {
> > +        _defaultSchemaName = schemaName;
> > +    }
> >  }
> >
> > Modified: incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/
> > java/org/apache/openjpa/persistence/jdbc/
> > PersistenceMappingDefaults.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > persistence-jdbc/src/main/java/org/apache/openjpa/persistence/jdbc/
> > PersistenceMappingDefaults.java?
> > view=diff&rev=525006&r1=525005&r2=525006
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/java/
> > org/apache/openjpa/persistence/jdbc/PersistenceMappingDefaults.java
> > (original)
> > +++ incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/java/
> > org/apache/openjpa/persistence/jdbc/PersistenceMappingDefaults.java
> > Mon Apr  2 19:48:10 2007
> > @@ -15,6 +15,7 @@
> >   */
> >  package org.apache.openjpa.persistence.jdbc;
> >
> > +import org.apache.commons.lang.StringUtils;
> >  import org.apache.openjpa.jdbc.meta.ClassMapping;
> >  import org.apache.openjpa.jdbc.meta.Discriminator;
> >  import org.apache.openjpa.jdbc.meta.FieldMapping;
> > @@ -114,17 +115,31 @@
> >
> >      @Override
> >      public String getTableName(ClassMapping cls, Schema schema) {
> > +        String name = "";
> > +
> > +        if(StringUtils.isNotEmpty(schema.getName())) {
> > +            name +=schema.getName() + '.';
> > +        }
> > +
> >          if (cls.getTypeAlias() != null)
> > -            return cls.getTypeAlias();
> > +            name += cls.getTypeAlias();
> > +
> >          else
> > -            return Strings.getClassName(
> > -                cls.getDescribedType()).replace('$', '_');
> > +            name += Strings.getClassName(cls.getDescribedType
> > ()).replace('$',
> > +                    '_');
> > +
> > +        return name;
> >      }
> >
> >      @Override
> >      public String getTableName(FieldMapping fm, Schema schema) {
> > +        String name = "";
> > +        if(StringUtils.isNotEmpty(schema.getName())) {
> > +            name +=schema.getName() + '.';
> > +        }
> > +
> >          // base name is table of defining type + '_'
> > -        String name = fm.getDefiningMapping().getTable().getName()
> > + "_";
> > +        name += fm.getDefiningMapping().getTable().getName() + "_";
> >
> >          // if this is an assocation table, spec says to suffix
> > with table of
> >          // the related type. spec doesn't cover other cases; we're
> > going to
> >
> > Modified: incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/
> > java/org/apache/openjpa/persistence/jdbc/
> > XMLPersistenceMappingParser.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > persistence-jdbc/src/main/java/org/apache/openjpa/persistence/jdbc/
> > XMLPersistenceMappingParser.java?
> > view=diff&rev=525006&r1=525005&r2=525006
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/java/
> > org/apache/openjpa/persistence/jdbc/
> > XMLPersistenceMappingParser.java (original)
> > +++ incubator/openjpa/trunk/openjpa-persistence-jdbc/src/main/java/
> > org/apache/openjpa/persistence/jdbc/
> > XMLPersistenceMappingParser.java Mon Apr  2 19:48:10 2007
> > @@ -50,6 +50,7 @@
> >  import org.apache.openjpa.meta.ClassMetaData;
> >  import org.apache.openjpa.meta.FieldMetaData;
> >  import org.apache.openjpa.meta.JavaTypes;
> > +import org.apache.openjpa.meta.MetaDataRepository;
> >  import org.apache.openjpa.persistence.XMLPersistenceMetaDataParser;
> >  import static org.apache.openjpa.persistence.jdbc.MappingTag.*;
> >
> > @@ -910,4 +911,18 @@
> >               TRUE,
> >               FALSE
> >       }
> > +
> > +    @Override
> > +    protected void endClass(String elem)
> > +        throws SAXException {
> > +        if (StringUtils.isNotEmpty(_schema)) {
> > +            Class cls = classForName(currentClassName());
> > +
> > +            MetaDataRepository repos = getRepository();
> > +            ClassMapping meta = (ClassMapping)
> > repos.getCachedMetaData(cls);
> > +
> > +            meta.getMappingInfo().setDefaultSchemaName(_schema);
> > +        }
> > +        super.endClass(elem);
> > +    }
> >  }
> >
> >
>
>
> Notice:  This email message, together with any attachments, may contain
> information  of  BEA Systems,  Inc.,  its subsidiaries  and  affiliated
> entities,  that may be confidential,  proprietary,  copyrighted  and/or
> legally privileged, and is intended solely for the use of the individual or
> entity named in this message. If you are not the intended recipient, and
> have received this message in error, please immediately return this by email
> and then delete it.
>



-- 
-Michael Dick

Mime
View raw message