eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456480105
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
@rmannibucau about applying removeDelimiters only when needed:
my original fix was to handle this only in SchemaTool, but just adding the new test case showed that the situation is more complex.
Table class does not have a reference to the DBDictionary and we can't change the signature of all methods, it is kind of Public API that can be overridden by custom DBDictionary implementations
Tests are also failing if you switch to `DBIdentifier.toUpper(DBIdentifier.removeDelimiters(name), true);`
IMHO we could also probably clean up some of those methods in order to reduce useless allocation, but it can be a separate follow up patch.
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
`_colMap.keySet()` contains an internal representation of column names (all uppercased)
this method is used only for error messages and IMHO it is better to have a consistent handling of this internal data structure
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/SchemaTool.java
##########
@@ -1030,10 +1032,7 @@ private void drop(SchemaGroup db, SchemaGroup repos, boolean considerDatabaseSta
continue;
if (dropColumn(cols[k])) {
- if (dbTable != null)
Review comment:
dbTable is always not null here, we have a null check 4 lines above,
I can revert if you want
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org