-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Migrate to gobuffalo/pop #1730
Description
Is your feature request related to a problem? Please describe.
We're currently using sqlx with a lot of custom queries. This is problematic because the code is very verbose. For example, we have lots of helper structs such as this one whose only purpose is to provide arguments for NamedExec which in turn alows us to do
instead of manually going over all the fields and putting them in a map[string]interface{} for example. For more info on sqlx check here: https://github.com/jmoiron/sqlx#usage
Unfortunately, sqlx does not work with INSERT or UPDATE statements which is why we have things like this ( https://github.com/ory/hydra/blob/master/consent/sql_helper.go#L53 ) which are then being used with some strings.Join to generate the SQL insert and update statements (e.g. https://github.com/ory/hydra/blob/master/consent/manager_sql.go#L368-L369 )
On top of that, our in-memory manager is basically a duplication of the SQL adapter with some locking. We could however use SQLite with an in-memory database instead (as we do in Kratos) which would mean that the consistency checks and everything else work fine and that you can actually run Hydra without any external dependencies for a small test environment based on SQLite.
I hope/know that by switching to gobuffalo/pop, we can reduce the code a lot in verbosity, lines of code, and improve readability and testability.
For that, I would like ORY Hydra to implement the persitence/persister model of ORY Kratos. This also means that we have to move the migrations from the model in Hydra (here, every module is responsible for its own migrations) to one package and also to gobuffalo/fizz.
If we do that, we need to make sure that:
- Old migrations do not break. This means that we have to translate the old migration tables (e.g. https://github.com/ory/hydra/blob/master/consent/manager_sql.go#L55) into something that fizz understands.
- If applied with new migrations, the resulting schema is 1:1 the same for all databases
Additional context
This is a lot of work. I also think that we can refactor the tests a bit to make them more readable. But first, we should use pop. Use the same tests (except for maybe how they are being set up, but the contents should be the same), and in a next phase refactor the tests too.