Skip to content

Allow Table and Column name expressions to return SqlIdentifier #2077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow Table and Column name expressions to return SqlIdentifier #2077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 18 additions & 25 deletions ...ava/org/springframework/data/relational/core/mapping/BasicRelationalPersistentEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,17 @@
* @author Bastian Wilhelm
* @author Mikhail Polivakha
* @author Kurt Niemi
* @author Sergey Korotaev
*/
class BasicRelationalPersistentEntity<T> extends BasicPersistentEntity<T, RelationalPersistentProperty>
implements RelationalPersistentEntity<T> {

private static final SpelExpressionParser PARSER = new SpelExpressionParser();

private final Lazy<SqlIdentifier> tableName;
private final @Nullable Expression tableNameExpression;
private final Lazy<SqlIdentifier> tableNameExpression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what was meant here. We should remain with Expression (or switch to ValueExpression).

It was rather meant that a Expression (respective ValueExpression) should be able to return an SqlIdentifier and not be forced to return a String (as it is done now).

We also want to evaluate the expression each time we obtain names and we don't want to introduce caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but perhaps I don't understand correctly. I see that ExpressionEvaluator returns String from Expression. Do you mean about it that no it is forced to return a String and should be able to return an SqlIdentifier? Like this
String result = expression.getValue(provider.getEvaluationContext(null), String.class);

Roughly, here we need to return SqlIdentifier, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExpressionEvaluator should return a SqlIdentifier. This is a package-protected class and we're free to change its signature.

I suggest renaming it to SqlIdentifierExpressionEvaluator and not force a String type during expression evaluation (expression.getValue(…)), but introspect the returned object.

If it is already a SqlIdentifier, then just return it as-is, otherwise, transform the value to String and apply SqlIdentifierSanitizer. Then, apply force-quoting as per isForceQuote() of the calling code.

Does that help or did that increase confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, thanks, it's clear! In first time I thought about something like this (not so detailed) but I was scary and not sure to change ExpressionEvaluator, sorry....
I'll do it a little bit later

private final Lazy<Optional<SqlIdentifier>> schemaName;
private final @Nullable Expression schemaNameExpression;
private final Lazy<SqlIdentifier> schemaNameExpression;
private final ExpressionEvaluator expressionEvaluator;
private boolean forceQuote = true;

Expand All @@ -72,19 +73,19 @@ class BasicRelationalPersistentEntity<T> extends BasicPersistentEntity<T, Relati

this.tableName = Lazy.of(() -> StringUtils.hasText(table.value()) ? createSqlIdentifier(table.value())
: createDerivedSqlIdentifier(namingStrategy.getTableName(getType())));
this.tableNameExpression = detectExpression(table.value());
this.tableNameExpression = Lazy.of(() -> detectExpression(table.value()));

this.schemaName = StringUtils.hasText(table.schema())
? Lazy.of(() -> Optional.of(createSqlIdentifier(table.schema())))
: defaultSchema;
this.schemaNameExpression = detectExpression(table.schema());
this.schemaNameExpression = Lazy.of(() -> detectExpression(table.schema()));

} else {

this.tableName = Lazy.of(() -> createDerivedSqlIdentifier(namingStrategy.getTableName(getType())));
this.tableNameExpression = null;
this.tableNameExpression = Lazy.empty();
this.schemaName = defaultSchema;
this.schemaNameExpression = null;
this.schemaNameExpression = Lazy.empty();
}
}

Expand All @@ -93,17 +94,20 @@ class BasicRelationalPersistentEntity<T> extends BasicPersistentEntity<T, Relati
* {@link LiteralExpression} (indicating that no subsequent evaluation is necessary).
*
* @param potentialExpression can be {@literal null}
* @return can be {@literal null}.
*/
@Nullable
private static Expression detectExpression(@Nullable String potentialExpression) {
private SqlIdentifier detectExpression(@Nullable String potentialExpression) {


if (!StringUtils.hasText(potentialExpression)) {
return null;
}

Expression expression = PARSER.parseExpression(potentialExpression, ParserContext.TEMPLATE_EXPRESSION);
return expression instanceof LiteralExpression ? null : expression;
if (expression instanceof LiteralExpression) {
return null;
}

return createSqlIdentifier(expressionEvaluator.evaluate(expression));
}

private SqlIdentifier createSqlIdentifier(String name) {
Expand All @@ -124,32 +128,21 @@ public void setForceQuote(boolean forceQuote) {

@Override
public SqlIdentifier getTableName() {

if (tableNameExpression == null) {
return tableName.get();
}

return createSqlIdentifier(expressionEvaluator.evaluate(tableNameExpression));
return tableNameExpression.getOptional()
.orElse(tableName.get());
}

@Override
public SqlIdentifier getQualifiedTableName() {

SqlIdentifier schema;
if (schemaNameExpression != null) {
schema = createSqlIdentifier(expressionEvaluator.evaluate(schemaNameExpression));
} else {
schema = schemaName.get().orElse(null);
}
schema = schemaNameExpression.getOptional()
.orElse(schemaName.get().orElse(null));

if (schema == null) {
return getTableName();
}

if (schemaNameExpression != null) {
schema = createSqlIdentifier(expressionEvaluator.evaluate(schemaNameExpression));
}

return SqlIdentifier.from(schema, getTableName());
}

Expand Down
38 changes: 17 additions & 21 deletions ...a/org/springframework/data/relational/core/mapping/BasicRelationalPersistentProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
* @author Florian Lüdiger
* @author Bastian Wilhelm
* @author Kurt Niemi
* @author Sergey Korotaev
*/
public class BasicRelationalPersistentProperty extends AnnotationBasedPersistentProperty<RelationalPersistentProperty>
implements RelationalPersistentProperty {
Expand All @@ -51,11 +52,11 @@ public class BasicRelationalPersistentProperty extends AnnotationBasedPersistent

private final Lazy<SqlIdentifier> columnName;
private final boolean hasExplicitColumnName;
private final @Nullable Expression columnNameExpression;
private final Lazy<SqlIdentifier> columnNameExpression;
private final SqlIdentifier sequence;
private final Lazy<Optional<SqlIdentifier>> collectionIdColumnName;
private final Lazy<SqlIdentifier> collectionKeyColumnName;
private final @Nullable Expression collectionKeyColumnNameExpression;
private final Lazy<SqlIdentifier> collectionKeyColumnNameExpression;
private final boolean isEmbedded;
private final String embeddedPrefix;

Expand Down Expand Up @@ -101,10 +102,10 @@ public BasicRelationalPersistentProperty(Property property, PersistentEntity<?,
() -> StringUtils.hasText(mappedCollection.keyColumn()) ? createSqlIdentifier(mappedCollection.keyColumn())
: createDerivedSqlIdentifier(namingStrategy.getKeyColumn(this)));

this.collectionKeyColumnNameExpression = detectExpression(mappedCollection.keyColumn());
this.collectionKeyColumnNameExpression = Lazy.of(() -> detectExpression(mappedCollection.keyColumn()));
} else {

this.collectionKeyColumnNameExpression = null;
this.collectionKeyColumnNameExpression = Lazy.empty();
}

if (isAnnotationPresent(Column.class)) {
Expand All @@ -114,7 +115,7 @@ public BasicRelationalPersistentProperty(Property property, PersistentEntity<?,

this.columnName = Lazy.of(() -> StringUtils.hasText(column.value()) ? createSqlIdentifier(column.value())
: createDerivedSqlIdentifier(namingStrategy.getColumnName(this)));
this.columnNameExpression = detectExpression(column.value());
this.columnNameExpression = Lazy.of(() -> detectExpression(column.value()));

if (collectionIdColumnName == null && StringUtils.hasText(column.value())) {
collectionIdColumnName = Lazy.of(() -> Optional.of(createSqlIdentifier(column.value())));
Expand All @@ -123,7 +124,7 @@ public BasicRelationalPersistentProperty(Property property, PersistentEntity<?,
} else {
this.hasExplicitColumnName = false;
this.columnName = Lazy.of(() -> createDerivedSqlIdentifier(namingStrategy.getColumnName(this)));
this.columnNameExpression = null;
this.columnNameExpression = Lazy.empty();
}

this.sequence = determineSequenceName();
Expand All @@ -145,17 +146,19 @@ void setExpressionEvaluator(ExpressionEvaluator expressionEvaluator) {
* {@link LiteralExpression} (indicating that no subsequent evaluation is necessary).
*
* @param potentialExpression can be {@literal null}
* @return can be {@literal null}.
*/
@Nullable
private static Expression detectExpression(@Nullable String potentialExpression) {
private SqlIdentifier detectExpression(@Nullable String potentialExpression) {

if (!StringUtils.hasText(potentialExpression)) {
return null;
}

Expression expression = PARSER.parseExpression(potentialExpression, ParserContext.TEMPLATE_EXPRESSION);
return expression instanceof LiteralExpression ? null : expression;
if (expression instanceof LiteralExpression) {
return null;
}

return createSqlIdentifier(expressionEvaluator.evaluate(expression));
}

private SqlIdentifier createSqlIdentifier(String name) {
Expand Down Expand Up @@ -186,12 +189,8 @@ public boolean isEntity() {

@Override
public SqlIdentifier getColumnName() {

if (columnNameExpression == null) {
return columnName.get();
}

return createSqlIdentifier(expressionEvaluator.evaluate(columnNameExpression));
return columnNameExpression.getOptional()
.orElse(columnName.get());
}

@Override
Expand All @@ -218,11 +217,8 @@ public SqlIdentifier getKeyColumn() {
return null;
}

if (collectionKeyColumnNameExpression == null) {
return collectionKeyColumnName.get();
}

return createSqlIdentifier(expressionEvaluator.evaluate(collectionKeyColumnNameExpression));
return collectionKeyColumnNameExpression.getOptional()
.orElse(collectionKeyColumnName.get());
}

@Override
Expand Down