rzo1 commented on code in PR #144: URL: https://github.com/apache/openjpa/pull/144#discussion_r3437864119
########## openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/jdbc/meta/TestJDBCSchemaManager.java: ########## @@ -0,0 +1,143 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.openjpa.jdbc.meta; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.UUID; + +import org.apache.openjpa.jdbc.conf.JDBCConfiguration; +import org.apache.openjpa.jdbc.sql.DBDictionary; +import org.apache.openjpa.jdbc.sql.OracleDictionary; +import org.apache.openjpa.persistence.OpenJPAEntityManagerFactorySPI; +import org.apache.openjpa.persistence.OpenJPAEntityManagerSPI; +import org.apache.openjpa.persistence.test.AbstractPersistenceTestCase; +import org.junit.Test; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; +import jakarta.persistence.SchemaValidationException; + +/** + * Test that a {@link MappingTool#ACTION_REFRESH} uses the right + * types for new columns and takes any mapping in DBDictionary into account. + */ +public class TestJDBCSchemaManager extends AbstractPersistenceTestCase { + /** + * First we create a schema mapping with boolean representation as CHAR(1). + * Then we create an entry. + * After that we create a diff from the entity to the current DB. + * This should result in an empty diff. + */ + @Test + public void testSchemaReset() throws Exception { + EntityManagerFactory emf = createEMF(UUIDEntity.class, DROP_TABLES); + EntityManager em = emf.createEntityManager(); + + assertNotNull(em); + + em.getTransaction().begin(); + UUIDEntity ue1 = new UUIDEntity(); + ue1.setValue("Something"); + em.persist(ue1); + + em.getTransaction().commit(); + UUID id = ue1.getId(); + closeEM(em); + + EntityManager em2 = emf.createEntityManager(); + assertNotNull(em2); + + UUIDEntity ue2 = em2.find(UUIDEntity.class, id); + assertNotNull(ue2); + assertNotEquals(ue1, ue2); + + closeEM(em2); + + closeEMF(emf); + } + + @Test + public void testBuildSchema() throws IOException, SQLException { + EntityManagerFactory emf = createEMF(UUIDEntity.class, EntityBoolChar.class, DROP_TABLES); + emf.createEntityManager(); + + emf.getSchemaManager().create(false); + emf.getSchemaManager().drop(false); + } + + @Test + public void testTruncate() { + EntityManagerFactory emf = createEMF(UUIDEntity.class, EntityBoolChar.class, DROP_TABLES); + EntityManager em = emf.createEntityManager(); + + em.getTransaction().begin(); + UUIDEntity ue1 = new UUIDEntity(); + ue1.setValue("Something"); + em.persist(ue1); + + em.getTransaction().commit(); + UUIDEntity ue2 = em.find(UUIDEntity.class, ue1.getId()); + assertNotNull(ue2); + + closeEM(em); + + emf.getSchemaManager().truncate(); + + em = emf.createEntityManager(); + assertNull(em.find(UUIDEntity.class, ue1.getId())); + closeEM(em); + } + + @Test + public void testValidate() { + OpenJPAEntityManagerFactorySPI emf = createEMF(UUIDEntity.class, EntityBoolChar.class, DROP_TABLES);; + EntityManager em = emf.createEntityManager(); + + em.getTransaction().begin(); + UUIDEntity ue1 = new UUIDEntity(); + ue1.setValue("Something"); + em.persist(ue1); + em.getTransaction().commit(); + + Connection conn = (Connection) ((OpenJPAEntityManagerSPI) em.getDelegate()).getConnection(); + + JDBCConfiguration conf = ((JDBCConfiguration) emf.getConfiguration()); + DBDictionary dict = conf.getDBDictionaryInstance(); + + try (Statement stmt = conn.createStatement()) { + stmt.executeUpdate("ALTER TABLE UUIDEntity DROP COLUMN value_"); + stmt.executeUpdate("ALTER TABLE UUIDEntity ADD " + (dict instanceof OracleDictionary ? "" : "COLUMN") + " value_ BOOLEAN DEFAULT FALSE"); Review Comment: The reason validate() stops throwing once you swap BOOLEAN → INT is MappingInfo.mergeColumn(). When the live column is numeric (INT → Oracle NUMBER → reflected as NUMERIC) and the mapping expects VARCHAR, there's a branch that silently upgrades numeric → VARCHAR instead of reporting drift. That branch was added for the @MapKeyEnumerated ORDINAL/STRING shared-column case, so it's intentional but it also defeats validate()'s drift detection here. It's the same thin already documented for PostgreSQL in PostgresDictionary.newColumn() (PG reports bool as Types.BIT, which is "numeric" to that code too) -> there we remap bool → Types.BOOLEAN so the drift is seen again. We can't reuse that trick on Oracle: pre-23c there's no native BOOLEAN, and an INT column genuinely is a NUMBER, so numeric-vs-VARCHAR is exactly the case the code is built to tolerate. So rather than a version guard, we could make the test induce a drift that's genuinely incompatible with the String → VARCHAR2 mapping on every Oracle version: replace the column with a BLOB on Oracle (keep BOOLEAN elsewhere). OracleDictionary.getColumns() reports BLOB as Types.BLOB, which is neither varchar- nor numeric-compatible, so validate() throws as expected. Something like: ``` boolean isOracle = dict instanceof OracleDictionary; stmt.executeUpdate("ALTER TABLE UUIDEntity DROP COLUMN value_"); if (isOracle) { // Oracle has no native BOOLEAN before 23c, and an INT/NUMBER replacement // is silently coerced to VARCHAR by MappingInfo.mergeColumn (the numeric<->varchar // tolerance added for @MapKeyEnumerated), so validate() wouldn't detect the drift. // BLOB is genuinely incompatible with the String -> VARCHAR mapping on every Oracle version. stmt.executeUpdate("ALTER TABLE UUIDEntity ADD value_ BLOB"); } else { stmt.executeUpdate("ALTER TABLE UUIDEntity ADD COLUMN value_ BOOLEAN DEFAULT FALSE"); } ``` The alternative is to fix numeric↔varchar coercion in mergeColumn on adapt; coerce when building/adapting a schema (adapt=true), but throw when validating (adapt=false). Then validate() becomes strict on every DB, the Oracle INT drift is correctly detected, and the test needs no DB-specific branch at all. Something in that way maybe? wdyt? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
