-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add InsertLog Query #383
Merged
Merged
Add InsertLog Query #383
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 was referenced Apr 25, 2024
Merged
This stack of pull requests is managed by Graphite. Learn more about stacking. |
insipx
force-pushed
the
insipx/insert-addresslog
branch
from
April 25, 2024 19:10
eda8919
to
f084fc7
Compare
Closed
neekolas
reviewed
Apr 25, 2024
neekolas
approved these changes
Apr 25, 2024
neekolas
pushed a commit
that referenced
this pull request
Apr 25, 2024
* Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests
neekolas
pushed a commit
that referenced
this pull request
Apr 25, 2024
* Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests
neekolas
pushed a commit
that referenced
this pull request
Apr 25, 2024
* Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests
neekolas
added a commit
that referenced
this pull request
Apr 25, 2024
…382) * Initial commit * Set up sqlc * Initial commit * Set up sqlc * Migrate first store methods to sqlc * Migrate first store methods to sqlc * Migrate first store methods to sqlc * Add GetAddressLog SQL * Add GetAddressLog SQL * Merge branch '04-24-migrate_first_store_methods_to_sqlc' of github.com:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc * Add GetAddressLog SQL * Initial commit * Set up sqlc * Initial commit * Set up sqlc * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests * Migrate first store methods to sqlc * Migrate MLS DB To SQLC (#380) Bun has been bothering me for a while, so I've been working on a migration that will get rid of our ORM altogether and just use boring SQL queries for everything. [`sqlc`](https://sqlc.dev/) is a very slick tool to generate code based on plain SQL queries using placeholders for arguments. It's not perfect...I had to do some gymnastics to make a few of the query types work. But the fact that there is no runtime other than the standard SQL driver and some generated code outweighs its limitations IMO. There's no fancy ORM library to worry about mangling your queries, and the learning curve is basically just "how well do you know SQL". - No support for serializable transactions - The SQL driver is not as well maintained as PGX - High learning curve to build complex queries, even if you know SQL well - Relations system is not very powerful and ends up doing N+1 queries a lot of the time. - Configuring the database with struct tags is errorprone, and there aren't great checks to make sure the struct tags actually match the schema. - I can't find a good way to have dynamic ORDER BY expressions. So I literally have separate queries for ASC and DESC versions. It's not the end of the world, but it's very frustrating. There's an [issue to fix it](sqlc-dev/sqlc#2061), and some hacky workarounds using CASE statements, but it's not great. - Making the filters play nice with `json_populate_recordset` is a bit of a pain. Switching to the `pgx` driver helped, since I think there was a bug in Bun's pgdriver. We use Bun in a lot of places and for a lot of things today. - It powers the `authz` database and all the migrations there - It powers the migrations for the `message` database (but not the queries) - It powers the `mls` database and all the queries in the `mlsstore`. The priority right now is to remove it from the `mlsstore`. We will still use it for migrations (`sqlc` can read Bun migrations just fine). This involves replacing the bun `pgdriver` with `pgx` (done in this PR) and replacing all the Bun ORM queries with `sqlc` queries. I have most of the queries written, but I'll split up the actual migration over several PRs. This can be done incrementally, but once the process is complete we can delete the Bun models. We aren't using any of the fancy `sqlc` cloud features and have no plans to. Ummmm. 😬. That was me. * Migrate first store methods to sqlc * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests * Add GetAddressLog SQL * Merge branch '04-24-migrate_first_store_methods_to_sqlc' of github.com:xmtp/xmtp-node-go into 04-24-migrate_first_store_methods_to_sqlc * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests * Add InsertLog Query (#383) * Add insertlog query * validation service * insert log * revocation for removed members * lint * remove unnecessary log * change test to use query from sqlc * remove comments * fix tests --------- Co-authored-by: Andrew Plaza <[email protected]>
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.
No description provided.