-
Notifications
You must be signed in to change notification settings - Fork 44
Feat: Add support for generated columns #101
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.
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
Feat: Add support for generated columns #101
Conversation
stephencelis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great! Just a couple comments. Let us know what you think of the suggestions and if you have any of your own.
| guard | ||
| let memberName = argument.expression.as(MemberAccessExprSyntax.self)?.declName | ||
| .baseName.text, | ||
| ["stored", "virtual"].contains(memberName) | ||
| else { | ||
| diagnostics.append( | ||
| Diagnostic( | ||
| node: argument.expression, | ||
| message: MacroExpansionErrorMessage( | ||
| "Argument 'generated' must be '.stored' or '.virtual'" | ||
| ) | ||
| ) | ||
| ) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the type-checking done at the macro level is sufficient enough, so these diagnostics shouldn't be necessary.
| guard | |
| let memberName = argument.expression.as(MemberAccessExprSyntax.self)?.declName | |
| .baseName.text, | |
| ["stored", "virtual"].contains(memberName) | |
| else { | |
| diagnostics.append( | |
| Diagnostic( | |
| node: argument.expression, | |
| message: MacroExpansionErrorMessage( | |
| "Argument 'generated' must be '.stored' or '.virtual'" | |
| ) | |
| ) | |
| ) | |
| continue | |
| } |
| public let generated = StructuredQueriesCore.TableColumn<QueryValue, String>("generated", keyPath: \QueryValue.generated) | ||
| public static var allColumns: [any StructuredQueriesCore.TableColumnExpression] { | ||
| [QueryValue.columns.name, QueryValue.columns.generated] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried it yet, but I think exposing generated table columns as TableColumn means they could still be used to generate invalid INSERT and UPDATE statements that attempt to assign values to these columns in non-draft versions of the builders. Ideally we should make generated columns distinguishable in some way to prevent that.
The simplest solution could look something like:
-public let generated = StructuredQueriesCore.TableColumn<QueryValue, String>("generated", keyPath: \QueryValue.generated)
+public var generated: some QueryExpression<String> {
+ StructuredQueriesCore.TableColumn<QueryValue, String>("generated", keyPath: \QueryValue.generated)
+ // Or even 'SQLQueryExpression("\(QueryValue.self).\(quote: "generated")")'
+}
public static var allColumns: [any StructuredQueriesCore.TableColumnExpression] {
- [QueryValue.columns.name, QueryValue.columns.generated]
+ [QueryValue.columns.name]
}Now this change alone isn't enough to work because TableDefinition relies on allColumns to build the selected columns in the query, but maybe we start generating the queryFragment directly in the macro now instead of relying on that extension.
With those changes my hope is that SELECT queries and decoding should still work fine, and INSERT and UPDATE statements won't be able to "assign" to generated column.
A more complex solution might be to have a kind of "hierarchy" of WritableTableColumnExpression to distinguish things at the type level, but that's a bigger design challenge and I think could come later if we can motivate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I tried experimenting with generating the query fragment directly in the macro but I'm not sure how it should be managed after generation.
- Does generating
TableColumnslike this make sense?
public let name = StructuredQueriesCore.TableColumn<QueryValue, String>("name", keyPath: \QueryValue.name)
public var generated: some StructuredQueriesCore.QueryExpression<String> {
StructuredQueriesCore.TableColumn<QueryValue, String>("generated", keyPath: \QueryValue.generated)
}
public static var allColumns: [any StructuredQueriesCore.TableColumnExpression] {
[QueryValue.columns.name]
}
public static var queryFragment: String {
#""name", "generated""#
}
- The newly generated
queryFragmentdoes not trigger any error or changes on generated SQL queries. Where should we connectqueryFragmentso that it gets used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remimarsal I think it can be non-static and return a QueryFragment (which can be a string literal). Something like:
-public static var queryFragment: String {
+public var queryFragment: QueryFragment {
#""name", "generated""#
}Does that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thank you, it does work. However, simply generating the column names as String is not enough to perform queries with joins as we also need the table names. Since there is a somewhat complex logic regarding tableName in ExtensionMacro, and we are generating the query fragment in the MemberMacro, we may want to generate a computed property instead like below to avoid duplication. What do you think?
public var queryFragment: QueryFragment {
QueryFragment(stringLiteral: [\(raw: selectableColumns)].map { "\\"\\(QueryValue.tableName)\\".\\"\\($0)\\"" }
.joined(separator: ", "))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup we definitely want to prefix everything with the quoted table name, as well. Wanna push code that gets things working and we can consider any cleanup after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I just pushed
|
@remimarsal Just wanted to know if you were up for those changes. If not we can try to take this PR to the finish line for you. I just didn't want to step on your toes if you were still in the middle of it :) |
|
@stephencelis Absolutely, I'll look into it today! ;) |
stephencelis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things are looking great! Pushed a small bit of cleanup around the query fragment and I think we'll be ready to merge on green.
|
Hi @stephencelis, following up on this feature. While our previous change successfully prevents runtime SQL errors when a generated column is manually added to an I think we are still missing a different use case. Given the following model: When inserting a model instance and using |
This PR introduces support for database-generated columns, as discussed in the proposal and subsequent feedback.
Motivation
Databases often use generated columns (e.g.,
GENERATED ALWAYS AS ...in SQLite) to compute values automatically. Currently,swift-structured-querieslacks a way to model these read-only columns, as they must be included inSELECTqueries but excluded fromINSERTandUPDATEoperations. Marking them as@Ephemeralis incorrect because the column does exist in the database.Implementation
generatedparameter accepts.storedor.virtual.Tablemacro now identifies properties with this attribute and includes them in theTableColumnsdefinition forSELECTqueries.Draftstruct, preventing them from being part ofINSERTorUPDATEstatements.