Skip to content

Commit a938032

Browse files
kevinmaretekevinmarete
authored
[DT-3865] Restore dataset alias sequence with migration fix (#3017)
1 parent 0744df7 commit a938032

5 files changed

Lines changed: 335 additions & 3 deletions

File tree

‎src/main/java/org/broadinstitute/consent/http/db/DatasetDAO.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,9 +132,9 @@ public interface DatasetDAO extends Transactional<DatasetDAO> {
132132
"""
133133
INSERT INTO dataset
134134
(name, create_date, create_user_id, update_date,
135-
update_user_id, object_id, dac_id, alias, data_use)
136-
(SELECT :name, :createDate, :createUserId, :createDate,
137-
:createUserId, :objectId, :dacId, COALESCE(MAX(alias),0)+1, :dataUse FROM dataset)
135+
update_user_id, object_id, dac_id, data_use)
136+
VALUES (:name, :createDate, :createUserId, :createDate,
137+
:createUserId, :objectId, :dacId, :dataUse)
138138
""")
139139
@GetGeneratedKeys
140140
Integer insertDataset(

‎src/main/resources/changelog-master.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,5 +255,6 @@
255255
<include file="changesets/changelog-consent-2026-06-18-consolidate-so-approval-fields.xml" relativeToChangelogFile="true" />
256256
<include file="changesets/changelog-consent-2026-07-14-election-type.xml" relativeToChangelogFile="true" />
257257
<include file="changesets/changelog-consent-2026-08-07-so-dashboard-indices.xml" relativeToChangelogFile="true" />
258+
<include file="changesets/changelog-consent-2026-08-10-dataset-alias-sequence.xml" relativeToChangelogFile="true" />
258259
<include file="changesets/changelog-consent-2026-08-11-researcher-dashboard-indices.xml" relativeToChangelogFile="true" />
259260
</databaseChangeLog>

‎src/main/resources/changesets/changelog-consent-2026-08-10-dataset-alias-sequence.xml‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
2+
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
3+
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
4+
https://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.33.xsd">
5+
<changeSet id="changelog-consent-2026-08-10-dataset-alias-sequence" author="kmarete">
6+
<!-- Aliases are public identifiers, so unsafe legacy values must be reconciled explicitly
7+
rather than silently rewritten by this migration. Alias 0 is a valid legacy identifier. -->
8+
<preConditions onFail="HALT" onError="HALT"
9+
onFailMessage="Dataset aliases must be unique, non-null integers between 0 and 2147483646 before migration">
10+
<dbms type="postgresql"/>
11+
<sqlCheck expectedResult="0">
12+
SELECT
13+
(SELECT COUNT(*) FROM dataset
14+
WHERE alias IS NULL
15+
OR alias &lt; 0
16+
OR alias &gt; 2147483646
17+
OR alias != trunc(alias))
18+
+
19+
(SELECT COUNT(*) FROM (
20+
SELECT alias FROM dataset GROUP BY alias HAVING COUNT(*) &gt; 1
21+
) duplicate_aliases)
22+
</sqlCheck>
23+
</preConditions>
24+
25+
<comment>
26+
Allocate public dataset aliases with a database sequence. The trigger is a rolling-deployment
27+
compatibility mechanism: it replaces aliases supplied by old application instances as well as
28+
allocating aliases for new instances that omit the column.
29+
</comment>
30+
31+
<!-- Keep writes out until the sequence is positioned and the trigger and constraints exist. -->
32+
<sql>LOCK TABLE dataset IN ACCESS EXCLUSIVE MODE</sql>
33+
34+
<createSequence sequenceName="dataset_alias_seq" startValue="1" incrementBy="1"/>
35+
<sql>
36+
SELECT setval(
37+
'dataset_alias_seq',
38+
(COALESCE((SELECT MAX(alias) FROM dataset), 0) + 1)::bigint,
39+
false
40+
)
41+
</sql>
42+
43+
<sql splitStatements="false">
44+
CREATE OR REPLACE FUNCTION allocate_dataset_alias() RETURNS trigger AS $$
45+
BEGIN
46+
NEW.alias := nextval('dataset_alias_seq');
47+
RETURN NEW;
48+
END;
49+
$$ LANGUAGE plpgsql
50+
</sql>
51+
<sql splitStatements="false">
52+
CREATE TRIGGER dataset_alias_allocate
53+
BEFORE INSERT ON dataset
54+
FOR EACH ROW EXECUTE FUNCTION allocate_dataset_alias()
55+
</sql>
56+
57+
<addNotNullConstraint tableName="dataset" columnName="alias" columnDataType="bigint"/>
58+
<addUniqueConstraint tableName="dataset" columnNames="alias"
59+
constraintName="dataset_alias_unique"/>
60+
<sql>
61+
ALTER TABLE dataset
62+
ADD CONSTRAINT dataset_alias_valid_integer
63+
CHECK (alias &gt;= 0 AND alias &lt;= 2147483647 AND alias = trunc(alias))
64+
</sql>
65+
66+
<rollback>
67+
<sql>ALTER TABLE dataset DROP CONSTRAINT dataset_alias_valid_integer</sql>
68+
<dropUniqueConstraint tableName="dataset" constraintName="dataset_alias_unique"/>
69+
<dropNotNullConstraint tableName="dataset" columnName="alias" columnDataType="bigint"/>
70+
<sql>DROP TRIGGER IF EXISTS dataset_alias_allocate ON dataset</sql>
71+
<sql>DROP FUNCTION IF EXISTS allocate_dataset_alias()</sql>
72+
<dropSequence sequenceName="dataset_alias_seq"/>
73+
<addDefaultValue tableName="dataset" columnName="alias" columnDataType="bigint"
74+
defaultValueNumeric="0"/>
75+
</rollback>
76+
</changeSet>
77+
</databaseChangeLog>

‎src/test/java/org/broadinstitute/consent/http/db/DatasetAliasSequenceMigrationTest.java‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
package org.broadinstitute.consent.http.db;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertFalse;
5+
import static org.junit.jupiter.api.Assertions.assertNull;
6+
import static org.junit.jupiter.api.Assertions.assertThrows;
7+
import static org.junit.jupiter.api.Assertions.assertTrue;
8+
9+
import java.sql.Connection;
10+
import java.sql.DriverManager;
11+
import java.sql.ResultSet;
12+
import java.sql.SQLException;
13+
import java.sql.Statement;
14+
import liquibase.Contexts;
15+
import liquibase.LabelExpression;
16+
import liquibase.Liquibase;
17+
import liquibase.database.Database;
18+
import liquibase.database.DatabaseFactory;
19+
import liquibase.database.jvm.JdbcConnection;
20+
import liquibase.exception.LiquibaseException;
21+
import liquibase.resource.ClassLoaderResourceAccessor;
22+
import org.junit.jupiter.api.AfterAll;
23+
import org.junit.jupiter.api.BeforeAll;
24+
import org.junit.jupiter.api.BeforeEach;
25+
import org.junit.jupiter.api.Test;
26+
import org.junit.jupiter.params.ParameterizedTest;
27+
import org.junit.jupiter.params.provider.ValueSource;
28+
import org.testcontainers.containers.PostgreSQLContainer;
29+
30+
class DatasetAliasSequenceMigrationTest {
31+
32+
private static final String CHANGELOG =
33+
"changesets/changelog-consent-2026-08-10-dataset-alias-sequence.xml";
34+
private static PostgreSQLContainer<?> postgres;
35+
36+
@BeforeAll
37+
static void startPostgres() {
38+
postgres = new PostgreSQLContainer<>(DAOTestHelper.POSTGRES_IMAGE);
39+
postgres.start();
40+
}
41+
42+
@AfterAll
43+
static void stopPostgres() {
44+
postgres.stop();
45+
}
46+
47+
@BeforeEach
48+
void createPreMigrationSchema() throws SQLException {
49+
try (Connection connection = connection();
50+
Statement statement = connection.createStatement()) {
51+
statement.execute("DROP SCHEMA public CASCADE");
52+
statement.execute("CREATE SCHEMA public");
53+
// Dev's legacy alias column is numeric, which exercises the setval bigint cast.
54+
statement.execute(
55+
"CREATE TABLE dataset (dataset_id bigserial PRIMARY KEY, alias numeric DEFAULT 0)");
56+
statement.execute("INSERT INTO dataset (alias) VALUES (42), (900000), (0)");
57+
}
58+
}
59+
60+
@Test
61+
void migrationPreservesAliasesAndAllocatesAboveMaximumForOldAndNewWriters() throws Exception {
62+
update();
63+
64+
assertEquals(42, queryLong("SELECT alias FROM dataset WHERE dataset_id = 1"));
65+
assertEquals(900000, queryLong("SELECT alias FROM dataset WHERE dataset_id = 2"));
66+
assertEquals(0, queryLong("SELECT alias FROM dataset WHERE dataset_id = 3"));
67+
68+
// An old instance supplies its MAX(alias) + 1 result, but the compatibility trigger replaces
69+
// it.
70+
assertEquals(
71+
900001,
72+
queryLong(
73+
"INSERT INTO dataset (alias) "
74+
+ "SELECT COALESCE(MAX(alias), 0) + 1 FROM dataset RETURNING alias"));
75+
execute("DELETE FROM dataset WHERE alias = 900001");
76+
// A new instance omits alias entirely.
77+
assertEquals(900002, queryLong("INSERT INTO dataset DEFAULT VALUES RETURNING alias"));
78+
79+
assertThrows(
80+
SQLException.class, () -> execute("UPDATE dataset SET alias = NULL WHERE dataset_id = 1"));
81+
assertThrows(
82+
SQLException.class, () -> execute("UPDATE dataset SET alias = 42 WHERE dataset_id = 2"));
83+
assertThrows(
84+
SQLException.class, () -> execute("UPDATE dataset SET alias = -1 WHERE dataset_id = 1"));
85+
assertThrows(
86+
SQLException.class, () -> execute("UPDATE dataset SET alias = 1.5 WHERE dataset_id = 1"));
87+
assertThrows(
88+
SQLException.class,
89+
() -> execute("UPDATE dataset SET alias = 2147483648 WHERE dataset_id = 1"));
90+
execute("UPDATE dataset SET alias = 2147483647 WHERE dataset_id = 1");
91+
assertEquals(2147483647, queryLong("SELECT alias FROM dataset WHERE dataset_id = 1"));
92+
}
93+
94+
@Test
95+
void migrationAllocatesLastIntegerAliasWhenExistingMaximumLeavesRoom() throws Exception {
96+
execute("UPDATE dataset SET alias = 2147483646 WHERE dataset_id = 2");
97+
98+
update();
99+
100+
assertEquals(2147483647, queryLong("INSERT INTO dataset DEFAULT VALUES RETURNING alias"));
101+
assertThrows(
102+
SQLException.class, () -> queryLong("INSERT INTO dataset DEFAULT VALUES RETURNING alias"));
103+
}
104+
105+
@ParameterizedTest
106+
@ValueSource(strings = {"NULL", "-1", "1.5", "42", "2147483647"})
107+
void migrationRejectsUnsafeExistingAliasesBeforeChangingSchema(String unsafeAlias)
108+
throws Exception {
109+
execute("INSERT INTO dataset (alias) VALUES (" + unsafeAlias + ")");
110+
111+
assertThrows(LiquibaseException.class, this::update);
112+
113+
assertNull(queryObject("SELECT to_regclass('dataset_alias_seq')"));
114+
assertFalse(
115+
queryBoolean(
116+
"SELECT EXISTS (SELECT 1 FROM pg_trigger WHERE tgname = 'dataset_alias_allocate')"));
117+
assertFalse(
118+
queryBoolean(
119+
"SELECT EXISTS (SELECT 1 FROM pg_constraint "
120+
+ "WHERE conname = 'dataset_alias_valid_integer')"));
121+
}
122+
123+
@Test
124+
void rollbackRestoresLegacyDefaultAndWriterBehavior() throws Exception {
125+
update();
126+
rollback();
127+
128+
assertNull(queryObject("SELECT to_regclass('dataset_alias_seq')"));
129+
assertFalse(
130+
queryBoolean(
131+
"SELECT EXISTS (SELECT 1 FROM pg_trigger WHERE tgname = 'dataset_alias_allocate')"));
132+
assertFalse(
133+
queryBoolean(
134+
"SELECT EXISTS (SELECT 1 FROM pg_constraint "
135+
+ "WHERE conname = 'dataset_alias_valid_integer')"));
136+
assertTrue(
137+
queryBoolean(
138+
"SELECT is_nullable = 'YES' FROM information_schema.columns "
139+
+ "WHERE table_schema = 'public' AND table_name = 'dataset' AND column_name = 'alias'"));
140+
assertEquals(
141+
"0",
142+
queryObject(
143+
"SELECT pg_get_expr(adbin, adrelid) FROM pg_attrdef "
144+
+ "WHERE adrelid = 'dataset'::regclass AND adnum = "
145+
+ "(SELECT attnum FROM pg_attribute WHERE attrelid = 'dataset'::regclass AND attname = 'alias')"));
146+
147+
assertEquals(0, queryLong("INSERT INTO dataset DEFAULT VALUES RETURNING alias"));
148+
assertEquals(7, queryLong("INSERT INTO dataset (alias) VALUES (7) RETURNING alias"));
149+
}
150+
151+
private static Connection connection() throws SQLException {
152+
return DriverManager.getConnection(
153+
postgres.getJdbcUrl(), postgres.getUsername(), postgres.getPassword());
154+
}
155+
156+
private void update() throws Exception {
157+
try (Connection connection = connection()) {
158+
Database database =
159+
DatabaseFactory.getInstance()
160+
.findCorrectDatabaseImplementation(new JdbcConnection(connection));
161+
try (Liquibase liquibase =
162+
new Liquibase(CHANGELOG, new ClassLoaderResourceAccessor(), database)) {
163+
liquibase.update(new Contexts(), new LabelExpression());
164+
}
165+
}
166+
}
167+
168+
private void rollback() throws Exception {
169+
try (Connection connection = connection()) {
170+
Database database =
171+
DatabaseFactory.getInstance()
172+
.findCorrectDatabaseImplementation(new JdbcConnection(connection));
173+
try (Liquibase liquibase =
174+
new Liquibase(CHANGELOG, new ClassLoaderResourceAccessor(), database)) {
175+
liquibase.rollback(1, new Contexts(), new LabelExpression());
176+
}
177+
}
178+
}
179+
180+
private void execute(String sql) throws SQLException {
181+
try (Connection connection = connection();
182+
Statement statement = connection.createStatement()) {
183+
statement.execute(sql);
184+
}
185+
}
186+
187+
private long queryLong(String sql) throws SQLException {
188+
return ((Number) queryObject(sql)).longValue();
189+
}
190+
191+
private boolean queryBoolean(String sql) throws SQLException {
192+
return (Boolean) queryObject(sql);
193+
}
194+
195+
private Object queryObject(String sql) throws SQLException {
196+
try (Connection connection = connection();
197+
Statement statement = connection.createStatement();
198+
ResultSet resultSet = statement.executeQuery(sql)) {
199+
resultSet.next();
200+
return resultSet.getObject(1);
201+
}
202+
}
203+
}

‎src/test/java/org/broadinstitute/consent/http/db/DatasetDAOTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@
2323
import java.util.Random;
2424
import java.util.Set;
2525
import java.util.UUID;
26+
import java.util.concurrent.CountDownLatch;
27+
import java.util.concurrent.ExecutorService;
28+
import java.util.concurrent.Executors;
29+
import java.util.concurrent.Future;
30+
import java.util.concurrent.TimeUnit;
2631
import java.util.stream.IntStream;
2732
import org.broadinstitute.consent.http.enumeration.ElectionStatus;
2833
import org.broadinstitute.consent.http.enumeration.ElectionType;
@@ -59,6 +64,52 @@
5964
@ExtendWith(MockitoExtension.class)
6065
class DatasetDAOTest extends DAOTestHelper {
6166

67+
@Test
68+
void testInsertDatasetAllocatesUniqueAliasesConcurrently() throws Exception {
69+
int insertCount = 16;
70+
User user = createUser();
71+
Timestamp now = Timestamp.from(Instant.now());
72+
DataUse dataUse = new DataUseBuilder().setGeneralUse(true).build();
73+
CountDownLatch ready = new CountDownLatch(insertCount);
74+
CountDownLatch start = new CountDownLatch(1);
75+
76+
try (ExecutorService executor = Executors.newFixedThreadPool(insertCount)) {
77+
List<Future<Integer>> inserts =
78+
IntStream.range(0, insertCount)
79+
.mapToObj(
80+
index ->
81+
executor.submit(
82+
() -> {
83+
ready.countDown();
84+
if (!start.await(10, TimeUnit.SECONDS)) {
85+
throw new IllegalStateException("Timed out waiting to start insert");
86+
}
87+
return datasetDAO.insertDataset(
88+
"Concurrent dataset " + index,
89+
now,
90+
user.getUserId(),
91+
"concurrent-object-" + index,
92+
dataUse.toString(),
93+
null);
94+
}))
95+
.toList();
96+
97+
try {
98+
assertTrue(ready.await(10, TimeUnit.SECONDS));
99+
} finally {
100+
start.countDown();
101+
}
102+
Set<Integer> aliases = new HashSet<>();
103+
for (Future<Integer> insert : inserts) {
104+
Dataset dataset = datasetDAO.findDatasetById(insert.get(10, TimeUnit.SECONDS));
105+
assertNotNull(dataset.getAlias());
106+
aliases.add(dataset.getAlias());
107+
}
108+
109+
assertEquals(insertCount, aliases.size());
110+
}
111+
}
112+
62113
@Test
63114
void testFindDatasetWithoutFSOInformation() {
64115
// The query under test specifically excludes FSO information

0 commit comments

Comments
 (0)