NEW NullDatabase #10016

chillu · 2021-07-07T21:48:55Z

We need to throw exceptions in a specific context: When you run composer install or composer update, we want to generate GraphQL code automatically through a composer plugin. These commands are also run on environments without a functional Silverstripe installation in CI, as part of automation testing as well as to create deployment packages sent to a separate runtime environment.

Option A: NullDatabase

This pull request. Given the massive API surface of Database and its dependencies, that's a lot of code to shim in order to throw a bunch of exceptions. But its marked as @internal, so we can pivot the approach at any time. One benefit here is that we avoid undesireable side effects by making other methods a no-op.

Option B: Dispatching events in Database

We already use https://github.com/silverstripe/silverstripe-event-dispatcher as a dependency on silverstripe/graphql. By dispatching events from Database, we could avoid introducing new (internal) implementations. Thankfully the methods where events are most useful are implemented on abstract class Database, rather than concrete implementations like MySQLDatabase, so we can achieve good coverage without worrying about modifications on other database adapters like PostgreSQL. This would also open up new ways of benchmarking queries (e.g. send the results to a monitoring service).

Option C: Proxy DB

Damian has written https://github.com/tractorcow/silverstripe-proxy-db. It is a quite powerful way to create fakes, but would add a pretty substantial dependency to core: https://github.com/phpspec/prophecy. The library is pretty common and active, but also has no explicit statements about semver or ongoing commitments to PHP version support. So on the long term, this could lead to similar challenges that we currently face by hard-wiring SapphireTest to specific PHPUnit features, which are now blocking PHP 8 compatibility in core. Note that we currently depend on this library in silverstripe/auditor which is a supported module, but not in core. And this dependency could probably disappear if we implemented Option B anyway.

unclecheese · 2021-07-07T23:37:11Z

So from my perspective, Option A solves the problem of "using the SS kernel when a database isn't available", while the other two options simply create a platform for solving the problem in a more elegant way.

I think the ideal approach here is both. We want to provide a solution for this not-entirely-uncommon problem but also provide a way of hooking into database activity in a general sense to do other things (e.g. track content changes!)

If we went with Option B:

Emit events in the Database abstract
Add an event handler that simply throws
Subscribe to every single database event
... or, just always emit a generic "Database activity" event"
This could be a good case of the event.action syntax that the event dispatcher uses:
- database.select, database.update, etc.
- Then just subscribe to database and throw.

Concerns:

The event dispatcher module isn't stable and no one has ever really vetted the API. We've talked about add a proper event cycle to framework for ages, so this is an important thing to get right. Major API changes would have to wait until v5.
Retroactive updates of Database abstract to include event emitters as the API expands. Not likely to happen very often, and I think seeing the presence of emitters elsewhere in the class would be enough context for a peer review, anyway.

If we went with Option C:

Write an extension that throws on all database events
Apply the proxy factory and the extension procedurally in the graphql / recipe plugin

Concerns:

New dependency, big footprint.
Maintenance overhead of remembering to update the proxy any time the Database API changes (low likelihood)

chillu · 2021-07-08T00:24:09Z

Discussed offline with Aaron, here's where we got to:

DatabaseLessKernel instead of modifying CoreKernel->boot() param signature
DatabaseLessKernel in core, marked @internal
DatabaseLessKernel hooks into new event system on the default database (not NullDatabase)
Event handler throws on database.query event
Bonus points: sake (via cli-script.php) accepts --database false trigger to instanciate DatabaseLessKernel, which enables generating manifests as part of release packages.
Out of scope: New sake dev/generatemanifests --database false command, and defining manifest storage location separate from other temp dir location (or force deployment logic to move manifests into an existing temp dir)

chillu · 2021-07-12T10:25:38Z

OK I tried Option B, see https://github.com/open-sausages/silverstripe-framework/tree/pulls/4/databaselesskernel, even done it with crazy anonymous classes for the single-method event handlers ;) The problem was that several methods are called before query(), e.g. DB::get_conn()->escapeString() triggered by Convert::raw2sql(). So we need to set some kind of database implementation anyway via DB::set_conn(), and make this at least a no-op (through NullDatabase). At that point, we might as well throw exceptions right in that class (Option A), rather than rely on the events model from Option B. I've updated the pull request to that effect.

Depends on silverstripe/silverstripe-framework#10016

unclecheese · 2021-07-12T21:51:59Z

Yup, that makes sense. Do you want to just remove those event emissions from the database class, though? Not sure why they're there anymore.

chillu · 2021-07-13T00:20:14Z

Of course, that's done now (removed the events from Database.php)

michalkleiner · 2021-07-13T00:37:00Z

Can remove the composer dependency and the use statements as well then?

chillu · 2021-07-13T01:39:07Z

Can remove the composer dependency and the use statements as well then?

Yeah just realised that after the failing build ... doing too many things at once :)
@michalkleiner While I have your attention: Could you have a look at my rationale in silverstripe/silverstripe-graphql#388 for GraphQL code gen as part of composer install?

michalkleiner · 2021-07-13T03:01:59Z

I like this NullDatabase approach here, thinking how we could live without it before. Will definitely speed up some functional tests for us as well where no database is needed.

Will check the other issue.

chillu · 2021-07-13T20:51:43Z

@michalkleiner Haven't thought of the test run use case before. Once this is merged you could look at sake with a --database false option (via cli-script.php)? Another benefit of this is generating config and class manifests as part of deployment, before pushing out the deployment package to multiple servers. Which should make deployments a bit faster, and put new instances into service quicker on auto scaling events. That's not an explicit objective right now, and we'd need to work through the implications for mixed temp folder usage, but it opens up that possibility.

emteknetnz

Seems fine, main thing is to see if you can interfaces here rather than extending a base class.

emteknetnz · 2021-11-11T01:24:35Z

src/Core/DatabaselessKernel.php

+ *
+ * @internal
+ */
+class DatabaselessKernel extends CoreKernel


Is there any hope of getting this to implement Kernel rather than extend CoreKernel (which itself implements Kernel)? Or would there be too much code you'd need to end up duplicating from CoreKernel?

Just seems really weird extending something then removing something (the database) from it

Agree, but we can't add an interface in a minor release. That's why we're subclassing.

I think it's doable if you did this:

interface Kernel |- abstract class BaseKernel implements Kernel (New class - move bunch of stuff from CoreKernel to here) |- class CoreKernel extends BaseKernel (Include database connection methods) |- class DatabaselessKernel extends BaseKernel

That would mean that CoreKernel keeps the same signature and functionality

So we're going from adding one new @internal class to solve an edge case to three new classes and one new interface?

I'm just concerned that the motivation behind this was to keep it as simple and low-impact as possible, and now we're just expanding the maintenance footprint. I agree it's a better architecture, but I just don't want to lose sight over why were all here.

We're adding one new abstract class + one new class, as opposed to one new class. We'll have the same number of methods, so it's literally just one extra file to make things tidier which IMO will reduce the maintenance footprint.. Also as Michal mentioned above the DatabaselessKernel could be used for speedier functional testing as well.

Ah, right, I didn't see that the interface already existed. OK, yeah this makes more sense now. Thanks.

move bunch of stuff from CoreKernel to here

I think this is the only bit I'm not sold on. What "stuff" we move is kind of important. In this case, moving all the database-related methods out of the base class and into the CoreKernel would achieve what we need, but it's a bit arbitrary to draw the line at database concerns. If the point of this is abstraction and composability, then why not allow for configless or manifestless kernels as well? If the base class is going to be database agnostic, I see no reason why it can't be agnostic to all the other layers as well.

Here's one option:

BaseKernel becomes a collection of services that can be used to boot the kitchen sink of application layers, e.g. setThemeResourceLoader, setInjectorLoader, etc..

boot() method remains abstract

All bootXXX protected methods, e.g. bootConfigs() defined in the concretion

To maintain a horizontal architecture, we move bootXXX() methods to traits

Implementations can pick and choose which boots they want. That way CoreKernel and Databaseless kernel can both use the ManifestBoot and ConfigBoot trait, for example.

bootPHP would be the exception. A non-PHP boot isn't plausible. :-)

If we were to keep bootXXX() methods in CoreKernel, then DatabaselessKernel would have to extend CoreKernel to have access to them, and then we're right back where we started.

Traits is probably overkill, though I know you love your composability :-) Maybe a middle ground is just leave all the stuff you'd move to traits in BaseKernel as method and just use the abstract boot() method to choose what methods get booted

I'm not overly fussed how you split it though

It's too complicated. There's too much coupling in those boot methods, and it's probably not realistic to boot without a manifest.

A few leaky abstractions here and there, too, e.g. isFlushed() not being part of the interface, yet expected in the public consumption of the API. Annoying.

Have refactored.

src/ORM/Connect/NullDatabase.php

emteknetnz

Looks good, some minor changes to make

I've double checked that all things removed from CoreKernel were copied over to BaseKernel

Running a website or normal unit test still works as it normally does

I can validate so much as the --no-database option works and throws NullDatabaseExceptions e.g. running vendor/bin/sake --no-database dev/config

I'm not sure how to test something running successfully without a database @unclecheese presumably you've validated this does what you need it to for building GraphQL 4 schemas?

src/Core/DatabaselessKernel.php

emteknetnz · 2021-12-08T03:16:47Z

src/Core/CoreKernel.php


    /**
     * Indicates whether the Kernel has been flushed on boot
-     * Unitialized before boot
+     * Uninitialised before boot
     *
     * @var bool
     */
    private $flush;


As noted on DatabaselessKernel, more $this->flush along with to BaseKernel as a protected field along with isFlushed()

I can't see really see any need to keep isFlushed abstract and open to different implementations

Replied on the other comment.

emteknetnz · 2021-12-08T03:18:52Z

src/ORM/Connect/NullDatabase.php

+    /**
+     * @var string
+     */
+    protected $queryErrorMessage = 'Using NullDatabase, cannot execute query: %s';


Suggested change

protected $queryErrorMessage = 'Using NullDatabase, cannot execute query: %s';

private $queryErrorMessage = 'Using NullDatabase, cannot execute query: %s';

emteknetnz · 2021-12-08T03:18:59Z

src/ORM/Connect/NullDatabase.php

+    /**
+     * @var string
+     */
+    protected $errorMessage = 'Using NullDatabase, cannot interact with database';


Suggested change

protected $errorMessage = 'Using NullDatabase, cannot interact with database';

private $errorMessage = 'Using NullDatabase, cannot interact with database';

emteknetnz · 2021-12-08T03:19:31Z

src/ORM/Connect/NullDatabaseException.php

@@ -0,0 +1,10 @@
+<?php
+
+


Suggested change

emteknetnz · 2021-12-08T03:19:44Z

src/ORM/Connect/NullDatabase.php

+ */
+class NullDatabase extends Database
+{
+


Suggested change

emteknetnz · 2021-12-08T03:19:59Z

src/Core/DatabaselessKernel.php

+ */
+class DatabaselessKernel extends BaseKernel
+{
+    protected $queryErrorMessage = 'Booted with DatabaseLessKernel, cannot execute query: %s';


Suggested change

protected $queryErrorMessage = 'Booted with DatabaseLessKernel, cannot execute query: %s';

private $queryErrorMessage = 'Booted with DatabaseLessKernel, cannot execute query: %s';

What's this even used for? It can probably just be removed

emteknetnz · 2021-12-08T03:23:08Z

src/Core/BaseKernel.php

+        return [];
+    }
+
+


Suggested change

emteknetnz · 2021-12-08T03:23:55Z

src/Core/BaseKernel.php

+
+    abstract public function boot($flush = false);
+
+    abstract public function isFlushed();


As mentioned elsewhere, no need for this to be abstract, the implementation is the same in both CoreKernel + DatabaselessKernel, so may as well just do it here

My instinct would be to keep it as is.

Looking at the implementation of isFlushed, it seems like it's tied in to the implementation of boot. Since boot is abstract, it would seem to make some sense to say that isFlushed should also be abstract.

Otherwise, we're putting an implicit requirement on the sub classes to update the isFlushed variable in the boot method. Defining isFlushed as abstract makes that requirement explicit.

emteknetnz · 2021-12-08T03:24:10Z

src/Core/BaseKernel.php

+    }
+
+
+


Remove 2 empty lines here

This is required for GraphQL code generation in CI (without a working runtime database/webserver environment). Context: silverstripe/silverstripe-graphql#388

maxime-rainville

Rebased this bad boy + made some tweaks.

maxime-rainville · 2022-02-03T04:27:18Z

src/Core/BaseKernel.php

+
+    abstract public function boot($flush = false);
+
+    abstract public function isFlushed();


My instinct would be to keep it as is.

Looking at the implementation of isFlushed, it seems like it's tied in to the implementation of boot. Since boot is abstract, it would seem to make some sense to say that isFlushed should also be abstract.

Otherwise, we're putting an implicit requirement on the sub classes to update the isFlushed variable in the boot method. Defining isFlushed as abstract makes that requirement explicit.

maxime-rainville · 2022-02-03T04:27:31Z

src/Core/BaseKernel.php

+    }
+
+
+


maxime-rainville · 2022-02-03T04:28:58Z

src/Core/CoreKernel.php


    /**
     * Indicates whether the Kernel has been flushed on boot
-     * Unitialized before boot
+     * Uninitialised before boot
     *
     * @var bool
     */
    private $flush;


Replied on the other comment.

maxime-rainville · 2022-02-03T04:31:43Z

src/Core/DatabaselessKernel.php

+ */
+class DatabaselessKernel extends BaseKernel
+{
+    protected $queryErrorMessage = 'Booted with DatabaseLessKernel, cannot execute query: %s';


src/Core/DatabaselessKernel.php

src/ORM/Connect/NullDatabase.php

* NEW DatabaselessKernel to support operation without DB This is required for GraphQL code generation in CI (without a working runtime database/webserver environment). Context: silverstripe/silverstripe-graphql#388 * New --no-database option for sake * Refactor to abstract class * Apply feedback peer review Co-authored-by: Aaron Carlino <unclecheese@leftandmain.com> Co-authored-by: Maxime Rainville <maxime@silverstripe.com>

chillu mentioned this pull request Jul 7, 2021

Generate code as part of "composer install" silverstripe/silverstripe-graphql#388

Open

17 tasks

chillu force-pushed the pulls/nulldatabase branch from 3b0e68d to d25e0fa Compare July 12, 2021 10:21

chillu force-pushed the pulls/nulldatabase branch from d25e0fa to 625e724 Compare July 12, 2021 21:35

chillu added a commit to silverstripe/silverstripe-graphql-composer-plugin that referenced this pull request Jul 12, 2021

Use NullDatabase and DatabaselessKernel

23f9af3

Depends on silverstripe/silverstripe-framework#10016

chillu force-pushed the pulls/nulldatabase branch from 625e724 to e9f0d25 Compare July 13, 2021 00:19

chillu force-pushed the pulls/nulldatabase branch 2 times, most recently from 00876c5 to 138655d Compare July 13, 2021 01:24

unclecheese marked this pull request as ready for review November 4, 2021 21:56

unclecheese force-pushed the pulls/nulldatabase branch from 138655d to 9194e05 Compare November 4, 2021 23:02

unclecheese mentioned this pull request Nov 5, 2021

NEW: Support --no-database build option silverstripe/silverstripe-graphql#414

Merged

emteknetnz requested changes Nov 11, 2021

View reviewed changes

unclecheese force-pushed the pulls/nulldatabase branch 2 times, most recently from 120aa94 to 2d5f990 Compare November 16, 2021 22:30

emteknetnz mentioned this pull request Nov 17, 2021

API Selectively register test folder in manifest #10150

Merged

1 task

unclecheese force-pushed the pulls/nulldatabase branch from 2d5f990 to db878a6 Compare November 28, 2021 22:35

emteknetnz requested changes Dec 8, 2021

View reviewed changes

maxime-rainville assigned unclecheese Jan 24, 2022

chillu and others added 3 commits February 3, 2022 15:43

NEW DatabaselessKernel to support operation without DB

27bdaa5

This is required for GraphQL code generation in CI (without a working runtime database/webserver environment). Context: silverstripe/silverstripe-graphql#388

New --no-database option for sake

49970a4

Refactor to abstract class

32b19f8

maxime-rainville force-pushed the pulls/nulldatabase branch from db878a6 to 32b19f8 Compare February 3, 2022 03:37

Apply feedback peer review

7062842

maxime-rainville reviewed Feb 3, 2022

View reviewed changes

maxime-rainville requested a review from emteknetnz February 3, 2022 04:54

maxime-rainville assigned emteknetnz and unassigned unclecheese Feb 3, 2022

emteknetnz approved these changes Feb 3, 2022

View reviewed changes

emteknetnz merged commit d8499a2 into silverstripe:4 Feb 3, 2022

	protected $queryErrorMessage = 'Using NullDatabase, cannot execute query: %s';
	private $queryErrorMessage = 'Using NullDatabase, cannot execute query: %s';

	protected $errorMessage = 'Using NullDatabase, cannot interact with database';
	private $errorMessage = 'Using NullDatabase, cannot interact with database';

	protected $queryErrorMessage = 'Booted with DatabaseLessKernel, cannot execute query: %s';
	private $queryErrorMessage = 'Booted with DatabaseLessKernel, cannot execute query: %s';


		abstract public function boot($flush = false);

		abstract public function isFlushed();

NEW NullDatabase #10016

NEW NullDatabase #10016

Conversation

chillu commented Jul 7, 2021

Option A: NullDatabase

Option B: Dispatching events in Database

Option C: Proxy DB

unclecheese commented Jul 7, 2021

If we went with Option B:

Concerns:

If we went with Option C:

Concerns:

chillu commented Jul 8, 2021 • edited Loading

chillu commented Jul 12, 2021

unclecheese commented Jul 12, 2021

chillu commented Jul 13, 2021

michalkleiner commented Jul 13, 2021

chillu commented Jul 13, 2021

michalkleiner commented Jul 13, 2021

chillu commented Jul 13, 2021

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

emteknetnz Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclecheese Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

emteknetnz Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chillu commented Jul 8, 2021 •

edited

Loading

emteknetnz left a comment •

edited

Loading

emteknetnz Nov 11, 2021 •

edited

Loading

emteknetnz Nov 11, 2021 •

edited

Loading

unclecheese Nov 11, 2021 •

edited

Loading

emteknetnz Nov 11, 2021 •

edited

Loading