sql/backwards_extremities: Shift to table format and share code #985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an initial cut to reduce boilerplate at the storage layer. It removes the need for 2x `_table.go` files, one for each DB engine, replacing it with a single struct which has an interface which implements the raw SQL statements. The actual impl sits alongside the interface declaration which is generally regarded as best practice (though no canonical sources). Especially in this case where the impl is tiny (functions returning strings) and relies heavily on the function signatures of the table struct (for parameters), having the context in the same file is useful.
neilalexander
approved these changes
Apr 28, 2020
There was a problem hiding this comment.
Generally I quite like this pattern. What we will need to be particularly careful of is if there are any statements where we give parameters in a different order or if we provide different parameters altogether (because they are commented out, like in the public rooms API).
|
Yeah this is where we will still need different files for different DB engines. That should hopefully be the exception though, not the rule! |
kegsay
added a commit
that referenced
this pull request
May 13, 2020
This goes down a different route than #985 which tried to even reduce the boilerplate of `ExecContext` etc. The previous pattern fails badly when there are subtle differences in parameters and hence the shared boilerplate to read from `QueryContext` breaks. Rather than attacking it at that level, the main place where we want to reuse code is for the `syncserver.go` itself - the database implementation which has lots of complex logic. So instead, this commit: - Makes `invites_table.go` an interface. - Makes `SyncServerDatasource` use that interface - This means some functions are now identical for pq/sqlite, so factor them out to a temporary `shared.Database` struct which will grow until it replaces all of `SyncServerDatasource`.
kegsay
added a commit
that referenced
this pull request
May 13, 2020
* Initial syncapi storage refactor to share pq/sqlite code This goes down a different route than #985 which tried to even reduce the boilerplate of `ExecContext` etc. The previous pattern fails badly when there are subtle differences in parameters and hence the shared boilerplate to read from `QueryContext` breaks. Rather than attacking it at that level, the main place where we want to reuse code is for the `syncserver.go` itself - the database implementation which has lots of complex logic. So instead, this commit: - Makes `invites_table.go` an interface. - Makes `SyncServerDatasource` use that interface - This means some functions are now identical for pq/sqlite, so factor them out to a temporary `shared.Database` struct which will grow until it replaces all of `SyncServerDatasource`. * Missing files
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an initial cut to reduce boilerplate at the storage layer.
It removes the need for 2x
_table.gofiles, one for each DB engine,replacing it with a single struct which has an interface which
implements the raw SQL statements.
The actual impl sits alongside the interface declaration which is
generally regarded as best practice (though no canonical sources).
Especially in this case where the impl is tiny (functions returning
strings) and relies heavily on the function signatures of the
table struct (for parameters), having the context in the same file
is useful.
Assuming we're happy with this structure, I'll go ahead and convert the rest of
syncapito use this new format (the mapping is fairly mechanical and boring) as it's annoying to have a mix of both formats in a component.The main victory will be to share as much of the
Databaseimplementation as possible, which will hopefully be possible once the tables have been ported over. Crucially, nothing stops us from breaking out of this structure should the tables/statements be vastly different between the two DB engines. We should only be making files in thetablespackage if there is genuine overlap.