[
https://issues.apache.org/jira/browse/OPENJPA-399?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12534211
]
Kevin Sutter commented on OPENJPA-399:
--------------------------------------
Comments on the patches... Overall, the concept and logic looks good. I just
have a few questions/comments about the proposed code changes. Ping me if you
have any questions on my abbreviated comments. Thanks.
o The restriction about requiring schema names for all tables if you use
schema names for at least one of the entities using the same table name... I'm
okay with this restriction, but is there anyway that we could detect this
condition? That is, if we detect the use of a "null" schema with other
existing schema names, couldn't we flag this a configuration error?
o Any updated patches should be attached to the Issue with the same name.
JIRA keeps track of the older versions automatically, so you don't have to
continually add a "version number" to your patch files.
o The testcases should be provided as part of the patch file. Makes it much
easier to merge the new files into an existing project. It also removes the
confusion with different project names (ie. openjpa10x_2 doesn't match my
project name).
o The testcases need the Apache licensing... :-) Take a look, you'll
understand.
o In TableJDBCSeq, the getStatus method updates are interesting. So, the
previous implementation of this method didn't act on the input parameter at
all? That's strange. Also, the javadoc for this method indicates that the
input parameter (mapping) could be null. Is it okay for your code to use
"null" as a key for the HashMap?
o In that same file, why not just initialize _stat to the new HashMap()
(similar to the original "new Status()")? And, the original declaration was
final. If you use an initializer, couldn't we go back to the "final"
declaration? Maybe I'm missing the reasons for your current _stat
initialization.
o In the addSchema() method, there's no longer a "fast path" return
conditional at the beginning of the method. Isn't there any means of bypassing
all of the processing if the table/schema already exists?
o I'm not following your code at the end of the addSchema() method with the
"Index idx" processing. It looks like this index processing is a side-effect
of calling addSchema(). Is that your intent? At a minimum, this requires
additional comments, but maybe it requires some re-factoring to make this
clearer.
o The processing of generating the tableName is repeated in at least three
areas of TableJDBCSeq. Could a common utility method be used instead so as not
to repeat the code?
o I don't understand the comments for Column.resetTableName(). Doesn't
"reset" mean to modify an existing set, yet the comment indicates that this can
only be called on columns without a table set.
o The cleanup in LocalConstraint.addColumn() looks good.
o The logic in SchemaGroup.findTable and SchemaGroup.findSequence seems to be
very close. Can any of this code be shared in a private utility method?
That's it,
Kevin
> openjpa did not handle multiple schema names with same table name
> -----------------------------------------------------------------
>
> Key: OPENJPA-399
> URL: https://issues.apache.org/jira/browse/OPENJPA-399
> Project: OpenJPA
> Issue Type: Bug
> Components: jdbc
> Affects Versions: 1.0.1
> Environment: JDK1.5, OPENJPA verison 580425
> Reporter: Teresa Kan
> Assignee: Teresa Kan
> Attachments: OPENJPA_399.patch, OPENJPA_399_2.patch,
> TestMultipleSchemaNames.zip
>
>
> Two entities have the same table name but with different schema, only one
> table is created. In addition, when two entities use the generatedType.AUTO
> for ID, only one OPENJPA_SEQUENCE-TABLE is created.
> The problem due to the SchemaGroup.findTable() which only looked for a table
> name from all the schemas. Once the table was found in one of the schema then
> it exited and assumed that the table existed. Same problem in the
> TableJDBCSeq.addSchema().
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
