Fix ping logic for SQL Registry

[ludydoo]

Fixes the Ping logic on the SQL registry

Related issue(s)

#2734

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[ludydoo]

@Benehiko @zepatrik @aeneasr

[codecov]

Codecov Report

Merging #3095 (3b6ad06) into master (152bddd) will decrease coverage by 0.16%.
The diff coverage is 63.15%.

@@            Coverage Diff             @@
##           master    #3095      +/-   ##
==========================================
- Coverage   79.70%   79.53%   -0.17%     
==========================================
  Files         112      112              
  Lines        7932     7971      +39     
==========================================
+ Hits         6322     6340      +18     
- Misses       1213     1225      +12     
- Partials      397      406       +9     

Impacted Files	Coverage Δ
driver/registry.go	77.14% <42.85%> (-2.86%)	⬇️
driver/registry_sql.go	78.26% <66.66%> (-2.07%)	⬇️
persistence/sql/persister.go	79.66% <100.00%> (+1.08%)	⬆️
persistence/sql/persister_jwk.go	64.58% <0.00%> (-2.09%)	⬇️
driver/registry_base.go	87.18% <0.00%> (-1.78%)	⬇️
persistence/sql/persister_oauth2.go	81.01% <0.00%> (-0.85%)	⬇️
oauth2/handler.go	68.27% <0.00%> (ø)
consent/strategy_default.go	70.52% <0.00%> (+0.20%)	⬆️
jwk/cast.go	60.00% <0.00%> (+60.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 152bddd...3b6ad06. Read the comment docs.

[Benehiko]

We will also need some tests for Janitor to make sure it does in fact throw an error on an incorrect DSN.

https://github.com/ory/hydra/blob/858f2cf362996f46a8f86841e359336e877436c5/cmd/cli/handler_janitor_test.go

[aeneasr]

Thank you!

[aeneasr]

Can you please take a look at the failing tests? https://github.com/ory/hydra/runs/6194460310?check_suite_focus=true#step:12:27

[aeneasr]

Is this really needed?

[Benehiko]

Hey @ludydoo

Thank you for taking the time to work on this - I think we can almost merge this :)

The PR just needs to address the following issues first:

• Mock the driver.Init() function to test database failures on an incorrect DSN
• Fix failing tests

[ludydoo]

@Benehiko @aeneasr hey there.

I have been analyzing this piece a little bit. Let me share my thoughts here:

One of the initial challenges I faced with this bit is around the dbal package. When I inspect the code inside this package, I would intuitively expect to see stuff about database connections, low-level utilities, etc. But it turns out that this package mainly parses dsns (string parsing). There is a connect.go file that is empty.

It got me a bit confused at first. Especially because the registry.NewRegistryFromDSN retrieves a driver based on the dsn using dbal.GetDriverFor. But it also turns out that what it returns is actually a registry.RegistrySQL.

So there is the indirection from registry->dbal->registry that I would like to suggest to simplify.

The second suggestion I would have would be around either the naming or the purpose of NewRegistryFromDSN. The problem I see with this is that we actually do not pass any dsn to this method, but rather a context, a configuration and a logger. The signature of this method seems not to represent what it actually does.

The third, and perhaps the most impactful thing I have noticed, is around the registry.Init method. I explain a bit below. That is of course completely outside the scope of this PR, but I include the comments here to explain the rationale.

I think that mocking the driver in its current state, unless I am missing something, would perhaps be a step backwards. Allow me to suggest the following.

Review the naming of the dbal package

The dbal package, currently, mostly parses strings. Perhaps that dsn would be a more descriptive name for this package.

Introduce a dsn primitive

In the dbal package, I think it would be useful to have an object that represents a dsn, such as

type DSN interface{
  fmt.Stringer
  Kind() Kind
}

type dsn struct{kind Kind, val String}
func (d dsn) Kind() Kind {return d.kind}
func (d dsn) String() string{return d.val}

type Kind uint8
const (
  MySQL Kind = iota + 1
  InMemory
  Cockroach
  PostgreSQL
)

func NewDSN(value string) (DSN, error){...}

So that when we have the method NewRegistryFromDSN, we could pass it an actual DSN

NewRegistryFromDSN(ctx, dsn, ...) Registry

Introduce a connection object (at some point)

In the registry.Init, we initialize a new pop.Connection in there. A nice thing about this is that we actually do not need the registry at all to do that, because there is no dependency (apart from the logger/tracer). Here is the bit of code

// new db connection
		pool, idlePool, connMaxLifetime, connMaxIdleTime, cleanedDSN := sqlcon.ParseConnectionOptions(m.l, m.C.DSN())
		c, err := pop.NewConnection(&pop.ConnectionDetails{
			URL:                       sqlcon.FinalizeDSN(m.l, cleanedDSN),
			IdlePool:                  idlePool,
			ConnMaxLifetime:           connMaxLifetime,
			ConnMaxIdleTime:           connMaxIdleTime,
			Pool:                      pool,
			UseInstrumentedDriver:     m.Tracer(ctx).IsLoaded(),
			InstrumentedDriverOptions: opts,
		})

Perhaps we could further enhance

type Connection interface{}
type connection struct{pop.Connection}
func NewConnection(dsn DSN) (Connection, error){
  ...
}

NewRegistry(ctx, connection Connection, ...) (Registry, error) {
  if err := connection.Ping(); err != nil { return nil, err}
}

--- 

func TestNewRegistry(t *testing.T){
  mockConnection := &mockConnection{}
  if _, err := NewRegistry(ctx, mockConnection); !assert.Error(err){
    return
  }
}

Review the registry.Init method (at some point)

There is a lot of stuff going on in the registry.Init. I think the main problem in my view is that

1. The registry has a persister as a dependency
2. The persister has the registry as a dependency
3. The method builds a lot of things, and puts together a lot of other things. In that regard, it is very much a factory.

I think it would make our lives easier when it comes to testability and mocking if we were to construct the different components (hardwareKeyManager, managerStragety, clientHasher, etc.) upstream, and inject them inside of the components that require them. I always found it useful to build these components at the very very root of the program (just above the cmd), and propagate this dependency tree from there, rather than constructing them at different levels.

This is of course a big big change, but I believe it would be kind of helpful to write tests.

[Benehiko]

Hey @ludydoo

Thank you for looking into this so extensively :)

The issues you have raised are quite valid and I do think it does make sense to have a refactor that makes the code base a bit easier to read. However, this would be a big undertaking since it would also impact Keto and Kratos since they also use the dbal package. I think it would make sense to move the refactor discussions to its own issue.

I think for this PR we should focus on getting the current failing tests passing so we can merge it ;)

[ludydoo]

@Benehiko sounds good.

The issue with the provided example is that it creates a mock using NewRegistryForDSN, which itself calls the Init(). So the database connection is created and pinged before it is able to be mocked.

func NewRegistryForDSN(...){
        ...
	if err := registry.Init(ctx); err != nil {
		return nil, err
	}
}

so that

func TestSomething(t *testing.T){
   r := NewRegistryForDSN(dummyDSN) <- calls init
   mock := &MockRegistry{r: r.(*RegistrySQL)} <- init has already been called
}
type MockRegistry{...}
func (m *MockRegistry) Init(...){...}

Because of this, we cannot mock the Init() function by creating a mock registry using NewRegistryForDSN, because the Init is already called in NewRegistryForDSN.

The only way I see would be to either

1. Not call Init in NewRegistryForDSN
2. Build the RegistrySQL without NewRegistryForDSN
3. Provide an option to NewRegistryForDSN to not call the Init() method. Probably a bit dirty.
4. Provide an internal newRegistryForDSN that does not call Init(), but have NewRegistryForDSN call newRegistryForDSN + Init()

What would be your recommended approach ?

[Benehiko]

Hey @ludydoo

Ah i see, yes you are correct. I think the easiest would be to construct a new function called NewRegistryWIthoutInit like we do in Kratos. Basically your option 4.

https://github.com/ory/kratos/blob/351760ece01d421687179b8e3f6f48a720247a1d/driver/factory.go#L22-L37

We could then easily mock the Init function after creating this. I think for now this is the easiest way to go about it.

[Benehiko]

Very nice! :)

[zepatrik]

Very nice 👍