faster sql query, fixes #15

[tacman]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 42.28%. Comparing base (3b28751) to head (16df034).
Report is 3 commits behind head on main.

Files	Patch %	Lines
src/HealthCheck/DoctrineConnectionHealthCheck.php	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #17      +/-   ##
============================================
+ Coverage     41.17%   42.28%   +1.10%     
+ Complexity       52       51       -1     
============================================
  Files            12       12              
  Lines           153      149       -4     
============================================
  Hits             63       63              
+ Misses           90       86       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thijskh]

Thanks, indeed it fixes the issue but we want to ensure just a little bit more; this query will return success even if the database in question does not exist or is compleltely empty, while we want to do the basic sanity check that things "work". So I think we'll only merge this if indeed the query in question is in fact configurable.

[tacman]

Is there a way to disable this test then from the application? It's broken for Postgres databases, so I can't use it at all. Or just get rid of the backticks? Or...?

…

On Thu, Apr 4, 2024 at 12:53 AM Thijs Kinkhorst ***@***.***> wrote: Thanks, indeed it fixes the issue but we want to ensure just a little bit more; this query will return success even if the database in question does not exist or is compleltely empty, while we want to do the basic sanity check that things "work". So I think we'll only merge this if indeed the query in question is in fact configurable. — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEXIQKQ6M63TL2NK4YBX3TY3TMDJAVCNFSM6AAAAABFVSDIN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZWGE4DQMJSGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[tacman]

You could also split the services, select 1 is fast and ensure that there's a database server, and a custom one, which checks the table, etc. But is there even a way to disable services from the application now?

…

On Thu, Apr 4, 2024 at 7:06 AM Tac Tacelosky ***@***.***> wrote: Is there a way to disable this test then from the application? It's broken for Postgres databases, so I can't use it at all. Or just get rid of the backticks? Or...? On Thu, Apr 4, 2024 at 12:53 AM Thijs Kinkhorst ***@***.***> wrote: > Thanks, indeed it fixes the issue but we want to ensure just a little bit > more; this query will return success even if the database in question does > not exist or is compleltely empty, while we want to do the basic sanity > check that things "work". So I think we'll only merge this if indeed the > query in question is in fact configurable. > > — > Reply to this email directly, view it on GitHub > <#17 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAEXIQKQ6M63TL2NK4YBX3TY3TMDJAVCNFSM6AAAAABFVSDIN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZWGE4DQMJSGA> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[MKodde]

@tacman if the current query is not working correctly on other database platforms then by all means, feel free to suggest a better query that runs on more platforms. If the backticks are the only thing messing things up, feel free to remove them here.

I do like your suggestion, to inject the query from the config. That way a user can configure an appropriate query for the application the monitor bundle is running on.

Some thoughts:

1. I like the current setup as it looks for the first schema in the database and runs a query on that. That's zero config for me. And I have many projects that run the monitor bundle. I'm not looking forward to configuring a test qurey for all of them. So maybe use the injected query as an override to the default behavior?
2. You could look into adding a service override for Postgres users. That would require adding some magic in the compile pass I assume. But it could be a solution?

But is there even a way to disable services from the application now?

If you mean: is it possible to remove a service from being added to the health checker service? My answer is yes and no. Any service that is tagged with the openconext.monitor.health_check is included in the health checker service. That task is performed here:

Monitor-bundle/src/DependencyInjection/Compiler/HealthCheckPass.php

Lines 44 to 46 in 9fad7f1

	foreach ($taggedServices as $id => $tags) {
	$definition->addMethodCall('addHealthCheck', array(new Reference($id)));
	}

.

No: that config is now hard coded in the services.yaml file. So it would need some tweaking to make it configurable.
Yes: you could add some additional logic to the compiler pass. It could check your database platform, and select the appropriate DB health checker that way.

Hope this helps you somewhat. If you need some more help, feel free to reach out to me on the OpenConext Slack channel or on the Symfony Devs Slack. You should be able to find me there

[tacman]

How about instead of hand-coding the query:

$query = 'SELECT * FROM `%s` LIMIT 1';
$this->entityManager->getConnection()->executeQuery(sprintf($query, $table->getName()));

We use a querybuilder, which automatically escapes the table name.

        $this->entityManager->createQueryBuilder()
            ->from($table->getName())
            ->setMaxResults(1)
            ->getQuery()
            ->getResult(); // or a count query, getOneOrNull(), or whatever.

There are faster ways to do this, but I think this will at least allow postgres. Maybe use the queryCache, since we're not actually concerned with the data? Or even a limit of 0, which would make the query but not hydrate an entity, though maybe you want to test that.

[tacman]

I think the backticks are only relevant when the table name is a reserved work. Unfortunately, Symfony's 'make:user' command create a user table if the database connection is postgres, which I always rename to 'users' so I can easily switch between postgres and sqlite.

[tacman]

I'd love to use the bundle, but because I use postgres it consistently fails.

I know the select 1 didn't accomplish what you want, but I'm not sure how to fix this.

[MKodde]

How about instead of hand-coding the query:

$query = 'SELECT * FROM `%s` LIMIT 1';
$this->entityManager->getConnection()->executeQuery(sprintf($query, $table->getName()));

We use a querybuilder, which automatically escapes the table name.

        $this->entityManager->createQueryBuilder()
            ->from($table->getName())
            ->setMaxResults(1)
            ->getQuery()
            ->getResult(); // or a count query, getOneOrNull(), or whatever.

There are faster ways to do this, but I think this will at least allow postgres. Maybe use the queryCache, since we're not actually concerned with the data? Or even a limit of 0, which would make the query but not hydrate an entity, though maybe you want to test that.

We think that should work for us as well. The performance penalty should be minimal. So feel free to go ahead with this suggestion.

[thijskh]

Hi @tacman. Can you move forward with the suggested change? An alternative that we would find acceptable to make the full SQL query a configurable string in the bundle config. That also seems like a flexible and generic solution.

[tacman]

ok, to confirm, that's

    $this->entityManager->createQueryBuilder()
        ->from($table->getName())
        ->setMaxResults(1)
        ->getQuery()
        ->getResult(); // or a count query, getOneOrNull(), or whatever.

I think getOneOrNull is good, because it has the benefit of hydrating the entity, so it will throw an error if a migration (on that table) is missing.