Skip to content

Commit

Permalink
fix!: EXPOSED-691 [PostgreSQL] Restrict dropping unmapped sequences t…
Browse files Browse the repository at this point in the history
…o related tables only (#2357)

* fix: [PostgreSQL] Restrict dropping unmapped sequences to related tables only

- Add existingSequences() method that checks for sequences that are directly
related to tables provided as arguments. This is currently only feasible with
PostgreSQL.
- Switch MigrationUtils method to use this instead of sequences() when generating
SQL to drop sequences.
- Adjust tests accordingly.

* fix!: EXPOSED-691 [PostgreSQL] Restrict dropping unmapped sequences to related tables only

- Update KDocs & Breaking changes doc

* fix!: EXPOSED-691 [PostgreSQL] Restrict dropping unmapped sequences to related tables only

- Remove investigative tests
- Update tests with found issue link
  • Loading branch information
bog-walk authored Jan 22, 2025
1 parent 4c52764 commit 576e1d5
Show file tree
Hide file tree
Showing 11 changed files with 213 additions and 10 deletions.
6 changes: 6 additions & 0 deletions documentation-website/Writerside/topics/Breaking-Changes.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Breaking Changes

## 0.59.0
* [PostgreSQL] `MigrationUtils.statementsRequiredForDatabaseMigration(*tables)` used to potentially return `DROP` statements for any database sequence not
mapped to an Exposed table object. Now it only checks against database sequences that have a relational dependency on any of the specified tables
(for example, any sequence automatically associated with a `SERIAL` column registered to `IdTable`). An unbound sequence created manually
via the `CREATE SEQUENCE` command will no longer be checked and will not generate a `DROP` statement.

## 0.57.0
* Insert, Upsert, and Replace statements will no longer implicitly send all default values (except for client-side default values) in every SQL request.
This change will reduce the amount of data Exposed sends to the database and make Exposed rely more on the database's default values.
Expand Down
5 changes: 5 additions & 0 deletions exposed-core/api/exposed-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -2226,6 +2226,7 @@ public final class org/jetbrains/exposed/sql/Sequence {
public final fun getMinValue ()Ljava/lang/Long;
public final fun getName ()Ljava/lang/String;
public final fun getStartWith ()Ljava/lang/Long;
public fun toString ()Ljava/lang/String;
}

public abstract class org/jetbrains/exposed/sql/SetOperation : org/jetbrains/exposed/sql/AbstractQuery {
Expand Down Expand Up @@ -3564,6 +3565,7 @@ public abstract class org/jetbrains/exposed/sql/statements/api/ExposedDatabaseMe
public abstract fun columns ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public abstract fun existingIndices ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public abstract fun existingPrimaryKeys ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public abstract fun existingSequences ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public final fun getDatabase ()Ljava/lang/String;
public abstract fun getDatabaseDialectName ()Ljava/lang/String;
public abstract fun getDatabaseProductVersion ()Ljava/lang/String;
Expand Down Expand Up @@ -3837,6 +3839,7 @@ public abstract interface class org/jetbrains/exposed/sql/vendors/DatabaseDialec
public abstract fun dropSchema (Lorg/jetbrains/exposed/sql/Schema;Z)Ljava/lang/String;
public abstract fun existingIndices ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public abstract fun existingPrimaryKeys ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public abstract fun existingSequences ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public abstract fun getDataTypeProvider ()Lorg/jetbrains/exposed/sql/vendors/DataTypeProvider;
public abstract fun getDatabase ()Ljava/lang/String;
public abstract fun getDefaultReferenceOption ()Lorg/jetbrains/exposed/sql/ReferenceOption;
Expand Down Expand Up @@ -3888,6 +3891,7 @@ public final class org/jetbrains/exposed/sql/vendors/DatabaseDialect$DefaultImpl
public static fun dropSchema (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;Z)Ljava/lang/String;
public static fun existingIndices (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;[Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public static fun existingPrimaryKeys (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;[Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public static fun existingSequences (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;[Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public static fun getDefaultReferenceOption (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;)Lorg/jetbrains/exposed/sql/ReferenceOption;
public static fun getLikePatternSpecialChars (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;)Ljava/util/Map;
public static fun getNeedsQuotesWhenSymbolsInNames (Lorg/jetbrains/exposed/sql/vendors/DatabaseDialect;)Z
Expand Down Expand Up @@ -4335,6 +4339,7 @@ public abstract class org/jetbrains/exposed/sql/vendors/VendorDialect : org/jetb
public fun dropSchema (Lorg/jetbrains/exposed/sql/Schema;Z)Ljava/lang/String;
public fun existingIndices ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public fun existingPrimaryKeys ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public fun existingSequences ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
protected fun fillConstraintCacheForTables (Ljava/util/List;)V
public final fun filterCondition (Lorg/jetbrains/exposed/sql/Index;)Ljava/lang/String;
protected final fun getAllTableNamesCache ()Ljava/util/Map;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class Sequence(
val identifier
get() = TransactionManager.current().db.identifierManager.cutIfNecessaryAndQuote(name)

override fun toString(): String = "Sequence(identifier=$identifier)"

/** The SQL statements that create this sequence. */
val ddl: List<String>
get() = createStatement()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package org.jetbrains.exposed.sql.statements.api

import org.jetbrains.exposed.sql.ForeignKeyConstraint
import org.jetbrains.exposed.sql.Index
import org.jetbrains.exposed.sql.Sequence
import org.jetbrains.exposed.sql.Table
import org.jetbrains.exposed.sql.vendors.ColumnMetadata
import org.jetbrains.exposed.sql.vendors.PrimaryKeyMetadata
Expand Down Expand Up @@ -64,6 +65,18 @@ abstract class ExposedDatabaseMetadata(val database: String) {
/** Returns a map with the [PrimaryKeyMetadata] in each of the specified [tables]. */
abstract fun existingPrimaryKeys(vararg tables: Table): Map<Table, PrimaryKeyMetadata?>

/**
* Returns a map with all the defined sequences that hold a relation to the specified [tables] in the database.
*
* **Note** PostgreSQL is currently the only database that maps relational dependencies for sequences created when
* a SERIAL column is registered to an `IdTable`. Using this method with any other database returns an empty map.
*
* Any sequence created using the CREATE SEQUENCE command will be ignored
* as it is not necessarily bound to any particular table. Sequences that are used in a table via triggers will also
* not be returned.
*/
abstract fun existingSequences(vararg tables: Table): Map<Table, List<Sequence>>

/** Returns a list of the names of all sequences in the database. */
abstract fun sequences(): List<String>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,18 @@ interface DatabaseDialect {
/** Returns a map with the primary key metadata in each of the specified [tables]. */
fun existingPrimaryKeys(vararg tables: Table): Map<Table, PrimaryKeyMetadata?> = emptyMap()

/**
* Returns a map with all the defined sequences that hold a relation to the specified [tables] in the database.
*
* **Note** PostgreSQL is currently the only database that maps relational dependencies for sequences created when
* a SERIAL column is registered to an `IdTable`. Using this method with any other database returns an empty map.
*
* Any sequence created using the CREATE SEQUENCE command will be ignored
* as it is not necessarily bound to any particular table. Sequences that are used in a table via triggers will also
* not be returned.
*/
fun existingSequences(vararg tables: Table): Map<Table, List<Sequence>> = emptyMap()

/** Returns a list of the names of all sequences in the database. */
fun sequences(): List<String>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ abstract class VendorDialect(
override fun existingPrimaryKeys(vararg tables: Table): Map<Table, PrimaryKeyMetadata?> =
TransactionManager.current().db.metadata { existingPrimaryKeys(*tables) }

override fun existingSequences(vararg tables: Table): Map<Table, List<Sequence>> =
TransactionManager.current().db.metadata { existingSequences(*tables) }

override fun sequences(): List<String> =
TransactionManager.current().db.metadata { sequences() }

Expand Down
1 change: 1 addition & 0 deletions exposed-jdbc/api/exposed-jdbc.api
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public final class org/jetbrains/exposed/sql/statements/jdbc/JdbcDatabaseMetadat
public fun columns ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public fun existingIndices ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public fun existingPrimaryKeys ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public fun existingSequences ([Lorg/jetbrains/exposed/sql/Table;)Ljava/util/Map;
public fun getDatabaseDialectName ()Ljava/lang/String;
public fun getDatabaseProductVersion ()Ljava/lang/String;
public fun getDefaultIsolationLevel ()I
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,63 @@ class JdbcDatabaseMetadataImpl(database: String, val metadata: DatabaseMetaData)
}
}

override fun existingSequences(vararg tables: Table): Map<Table, List<Sequence>> {
if (currentDialect !is PostgreSQLDialect) return emptyMap()

val transaction = TransactionManager.current()
return tables.associateWith { table ->
val (_, tableSchema) = tableCatalogAndSchema(table)
transaction.exec(
"""
SELECT seq_details.sequence_name,
seq_details.start,
seq_details.increment,
seq_details.max,
seq_details.min,
seq_details.cache,
seq_details.cycle
FROM pg_catalog.pg_namespace tns
INNER JOIN pg_catalog.pg_class t ON tns.oid = t.relnamespace AND t.relkind IN ('p', 'r')
INNER JOIN pg_catalog.pg_depend d ON t.oid = d.refobjid
LEFT OUTER JOIN (
SELECT s.relname AS sequence_name,
seq.seqstart AS start,
seq.seqincrement AS increment,
seq.seqmax AS max,
seq.seqmin AS min,
seq.seqcache AS cache,
seq.seqcycle AS cycle,
s.oid AS seq_id
FROM pg_catalog.pg_sequence seq
JOIN pg_catalog.pg_class s ON s.oid = seq.seqrelid AND s.relkind = 'S'
JOIN pg_catalog.pg_namespace sns ON s.relnamespace = sns.oid
WHERE sns.nspname = '$tableSchema'
) seq_details ON seq_details.seq_id = d.objid
WHERE tns.nspname = '$tableSchema' AND t.relname = '${table.nameInDatabaseCaseUnquoted()}'
""".trimIndent()
) { rs ->
val tmpSequences = mutableListOf<Sequence>()
while (rs.next()) {
rs.getString("sequence_name")?.let {
tmpSequences.add(
Sequence(
it,
rs.getLong("start"),
rs.getLong("increment"),
rs.getLong("min"),
rs.getLong("max"),
rs.getBoolean("cycle"),
rs.getLong("cache")
)
)
}
}
rs.close()
tmpSequences
}.orEmpty()
}
}

@Suppress("MagicNumber")
override fun sequences(): List<String> {
val sequences = mutableListOf<String>()
Expand Down
33 changes: 28 additions & 5 deletions exposed-migration/src/main/kotlin/MigrationUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import org.jetbrains.exposed.sql.SchemaUtils.checkExcessiveForeignKeyConstraints
import org.jetbrains.exposed.sql.SchemaUtils.checkExcessiveIndices
import org.jetbrains.exposed.sql.SchemaUtils.checkMappingConsistence
import org.jetbrains.exposed.sql.SchemaUtils.createStatements
import org.jetbrains.exposed.sql.SchemaUtils.statementsRequiredToActualizeScheme
import org.jetbrains.exposed.sql.Sequence
import org.jetbrains.exposed.sql.Table
import org.jetbrains.exposed.sql.exists
import org.jetbrains.exposed.sql.exposedLogger
import org.jetbrains.exposed.sql.transactions.TransactionManager
import org.jetbrains.exposed.sql.vendors.H2Dialect
import org.jetbrains.exposed.sql.vendors.MysqlDialect
import org.jetbrains.exposed.sql.vendors.PostgreSQLDialect
import org.jetbrains.exposed.sql.vendors.SQLiteDialect
import org.jetbrains.exposed.sql.vendors.currentDialect
import java.io.File
Expand Down Expand Up @@ -58,14 +58,22 @@ object MigrationUtils {

/**
* Returns the SQL statements that need to be executed to make the existing database schema compatible with
* the table objects defined using Exposed. Unlike [statementsRequiredToActualizeScheme], DROP/DELETE statements are
* included.
* the table objects defined using Exposed. Unlike `SchemaUtils.statementsRequiredToActualizeScheme()`,
* DROP/DELETE statements are included.
*
* **Note:** Some databases, like **SQLite**, only support `ALTER TABLE ADD COLUMN` syntax in very restricted cases,
* which may cause unexpected behavior when adding some missing columns. For more information,
* refer to the relevant documentation.
* For SQLite, see [ALTER TABLE restrictions](https://www.sqlite.org/lang_altertable.html#alter_table_add_column).
*
* **Note:** If this method is called on a **PostgreSQL** database, it will check for a mapping inconsistency
* between the specified [tables] and existing sequences that have a relational dependency on any of these [tables]
* (for example, any sequence automatically associated with a `SERIAL` column registered to `IdTable`). This means
* that an unbound sequence created manually via the `CREATE SEQUENCE` command will no longer be checked and will
* not generate a DROP statement.
* When called on other databases, such an inconsistency will be checked against all sequences from the database,
* potentially returning DROP statements for any sequence unlinked or unrelated to [tables].
*
* By default, a description for each intermediate step, as well as its execution time, is logged at the INFO level.
* This can be disabled by setting [withLogs] to `false`.
*/
Expand Down Expand Up @@ -139,7 +147,12 @@ object MigrationUtils {

/**
* Log Exposed table mappings <-> real database mapping problems and returns DDL Statements to fix them, including
* DROP/DELETE statements (unlike [checkMappingConsistence])
* DROP/DELETE statements (unlike [checkMappingConsistence]).
*
* **Note:** If this method is called on a PostgreSQL database, only sequences with a relational dependency on any
* of the specified [tables] will be checked for a mapping inconsistency. When called on other databases, all sequences
* from the database will be checked, potentially returning SQL statements to drop any sequences that are unlinked
* or unrelated to [tables].
*/
private fun mappingConsistenceRequiredStatements(vararg tables: Table, withLogs: Boolean = true): List<String> {
return checkMissingIndices(tables = tables, withLogs).flatMap { it.createStatement() } +
Expand Down Expand Up @@ -287,6 +300,7 @@ object MigrationUtils {
}
}

// all possible sequences checked, as 'mappedSequences' is the limiting factor, not 'existingSequencesNames'
val existingSequencesNames: Set<String> = currentDialect.sequences().toSet()

val missingSequences = mutableSetOf<Sequence>()
Expand All @@ -303,6 +317,10 @@ object MigrationUtils {
* Checks all [tables] for any that have sequences that exist in the database but are not mapped in the code. If
* found, this function also logs the SQL statements that can be used to drop these sequences.
*
* **Note:** If this method is called on a PostgreSQL database, only sequences with a relational dependency on any
* of the specified [tables] will be checked for a mapping in Exposed code. When called on other databases, all sequences
* from the database will be checked, potentially returning any [Sequence] unlinked or unrelated to [tables].
*
* @return List of sequences that are unmapped and can be dropped.
*/
private fun checkUnmappedSequences(vararg tables: Table, withLogs: Boolean): List<Sequence> {
Expand All @@ -316,7 +334,12 @@ object MigrationUtils {
}
}

val existingSequencesNames: Set<String> = currentDialect.sequences().toSet()
val existingSequencesNames: Set<String> = if (currentDialect is PostgreSQLDialect) {
// only sequences with related links to [tables] are checked, to avoid dropping every unmapped sequence
currentDialect.existingSequences(*tables).values.flatMap { it.map { it.name } }
} else {
currentDialect.sequences()
}.toSet()

val unmappedSequences = mutableSetOf<Sequence>()

Expand Down
Loading

0 comments on commit 576e1d5

Please sign in to comment.