Skip to content
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

Use Diesels MultiConnections Derive #3796

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

Conversation

BlackDex
Copy link
Collaborator

@BlackDex BlackDex commented Aug 24, 2023

With this PR we remove almost all custom macro's to create the multiple database type code. This is now handled by Diesel it self.

This removed the need of the following functions/macro's:

  • db_object!
  • ::to_db
  • .from_db()

It is also possible to just use one schema instead of multiple per type.


NOTE: There are just few items which cause some issues with models and schemas having the exact same name, and that causes some ambiguity. I have solved that currently by adding schema:: in-front of those specific items. But it would be nicer if we didn't need that though.

Not sure what the best way is to address this. Probably renaming the tables is the best option i think.

I still want to do some testing with the different database types with an empty database, an existing database which needs migrations etc.. etc..

Also, we probably want to wait until Diesel releases v2.1.1, since there are currently some bugs and missing features which got fixed, but not yet merged.

And if someone else wants to help test, please do!, The more the better.

With this PR we remove allmost all custom macro's to create the multiple
database type code. This is now handled by Diesel it self.

This removed the need of the following functions/macro's:
- `db_object!`
- `::to_db`
- `.from_db()`

All `conn` variables do not need a `mut` anymore (says nightly rust)

It is also possible to just use one schema instead of multiple per type.
@weiznich
Copy link

Is there anything from the diesel side of things I can to do move this forward?

@BlackDex
Copy link
Collaborator Author

Is there anything from the diesel side of things I can to do move this forward?

Not really, but thanks for asking.

We don't want to break a pending PR for SSO for example, since this is a huge change.

I do think that is the only one btw.

@BlackDex
Copy link
Collaborator Author

Also, during this i encountered a few naming-scheme errors, which i probably need to fix too.
Best to do that before merging something like this i think.
Same for some other optimizations like removing the mut for all the DbConn parameters.
Something on my ToDo list :)

@alvinpeters
Copy link

This is great! I'm in the process of adding non-diesel database backends (key-value stores like RocksDB for single-node setup and FoundationDB for clusters). This will make my life much easy. Is there anything I can help (on the coding side) to get this merged?

@BlackDex
Copy link
Collaborator Author

This is great! I'm in the process of adding non-diesel database backends (key-value stores like RocksDB for single-node setup and FoundationDB for clusters). This will make my life much easy. Is there anything I can help (on the coding side) to get this merged?

Not really a.t.m. we were waiting for some other PR's to be merged to prevent big changes for those PR's. Mainly the SSO one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants