You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@openjpa.apache.org by mi...@apache.org on 2007/04/03 04:48:11 UTC
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/
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);
+ }
}
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/
Posted by Patrick Linskey <pl...@bea.com>.
> On the other hand, I think there are cases where we can
> include limited
> functionality in the current release and then complete the function in
> release+1. An example of this is what I intended to do in
> OpenJPA-179.
I agree completely. In the case of OPENJPA-179, while I acknowledge that
Abe has some good points about improvements in it, I'm not so worried
about the feature going in. We may need to do more work on it, but (to
my knowledge) it's an improvement over where things are right now, and
it's not an API change, so it won't really have any long-term downsides.
We just need to fix it more later on.
Similarly, in the past, we have introduced partial new features that
took multiple releases to finish completely; that works out fine, as
long as we don't end up creating backwards-compatibility conflicts by
doing so. Which, of course, involves careful planning of how the feature
should be accessed.
-Patrick
--
Patrick Linskey
BEA Systems, Inc.
_______________________________________________________________________
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.
> -----Original Message-----
> From: Michael Dick [mailto:michael.d.dick@gmail.com]
> Sent: Tuesday, April 03, 2007 2:17 PM
> To: open-jpa-dev@incubator.apache.org
> 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/pers
> istence/jdbc/
>
> Thanks Patrick,
>
> That's exactly what I needed. I'll add a testcase shortly.
>
> I read through your response to one of Dave's commits and I
> think it applies
> to mine as well. Rather than hijacking that thread, I'm re-posting the
> relevant part here along with my response :
>
>
> I don't think that we should rush this change in just so it's
> in 0.9.7;
> > we can always delay 0.9.7 if it's a must-have, or have a 0.9.8 that
> > follows soon thereafter. Generally-speaking, my experience
> is that once
> > code gets committed, it tends to not get cleaned up, so the argument
> > "let's get it in for the release and then fix it later"
> usually ends up
> > turning into "let's get it in for the release". This seems
> especially
> > true for a change like this, that is exposing new APIs (the hints).
> >
> Thoughts?
>
>
> I agree. We need to try and avoid the commit and fix later
> mentality. If the
> code is not ready then it shouldn't be committed or the
> release should wait
> until the code is ready.
>
> On the other hand, I think there are cases where we can
> include limited
> functionality in the current release and then complete the function in
> release+1. An example of this is what I intended to do in
> OpenJPA-179. My
> intent was to include the support for default schemas which
> effected only
> the entities in orm.xml in 0.9.7, and then add support for
> persistence-unit
> wide defaults in 0.9.8. As Abe pointed out the code wasn't
> ready to be
> committed, and it needs to either be made ready or excluded
> from 0.9.8.
>
> For the record, I can go either way regarding OpenJPA-179
> with one exception
> - I'm unlikely to finish the persistence unit wide changes in the near
> future, so if that's required it would be better to leave the
> function out
> of the release.
>
>
>
> On 4/3/07, Patrick Linskey <pl...@bea.com> wrote:
> >
> > Hi,
> >
> > You might want to take a look at the SQLListenerTestCase --
> it's very
> > useful for ensuring that the right SQL is issued by a given command.
> >
> > That, in conjunction with direct invocations of
> MappingTool.run () with a
> > Configuration obtained from the test case's EMF, and JDK1.4 regex
> > support, should make it pretty simple to put together a
> unit test case.
> >
> > -Patrick
> >
> > --
> > Patrick Linskey
> > BEA Systems, Inc.
> >
> >
> ______________________________________________________________
> _________
> > 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.
> >
> > > -----Original Message-----
> > > From: Michael Dick [mailto: michael.d.dick@gmail.com]
> > > Sent: Tuesday, April 03, 2007 11:55 AM
> > > To: open-jpa-dev@incubator.apache.org
> > > 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/pers
> > > istence/jdbc/
> > >
> > > 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 <aw...@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
> > >
> >
> > 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
>
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.
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/
Posted by Michael Dick <mi...@gmail.com>.
Thanks Patrick,
That's exactly what I needed. I'll add a testcase shortly.
I read through your response to one of Dave's commits and I think it applies
to mine as well. Rather than hijacking that thread, I'm re-posting the
relevant part here along with my response :
I don't think that we should rush this change in just so it's in 0.9.7;
> we can always delay 0.9.7 if it's a must-have, or have a 0.9.8 that
> follows soon thereafter. Generally-speaking, my experience is that once
> code gets committed, it tends to not get cleaned up, so the argument
> "let's get it in for the release and then fix it later" usually ends up
> turning into "let's get it in for the release". This seems especially
> true for a change like this, that is exposing new APIs (the hints).
>
Thoughts?
I agree. We need to try and avoid the commit and fix later mentality. If the
code is not ready then it shouldn't be committed or the release should wait
until the code is ready.
On the other hand, I think there are cases where we can include limited
functionality in the current release and then complete the function in
release+1. An example of this is what I intended to do in OpenJPA-179. My
intent was to include the support for default schemas which effected only
the entities in orm.xml in 0.9.7, and then add support for persistence-unit
wide defaults in 0.9.8. As Abe pointed out the code wasn't ready to be
committed, and it needs to either be made ready or excluded from 0.9.8.
For the record, I can go either way regarding OpenJPA-179 with one exception
- I'm unlikely to finish the persistence unit wide changes in the near
future, so if that's required it would be better to leave the function out
of the release.
On 4/3/07, Patrick Linskey <pl...@bea.com> wrote:
>
> Hi,
>
> You might want to take a look at the SQLListenerTestCase -- it's very
> useful for ensuring that the right SQL is issued by a given command.
>
> That, in conjunction with direct invocations of MappingTool.run () with a
> Configuration obtained from the test case's EMF, and JDK1.4 regex
> support, should make it pretty simple to put together a unit test case.
>
> -Patrick
>
> --
> Patrick Linskey
> BEA Systems, Inc.
>
> _______________________________________________________________________
> 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.
>
> > -----Original Message-----
> > From: Michael Dick [mailto: michael.d.dick@gmail.com]
> > Sent: Tuesday, April 03, 2007 11:55 AM
> > To: open-jpa-dev@incubator.apache.org
> > 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/pers
> > istence/jdbc/
> >
> > 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 <aw...@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
> >
>
> 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
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/
Posted by Patrick Linskey <pl...@bea.com>.
Hi,
You might want to take a look at the SQLListenerTestCase -- it's very
useful for ensuring that the right SQL is issued by a given command.
That, in conjunction with direct invocations of MappingTool.run() with a
Configuration obtained from the test case's EMF, and JDK1.4 regex
support, should make it pretty simple to put together a unit test case.
-Patrick
--
Patrick Linskey
BEA Systems, Inc.
_______________________________________________________________________
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.
> -----Original Message-----
> From: Michael Dick [mailto:michael.d.dick@gmail.com]
> Sent: Tuesday, April 03, 2007 11:55 AM
> To: open-jpa-dev@incubator.apache.org
> 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/pers
> istence/jdbc/
>
> 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 <aw...@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
>
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.
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/
Posted by Michael Dick <mi...@gmail.com>.
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 <aw...@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
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/
Posted by Abe White <aw...@bea.com>.
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.
2. MappingInfo uses the default schema name sometimes (line 445) but
ignores it other times (line 469).
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.
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.
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.