Repository: ambari Updated Branches: refs/heads/branch-2.4 0f89b863b -> 022285dd3
AMBARI-18057 - NullPointerException When Retrieving Cluster via API Due To Database Inconsistency (jonathanhurley) Project: http://git-wip-us.apache.org/repos/asf/ambari/repo Commit: http://git-wip-us.apache.org/repos/asf/ambari/commit/022285dd Tree: http://git-wip-us.apache.org/repos/asf/ambari/tree/022285dd Diff: http://git-wip-us.apache.org/repos/asf/ambari/diff/022285dd Branch: refs/heads/branch-2.4 Commit: 022285dd3bf7bc6aaf012b96f607922a0abe7c73 Parents: 0f89b86 Author: Jonathan Hurley <[email protected]> Authored: Sun Aug 7 16:07:08 2016 -0400 Committer: Jonathan Hurley <[email protected]> Committed: Mon Aug 8 00:01:57 2016 -0400 ---------------------------------------------------------------------- .../apache/ambari/server/orm/DBAccessor.java | 29 ++++- .../ambari/server/orm/DBAccessorImpl.java | 114 +++++++++++++++---- .../server/orm/entities/AlertCurrentEntity.java | 8 -- .../server/orm/helpers/dbms/DbmsHelper.java | 10 ++ .../orm/helpers/dbms/GenericDbmsHelper.java | 34 ++++++ .../server/orm/helpers/dbms/OracleHelper.java | 16 +++ .../ambari/server/orm/DBAccessorImplTest.java | 75 +++++++++--- 7 files changed, 241 insertions(+), 45 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessor.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessor.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessor.java index 5f126f6..488c01e 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessor.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessor.java @@ -23,6 +23,7 @@ import java.sql.SQLException; import java.util.List; import org.apache.ambari.server.configuration.Configuration.DatabaseType; +import org.eclipse.jdt.internal.compiler.ast.FieldDeclaration; import org.eclipse.persistence.internal.databaseaccess.FieldTypeDefinition; import org.eclipse.persistence.sessions.DatabaseSession; @@ -138,8 +139,22 @@ public interface DBAccessor { String referenceTableName, String[] referenceColumns, boolean ignoreFailure) throws SQLException; + /** - * Add column to existing table + * Adds a column to an existing table. This method will honor the + * {@link DBColumnInfo#isNullable} and {@link DBColumnInfo#getDefaultValue()} + * methods and create a new column that has a {@code DEFAULT} constraint. + * <p/> + * Oracle is, of course, the exception here since their syntax doesn't conform + * to widely accepted SQL standards. Because they switch the order of the + * {@code NULL} constraint and the {@code DEFAULT} constraint, we need to + * create the table in a non-atomic fashion. This is because the EclipseLink + * {@link FieldDeclaration} class hard codes the order of the constraints. In + * the case of Oracle, we alter the nullability of the table after its + * creation. + * <p/> + * No work is performed if the column already exists. + * * @param tableName * @param columnInfo * @throws SQLException @@ -580,6 +595,18 @@ public interface DBAccessor { */ void dropPKConstraint(String tableName, String defaultConstraintName) throws SQLException; + /** + * Adds a default constraint to an existing column. + * + * @param tableName + * the table where the column is defined (not {@code null}). + * @param column + * the column information which contains the default value (not + * {@code null}). + * @throws SQLException + */ + void addDefaultConstraint(String tableName, DBColumnInfo column) throws SQLException; + enum DbType { ORACLE, MYSQL, http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java index ea5f496..bb3f826 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java @@ -550,18 +550,52 @@ public class DBAccessorImpl implements DBAccessor { } + /** + * {@inheritDoc} + */ @Override public void addColumn(String tableName, DBColumnInfo columnInfo) throws SQLException { - if (!tableHasColumn(tableName, columnInfo.getName())) { - //TODO workaround for default values, possibly we will have full support later - if (columnInfo.getDefaultValue() != null) { - columnInfo.setNullable(true); - } - String query = dbmsHelper.getAddColumnStatement(tableName, columnInfo); - executeQuery(query); + if (tableHasColumn(tableName, columnInfo.getName())) { + return; + } + + DatabaseType databaseType = configuration.getDatabaseType(); + switch (databaseType) { + case ORACLE: { + // capture the original null value and set the column to nullable if + // there is a default value + boolean originalNullable = columnInfo.isNullable(); + if (columnInfo.getDefaultValue() != null) { + columnInfo.setNullable(true); + } + + String query = dbmsHelper.getAddColumnStatement(tableName, columnInfo); + executeQuery(query); + + // update the column after it's been created with the default value and + // then set the nullable field back to the specified value + if (columnInfo.getDefaultValue() != null) { + updateTable(tableName, columnInfo.getName(), columnInfo.getDefaultValue(), ""); - if (columnInfo.getDefaultValue() != null) { - updateTable(tableName, columnInfo.getName(), columnInfo.getDefaultValue(), ""); + // if the column wasn't originally nullable, then set that here + if (!originalNullable) { + setColumnNullable(tableName, columnInfo, originalNullable); + } + + // finally, add the DEFAULT constraint to the table + addDefaultConstraint(tableName, columnInfo); + } + break; + } + case DERBY: + case MYSQL: + case POSTGRES: + case SQL_ANYWHERE: + case SQL_SERVER: + default: { + String query = dbmsHelper.getAddColumnStatement(tableName, columnInfo); + executeQuery(query); + break; } } } @@ -733,16 +767,7 @@ public class DBAccessorImpl implements DBAccessor { String whereClause) throws SQLException { StringBuilder query = new StringBuilder(String.format("UPDATE %s SET %s = ", tableName, columnName)); - - // Only String and number supported. - // Taken from: org.eclipse.persistence.internal.databaseaccess.appendParameterInternal - Object dbValue = databasePlatform.convertToDatabaseType(value); - String valueString = value.toString(); - if (dbValue instanceof String) { - valueString = "'" + value.toString() + "'"; - } - - query.append(valueString); + query.append(escapeParameter(value)); query.append(" "); query.append(whereClause); @@ -1200,4 +1225,55 @@ public class DBAccessorImpl implements DBAccessor { dropPKConstraint(tableName, primaryKeyConstraintName, true); } } + + /** + * {@inheritDoc} + */ + @Override + public void addDefaultConstraint(String tableName, DBColumnInfo column) throws SQLException { + String defaultValue = escapeParameter(column.getDefaultValue()); + StringBuilder builder = new StringBuilder(String.format("ALTER TABLE %s ", tableName)); + + DatabaseType databaseType = configuration.getDatabaseType(); + switch (databaseType) { + case DERBY: + case MYSQL: + case POSTGRES: + case SQL_ANYWHERE: + builder.append(String.format("ALTER %s SET DEFAULT %s", column.getName(), defaultValue)); + case ORACLE: + builder.append(String.format("MODIFY %s DEFAULT %s", column.getName(), defaultValue)); + break; + case SQL_SERVER: + builder.append( + String.format("ALTER COLUMN %s SET DEFAULT %s", column.getName(), defaultValue)); + break; + default: + builder.append(String.format("ALTER %s SET DEFAULT %s", column.getName(), defaultValue)); + break; + } + + executeQuery(builder.toString()); + } + + /** + * Gets an escaped version of the specified value suitable for including as a + * parameter when building statements. + * + * @param value + * the value to escape + * @return the escaped value + */ + protected String escapeParameter(Object value) { + // Only String and number supported. + // Taken from: + // org.eclipse.persistence.internal.databaseaccess.appendParameterInternal + Object dbValue = databasePlatform.convertToDatabaseType(value); + String valueString = value.toString(); + if (dbValue instanceof String) { + valueString = "'" + value.toString() + "'"; + } + + return valueString; + } } http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertCurrentEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertCurrentEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertCurrentEntity.java index 77f5acc..b30b100 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertCurrentEntity.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertCurrentEntity.java @@ -284,14 +284,6 @@ public class AlertCurrentEntity { } /** - * @param occurrences - * the occurrences to set - */ - public void setOccurrences(Long occurrences) { - this.occurrences = occurrences; - } - - /** * Gets the firmness of the alert, indicating whether or not it could be a * potential false positive. * http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/DbmsHelper.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/DbmsHelper.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/DbmsHelper.java index 30c06fb..1fe65db 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/DbmsHelper.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/DbmsHelper.java @@ -123,4 +123,14 @@ public interface DbmsHelper { * @return the statement (never {@code null}). */ String getSetNullableStatement(String tableName, DBAccessor.DBColumnInfo columnInfo, boolean nullable); + + /** + * Gets whether the database platform supports adding contraints after the + * {@code NULL} constraint. Some database, such as Oracle, don't allow this. + * Unfortunately, EclipsLink hard codes the order of constraints. + * + * @return {@code true} if contraints can be added after the {@code NULL} + * constraint, {@code false} otherwise. + */ + boolean isConstraintSupportedAfterNullability(); } http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/GenericDbmsHelper.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/GenericDbmsHelper.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/GenericDbmsHelper.java index 21fa361..042b4d2 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/GenericDbmsHelper.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/GenericDbmsHelper.java @@ -205,6 +205,11 @@ public class GenericDbmsHelper implements DbmsHelper { int length = columnInfo.getLength() != null ? columnInfo.getLength() : 0; FieldDefinition fieldDefinition = new FieldDefinition(columnInfo.getName(), columnInfo.getType(), length); fieldDefinition.setShouldAllowNull(columnInfo.isNullable()); + + if (null != columnInfo.getDefaultValue() && isConstraintSupportedAfterNullability()) { + fieldDefinition.setConstraint("DEFAULT " + escapeParameter(columnInfo.getDefaultValue())); + } + return fieldDefinition; } @@ -384,4 +389,33 @@ public class GenericDbmsHelper implements DbmsHelper { } }; } + + /** + * {@inheritDoc} + */ + @Override + public boolean isConstraintSupportedAfterNullability() { + return true; + } + + /** + * Gets an escaped version of the specified value suitable for including as a + * parameter when building statements. + * + * @param value + * the value to escape + * @return the escaped value + */ + protected String escapeParameter(Object value) { + // Only String and number supported. + // Taken from: + // org.eclipse.persistence.internal.databaseaccess.appendParameterInternal + Object dbValue = databasePlatform.convertToDatabaseType(value); + String valueString = value.toString(); + if (dbValue instanceof String) { + valueString = "'" + value.toString() + "'"; + } + + return valueString; + } } http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/OracleHelper.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/OracleHelper.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/OracleHelper.java index 88ef8fe..b5955b4 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/OracleHelper.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/helpers/dbms/OracleHelper.java @@ -63,4 +63,20 @@ public class OracleHelper extends GenericDbmsHelper { return statement.toString(); } + /** + * {@inheritDoc} + * <p/> + * Oracle supports the format: + * + * <pre> + * ALTER TABLE foo ADD COLUMN bar varchar2(32) DEFAULT 'baz' NOT NULL + * </pre> + * + * This syntax doesn't allow contraints added after the {@code NULL} + * constraint. + */ + @Override + public boolean isConstraintSupportedAfterNullability() { + return false; + } } http://git-wip-us.apache.org/repos/asf/ambari/blob/022285dd/ambari-server/src/test/java/org/apache/ambari/server/orm/DBAccessorImplTest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/orm/DBAccessorImplTest.java b/ambari-server/src/test/java/org/apache/ambari/server/orm/DBAccessorImplTest.java index ac8bea1..9dcaeef 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/orm/DBAccessorImplTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/orm/DBAccessorImplTest.java @@ -19,9 +19,13 @@ package org.apache.ambari.server.orm; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import static org.junit.matchers.JUnitMatchers.containsString; +import java.io.ByteArrayInputStream; +import java.sql.Clob; +import java.sql.Connection; +import java.sql.DatabaseMetaData; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; @@ -33,8 +37,6 @@ import java.util.Map; import java.util.Vector; import java.util.concurrent.atomic.AtomicInteger; -import junit.framework.Assert; - import org.apache.ambari.server.orm.DBAccessor.DBColumnInfo; import org.eclipse.persistence.sessions.DatabaseSession; import org.junit.After; @@ -45,9 +47,8 @@ import org.junit.rules.ExpectedException; import com.google.inject.Guice; import com.google.inject.Injector; -import java.io.ByteArrayInputStream; -import java.sql.Clob; -import java.sql.PreparedStatement; + +import junit.framework.Assert; public class DBAccessorImplTest { private Injector injector; @@ -494,32 +495,32 @@ public class DBAccessorImplTest { createMyTable(tableName); DBAccessorImpl dbAccessor = injector.getInstance(DBAccessorImpl.class); - dbAccessor.addColumn(tableName, new DBColumnInfo("isNullable", - String.class, 1000, "test", false)); + // create a column with a non-NULL constraint + DBColumnInfo dbColumnInfo = new DBColumnInfo("isNullable", String.class, 1000, "test", false); + dbAccessor.addColumn(tableName, dbColumnInfo); Statement statement = dbAccessor.getConnection().createStatement(); - ResultSet resultSet = statement.executeQuery("SELECT isNullable FROM " - + tableName); + ResultSet resultSet = statement.executeQuery("SELECT isNullable FROM " + tableName); ResultSetMetaData rsmd = resultSet.getMetaData(); - assertEquals(ResultSetMetaData.columnNullable, rsmd.isNullable(1)); + assertEquals(ResultSetMetaData.columnNoNulls, rsmd.isNullable(1)); statement.close(); - dbAccessor.setColumnNullable(tableName, new DBColumnInfo("isNullable", - String.class, 1000, "test", false), false); + // set it to nullable + dbAccessor.setColumnNullable(tableName, dbColumnInfo, true); statement = dbAccessor.getConnection().createStatement(); resultSet = statement.executeQuery("SELECT isNullable FROM " + tableName); rsmd = resultSet.getMetaData(); - assertEquals(ResultSetMetaData.columnNoNulls, rsmd.isNullable(1)); + assertEquals(ResultSetMetaData.columnNullable, rsmd.isNullable(1)); statement.close(); - dbAccessor.setColumnNullable(tableName, new DBColumnInfo("isNullable", - String.class, 1000, "test", false), true); + // set it back to non-NULL + dbAccessor.setColumnNullable(tableName, dbColumnInfo, false); statement = dbAccessor.getConnection().createStatement(); resultSet = statement.executeQuery("SELECT isNullable FROM " + tableName); rsmd = resultSet.getMetaData(); - assertEquals(ResultSetMetaData.columnNullable, rsmd.isNullable(1)); + assertEquals(ResultSetMetaData.columnNoNulls, rsmd.isNullable(1)); statement.close(); } @@ -532,4 +533,44 @@ public class DBAccessorImplTest { Statement customSchemaTableCreation = dbAccessor.getConnection().createStatement(); customSchemaTableCreation.execute(toString().format("Create table %s.%s (id int, time int)", schemaName, tableName)); } + + /** + * Checks to ensure that columns created with a default have the correct + * DEFAULT constraint value set in. + * + * @throws Exception + */ + @Test + public void testDefaultColumnConstraintOnAddColumn() throws Exception { + String tableName = getFreeTableName().toUpperCase(); + String columnName = "COLUMN_WITH_DEFAULT_VALUE"; + + createMyTable(tableName); + DBAccessorImpl dbAccessor = injector.getInstance(DBAccessorImpl.class); + + // create a column with a non-NULL constraint that has a default value + DBColumnInfo dbColumnInfo = new DBColumnInfo(columnName, String.class, 32, "foo", false); + dbAccessor.addColumn(tableName, dbColumnInfo); + + String schema = null; + Connection connection = dbAccessor.getConnection(); + DatabaseMetaData databaseMetadata = connection.getMetaData(); + ResultSet schemaResultSet = databaseMetadata.getSchemas(); + if (schemaResultSet.next()) { + schema = schemaResultSet.getString(1); + } + + schemaResultSet.close(); + + String columnDefaultVal = null; + ResultSet rs = databaseMetadata.getColumns(null, schema, tableName, columnName); + + if (rs.next()) { + columnDefaultVal = rs.getString("COLUMN_DEF"); + } + + rs.close(); + + assertEquals("'foo'", columnDefaultVal); + } }
