Skip to content

Commit 7739c08

Browse files
minleejaeminleejae
authored
Keep ALTER key accessors connected to structured index state (#2628)
* Keep ALTER key accessors connected to structured index state * Clarify ALTER key element type for static analysis * Make remaining ALTER key element types explicit * Simplify ALTER key adapters and document replacement semantics
1 parent b3b2084 commit 7739c08

4 files changed

Lines changed: 226 additions & 12 deletions

File tree

‎src/main/java/net/sf/jsqlparser/statement/alter/AlterExpression.java‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -505,20 +505,57 @@ public void setUsingIfExists(boolean usingIfExists) {
505505
this.usingIfExists = usingIfExists;
506506
}
507507

508+
/**
509+
* Returns a snapshot of the rendered key elements for a structured PRIMARY_KEY, or the legacy
510+
* list otherwise. Changing the snapshot does not change the index; use
511+
* {@link #setPkColumns(List)} to replace its keys, or {@link Index#getColumns()} for structured
512+
* edits.
513+
*/
508514
public List<String> getPkColumns() {
509-
return pkColumns;
515+
return hasKeyIndex(Index.Kind.PRIMARY_KEY) ? index.getColumnsNames() : pkColumns;
510516
}
511517

518+
/**
519+
* Replaces a structured PRIMARY_KEY's elements using {@link Index#setColumnsNames(List)}. The
520+
* strings are not parsed and existing element attributes are not retained, even when the
521+
* rendered SQL is unchanged. Use {@link Index#setColumns(List)} to preserve structured
522+
* elements. Without a matching index, stores the legacy list.
523+
*/
512524
public void setPkColumns(List<String> pkColumns) {
513-
this.pkColumns = pkColumns;
525+
if (hasKeyIndex(Index.Kind.PRIMARY_KEY)) {
526+
index.setColumnsNames(pkColumns);
527+
this.pkColumns = null;
528+
} else {
529+
this.pkColumns = pkColumns;
530+
}
514531
}
515532

533+
/**
534+
* Returns a snapshot of the rendered key elements for a structured UNIQUE, or the legacy list
535+
* otherwise. Changing the snapshot does not change the index; use {@link #setUkColumns(List)}
536+
* to replace its keys, or {@link Index#getColumns()} for structured edits.
537+
*/
516538
public List<String> getUkColumns() {
517-
return ukColumns;
539+
return hasKeyIndex(Index.Kind.UNIQUE) ? index.getColumnsNames() : ukColumns;
518540
}
519541

542+
/**
543+
* Replaces a structured UNIQUE's elements using {@link Index#setColumnsNames(List)}. The
544+
* strings are not parsed and existing element attributes are not retained, even when the
545+
* rendered SQL is unchanged. Use {@link Index#setColumns(List)} to preserve structured
546+
* elements. Without a matching index, stores the legacy list.
547+
*/
520548
public void setUkColumns(List<String> ukColumns) {
521-
this.ukColumns = ukColumns;
549+
if (hasKeyIndex(Index.Kind.UNIQUE)) {
550+
index.setColumnsNames(ukColumns);
551+
this.ukColumns = null;
552+
} else {
553+
this.ukColumns = ukColumns;
554+
}
555+
}
556+
557+
private boolean hasKeyIndex(Index.Kind kind) {
558+
return index != null && index.getKind() == kind;
522559
}
523560

524561
public String getUkName() {
@@ -1372,24 +1409,28 @@ public AlterExpression withColumnOldName(String columnOldName) {
13721409
return this;
13731410
}
13741411

1412+
/** Adds strings using the replacement semantics of {@link #setPkColumns(List)}. */
13751413
public AlterExpression addPkColumns(String... pkColumns) {
13761414
List<String> collection = Optional.ofNullable(getPkColumns()).orElseGet(ArrayList::new);
13771415
Collections.addAll(collection, pkColumns);
13781416
return this.withPkColumns(collection);
13791417
}
13801418

1419+
/** Adds strings using the replacement semantics of {@link #setPkColumns(List)}. */
13811420
public AlterExpression addPkColumns(Collection<String> pkColumns) {
13821421
List<String> collection = Optional.ofNullable(getPkColumns()).orElseGet(ArrayList::new);
13831422
collection.addAll(pkColumns);
13841423
return this.withPkColumns(collection);
13851424
}
13861425

1426+
/** Adds strings using the replacement semantics of {@link #setUkColumns(List)}. */
13871427
public AlterExpression addUkColumns(String... ukColumns) {
13881428
List<String> collection = Optional.ofNullable(getUkColumns()).orElseGet(ArrayList::new);
13891429
Collections.addAll(collection, ukColumns);
13901430
return this.withUkColumns(collection);
13911431
}
13921432

1433+
/** Adds strings using the replacement semantics of {@link #setUkColumns(List)}. */
13931434
public AlterExpression addUkColumns(Collection<String> ukColumns) {
13941435
List<String> collection = Optional.ofNullable(getUkColumns()).orElseGet(ArrayList::new);
13951436
collection.addAll(ukColumns);

‎src/main/java/net/sf/jsqlparser/statement/create/table/Index.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,12 +134,22 @@ public void appendConstraintAttributesTo(StringBuilder sql) {
134134
}
135135
}
136136

137+
/**
138+
* Returns a mutable snapshot of the rendered key elements, including their options. An index
139+
* without columns returns an empty snapshot. Use {@link #getColumns()} for structured edits.
140+
*/
137141
public List<String> getColumnsNames() {
138-
return columns.stream()
139-
.map(ColumnParams::toString)
140-
.collect(toList());
142+
return columns == null ? new ArrayList<>()
143+
: columns.stream()
144+
.map(ColumnParams::toString)
145+
.collect(toList());
141146
}
142147

148+
/**
149+
* Replaces all key elements with plain {@link ColumnParams} wrapping the supplied strings, or
150+
* clears them for null. The strings are not parsed and existing expressions and element options
151+
* are not retained. Use {@link #setColumns(List)} to preserve structured elements.
152+
*/
143153
public void setColumnsNames(List<String> list) {
144154
if (list == null) {
145155
this.columns = Collections.emptyList();

‎src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1583,13 +1583,10 @@ public class CCJSqlParser extends AbstractJSqlParser<CCJSqlParser> {
15831583
return false;
15841584
}
15851585

1586-
/** Keeps legacy ALTER key accessors populated from the common structured definition. */
1586+
/** Attaches the structured index and populates the remaining legacy constraint metadata. */
15871587
private static void setAlterTableIndex(AlterExpression alterExp, Index index) {
15881588
alterExp.setIndex(index);
1589-
if (index.getKind() == Index.Kind.PRIMARY_KEY) {
1590-
alterExp.setPkColumns(index.getColumnsNames());
1591-
} else if (index.getKind() == Index.Kind.UNIQUE) {
1592-
alterExp.setUkColumns(index.getColumnsNames());
1589+
if (index.getKind() == Index.Kind.UNIQUE) {
15931590
alterExp.setUkName(index instanceof NamedConstraint
15941591
? ((NamedConstraint) index).getIndexName() : index.getName());
15951592
alterExp.setUk(index.getType().toUpperCase(Locale.ROOT).contains("KEY"));

‎src/test/java/net/sf/jsqlparser/statement/alter/AlterKeyAccessorsTest.java‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
/*-
2+
* #%L
3+
* JSQLParser library
4+
* %%
5+
* Copyright (C) 2004 - 2019 JSQLParser
6+
* %%
7+
* Dual licensed under GNU LGPL 2.1 or Apache License 2.0
8+
* #L%
9+
*/
10+
package net.sf.jsqlparser.statement.alter;
11+
12+
import static org.junit.jupiter.api.Assertions.*;
13+
import java.util.ArrayList;
14+
import java.util.List;
15+
import net.sf.jsqlparser.parser.CCJSqlParserUtil;
16+
import net.sf.jsqlparser.schema.Column;
17+
import net.sf.jsqlparser.statement.create.table.Index;
18+
import net.sf.jsqlparser.util.deparser.StatementDeParser;
19+
import org.junit.jupiter.api.Test;
20+
import org.junit.jupiter.params.ParameterizedTest;
21+
import org.junit.jupiter.params.provider.ValueSource;
22+
23+
class AlterKeyAccessorsTest {
24+
private List<String> columns(AlterExpression action, boolean primary) {
25+
return primary ? action.getPkColumns() : action.getUkColumns();
26+
}
27+
28+
private void setColumns(AlterExpression action, boolean primary, List<String> columns) {
29+
if (primary) {
30+
action.setPkColumns(columns);
31+
} else {
32+
action.setUkColumns(columns);
33+
}
34+
}
35+
36+
private void assertDeparsed(Alter statement, String expected) {
37+
assertEquals(expected, statement.toString());
38+
StringBuilder output = new StringBuilder();
39+
statement.accept(new StatementDeParser(output), null);
40+
assertEquals(expected, output.toString());
41+
}
42+
43+
@ParameterizedTest
44+
@ValueSource(booleans = {false, true})
45+
void readsCurrentIndexAndAppliesSettersAndFluentAdditions(boolean primary) throws Exception {
46+
String prefix = "ALTER TABLE t ADD " + (primary ? "PRIMARY KEY" : "UNIQUE");
47+
Alter statement = (Alter) CCJSqlParserUtil.parse(prefix + " (a)");
48+
AlterExpression action = statement.getAlterExpressions().get(0);
49+
Index originalIndex = action.getIndex();
50+
originalIndex.setColumnsNames(List.of("b"));
51+
List<String> snapshot = columns(action, primary);
52+
assertEquals(List.of("b"), snapshot);
53+
snapshot.set(0, "c");
54+
assertEquals(List.of("b"), columns(action, primary));
55+
assertDeparsed(statement, prefix + " (b)");
56+
setColumns(action, primary, List.of("c"));
57+
assertDeparsed(statement, prefix + " (c)");
58+
if (primary) {
59+
action.addPkColumns("d").addPkColumns(List.of("e"));
60+
} else {
61+
action.addUkColumns("d").addUkColumns(List.of("e"));
62+
}
63+
assertEquals(List.of("c", "d", "e"), columns(action, primary));
64+
assertSame(originalIndex, action.getIndex());
65+
assertDeparsed(statement, prefix + " (c, d, e)");
66+
setColumns(action, primary, null);
67+
assertTrue(columns(action, primary).isEmpty());
68+
assertTrue(originalIndex.getColumns().isEmpty());
69+
}
70+
71+
@ParameterizedTest
72+
@ValueSource(booleans = {false, true})
73+
void replacingWithBareNamesDropsElementOptionsButKeepsIndexMetadata(boolean primary)
74+
throws Exception {
75+
String prefix = "ALTER TABLE t ADD CONSTRAINT key_t "
76+
+ (primary ? "PRIMARY KEY" : "UNIQUE");
77+
Alter statement = (Alter) CCJSqlParserUtil.parse(prefix + " (a DESC, b) DEFERRABLE");
78+
AlterExpression action = statement.getAlterExpressions().get(0);
79+
Index index = action.getIndex();
80+
Index.ColumnParams first = index.getColumns().get(0);
81+
assertEquals("a DESC", first.toString());
82+
setColumns(action, primary, List.of("b", "a"));
83+
assertEquals(List.of("b", "a"), columns(action, primary));
84+
assertNull(index.getColumns().get(1).getSortOrder());
85+
assertNull(index.getColumns().get(1).getParams());
86+
assertNotSame(first, index.getColumns().get(1));
87+
assertSame(index, action.getIndex());
88+
assertEquals("key_t", index.getName());
89+
assertDeparsed(statement, prefix + " (b, a) DEFERRABLE");
90+
}
91+
92+
@ParameterizedTest
93+
@ValueSource(booleans = {false, true})
94+
void settingAnUnchangedSnapshotPreservesSqlButRebuildsPlainElements(boolean primary)
95+
throws Exception {
96+
String prefix = "ALTER TABLE t ADD " + (primary ? "PRIMARY KEY" : "UNIQUE");
97+
Alter statement = (Alter) CCJSqlParserUtil.parse(prefix + " (a DESC, b)");
98+
AlterExpression action = statement.getAlterExpressions().get(0);
99+
Index index = action.getIndex();
100+
Index.ColumnParams expression = new Index.ColumnParams(new Column("a"))
101+
.withExpressionParenthesized(false)
102+
.withSortOrder(Index.ColumnParams.SortOrder.DESC);
103+
index.getColumns().set(0, expression);
104+
setColumns(action, primary, new ArrayList<>(columns(action, primary)));
105+
Index.ColumnParams replacement = index.getColumns().get(0);
106+
assertNotSame(expression, replacement);
107+
assertEquals("a DESC", replacement.getColumnName());
108+
assertNull(replacement.getExpression());
109+
assertNull(replacement.getSortOrder());
110+
assertDeparsed(statement, prefix + " (a DESC, b)");
111+
}
112+
113+
@ParameterizedTest
114+
@ValueSource(booleans = {false, true})
115+
void structuredReorderingRetainsElementIdentityAndOptions(boolean primary) throws Exception {
116+
String prefix = "ALTER TABLE t ADD " + (primary ? "PRIMARY KEY" : "UNIQUE");
117+
Alter statement = (Alter) CCJSqlParserUtil.parse(prefix + " (a DESC, b)");
118+
AlterExpression action = statement.getAlterExpressions().get(0);
119+
Index index = action.getIndex();
120+
Index.ColumnParams first = index.getColumns().get(0);
121+
Index.ColumnParams second = index.getColumns().get(1);
122+
index.setColumns(List.of(second, first));
123+
assertSame(first, index.getColumns().get(1));
124+
assertSame(second, index.getColumns().get(0));
125+
assertEquals(Index.ColumnParams.SortOrder.DESC, first.getSortOrder());
126+
assertEquals(List.of("b", "a DESC"), columns(action, primary));
127+
assertDeparsed(statement, prefix + " (b, a DESC)");
128+
}
129+
130+
@ParameterizedTest
131+
@ValueSource(booleans = {false, true})
132+
void supportsAnIndexWithoutColumnsAndFluentPopulation(boolean primary) {
133+
Index index = new Index().withType(primary ? "PRIMARY KEY" : "UNIQUE");
134+
AlterExpression action = new AlterExpression().withOperation(AlterOperation.ADD)
135+
.withIndex(index);
136+
assertTrue(index.getColumnsNames().isEmpty());
137+
assertTrue(columns(action, primary).isEmpty());
138+
index.getColumnsNames().add("detached");
139+
assertNull(index.getColumns());
140+
if (primary) {
141+
action.addPkColumns("a");
142+
} else {
143+
action.addUkColumns("a");
144+
}
145+
assertEquals(List.of("a"), columns(action, primary));
146+
index.setColumns(null);
147+
assertTrue(columns(action, primary).isEmpty());
148+
setColumns(action, primary, List.of("b"));
149+
assertEquals(List.of("b"), index.getColumnsNames());
150+
}
151+
152+
@Test
153+
void retainsLegacyOnlyConstructionAndIgnoresUnrelatedIndexes() {
154+
AlterExpression action = new AlterExpression().withOperation(AlterOperation.ADD)
155+
.withPkColumns(new ArrayList<>(List.of("a")));
156+
action.getPkColumns().add("b");
157+
assertEquals("ADD PRIMARY KEY (a, b)", action.toString());
158+
Index unrelated = new Index().withType("INDEX").withColumnsNames(List.of("other"));
159+
action.setIndex(unrelated);
160+
action.setPkColumns(List.of("pk"));
161+
action.setUkColumns(List.of("uk"));
162+
assertEquals(List.of("pk"), action.getPkColumns());
163+
assertEquals(List.of("uk"), action.getUkColumns());
164+
assertEquals(List.of("other"), unrelated.getColumnsNames());
165+
}
166+
}

0 commit comments

Comments
 (0)