all: Introduce database backend interface and update plugin system an… #956
Conversation
There was a problem hiding this comment.
I really like this, great work! I think there should be tests for the different backends. You might want to have a test suite that simply checks if all of the instantiators work and (optionally) maybe run one or two functions in there. You could probably use the interface as the range struct like
for _, tc := []BackendManager{
memory...
sql...
} {
t.Run(/* ... */ )
}Apart from that and the things mentioned in the comments I think this is good to go.
cmd/server/handler_client_factory.go
Outdated
| panic("Unknown connection type.") | ||
| } | ||
| return nil | ||
| expectDependency(c.GetLogger(), ctx.Hasher, ctx.Connection) |
There was a problem hiding this comment.
We don't really need the ctx.Hash/logger dependencies here. This could be the case for plugins for example (they might use a different hasher). I think it would make more sense to put expectDependency in the concrete implementations.
There was a problem hiding this comment.
Hmmm - so move the function and all calls to config and call it as required there, e.g. for the SQL Manager? I'm not entirely sure why these were added so I just copied them back as best I could.
There was a problem hiding this comment.
The problem is that some services have dependencies to others. In some cases (in the past) these had not been properly instantiated during the set up of a particular service/manager. This caused nil point exceptions (see #928 for example). To solve that I added this little helper that makes sure that no dependency is nil.
Adding this to the SQL or Memory Managers (where required) prevents that.
config/config.go
Outdated
| } | ||
| connection = backend | ||
| } else { | ||
| c.GetLogger().Fatalf(`Unknown DSN "%s" in DATABASE_URL: %s`, scheme, c.DatabaseURL) |
There was a problem hiding this comment.
I think this should read: Unknown DSN scheme "%s" in DATABASE_URL "%s\", schemes %v supported
config/backend_manager.go
Outdated
| bmutex sync.Mutex | ||
| ) | ||
|
|
||
| type BackendManager interface { |
There was a problem hiding this comment.
I think Connection or Connector would be a better choice here :)
There was a problem hiding this comment.
Sure, so BackendConnector then? Any thoughts on what variable we should load from the database plugins? I'm using Manager right now but can update this to Connector to match this name change.
There was a problem hiding this comment.
Yup, BackendConnector or Connector both work fine. Choose one you like. The plugin should export the same name, so also either BackendConnector or Connector.
someone1
force-pushed
the
949-plugin-update
branch
from
6ee21f5 to
6abed4a
Compare
|
Updated though one thing to note is dependency checks are still used in |
someone1
force-pushed
the
949-plugin-update
branch
from
6abed4a to
6dfc8f7
Compare
…d boostrap accordingly Signed-off-by: Prateek Malhotra <someone1@gmail.com>
someone1
force-pushed
the
949-plugin-update
branch
from
6dfc8f7 to
6c0e8f3
Compare
That makes total sense and is much cleaner. Thank you for your work on this, I truly think this is a step forward to re-usability and customization of Hydra and I really like the design you chose. Thank you for this contribution! |
…d boostrap accordingly (#949)
Signed-off-by: Prateek Malhotra someone1@gmail.com