NEW: Add method to DB to check if a class is ready for db queries. #10041

GuySartorelli · 2021-08-03T03:43:40Z

This effectively makes the functionality from Security::database_is_ready() reusable for any arbitrary DataObject subclass.

Let me know if I need to write any specific tests for this, but my assumption is that this is already being tested through the tests that target Security specifically.

GuySartorelli · 2021-08-03T03:55:37Z

Some tests are failing - marking as draft until I get that fixed.
Edit: Hmm... I can't see any reason why this would fail. If I do effectively exactly what the test is doing, but outside of a test context, it works perfectly fine.
If someone sees this and can see why this is failing I'd appreciate it - otherwise I'll just have to come back to this later.

src/ORM/DB.php

dhensby · 2021-08-03T15:24:58Z

src/Security/Security.php

+            Permission::class,
+        ];
+        foreach ($toClear as $class) {
+            DB::clear_database_is_ready($class);


this feels like a subtle, but still significant change in behaviour. When before this would completely reset the ready state of the database, we are now only resetting the ready state of some particular classes. As such, I think this should call DB::clear_database_is_ready() (no argument) to completely reset the state and so that it has the equivalent behaviour to before.

The way I think of it, the only reason it completely set the "ready" state of the database is because the "ready" state only recorded classes relevant to Security anyway - there was no way to record a "ready" state for any other classes/tables.
I'll make this change if you want me to, but I think it is more logically consistent for Security::clear_database_is_ready to only clear the ready state of classes relevant to Security - i.e. the classes that have their ready state set by Security::database_is_ready

I think have the list of 3 classes is correct, because that does mimick the previous behaviour because there was no way that a class other than Member, Group or Permission would be added to the $database_is_ready array

However it also previously had ClassInfo::dataClassesFor(Member::class) meaning subclasses of Member (though looks like it was assumed Group and Permission would never be subclassed, which seems fair enough, we've make a similar assumption about Folder in the assets module)

So you'll need to update this array to include all Member subclasses

So you'll need to update this array to include all Member subclasses

DB::clearDatabaseIsReady() has an optional $clearFullHierarchy parameter which defaults to true, which the adds ClassInfo::dataClassesFor($class) to the list of classes to be cleared - so subclasses of Member are already cleared with the current implementation.

dhensby · 2021-08-03T15:26:59Z

src/Security/Security.php

+            Permission::class,
+        ];
+        foreach ($toForce as $class) {
+            DB::force_database_is_ready($class, $isReady);


as with clear_database_is_ready() - I think this is a subtle but significant behaviour change

My thinking is the same as with clear_database_is_ready - but I'll make a change if you want me to. What change would you recommend here? I don't think it would be appropriate to force DB to consider all classes as "ready".

Ditto include Member subclasses

Because this is only used for tests, and those tests don't need us to check subclasses of Member are ready, and databaseIsReady returns true if the class passed in is in $databaseReadyClasses (with the assumption that it has therefore already gone through the full hierarchy check), I don't think we need to add Member subclasses here.

This is also consistent with the old check:

if (self::$force_database_is_ready !== null) { return self::$force_database_is_ready; }

dhensby · 2021-08-03T15:28:56Z

If someone sees this and can see why this is failing I'd appreciate it - otherwise I'll just have to come back to this later.

This is the test that is failing:

silverstripe-framework/tests/php/Security/SecurityTest.php

Lines 691 to 709 in dc3b907

    
               public function testDatabaseIsReadyWithInsufficientMemberColumns() 
        
               { 
        
                   Security::clear_database_is_ready(); 
        
                   DBEnum::flushCache(); 
        
                   // Assumption: The database has been built correctly by the test runner, 
        
                   // and has all columns present in the ORM 
        
                   /** 
        
                    * @skipUpgrade 
        
                    */ 
        
                   DB::get_schema()->renameField('Member', 'Email', 'Email_renamed'); 
        
                   // Email column is now missing, which means we're not ready to do permission checks 
        
                   $this->assertFalse(Security::database_is_ready()); 
        
                   // Rebuild the database (which re-adds the Email column), and try again 
        
                   static::resetDBSchema(true); 
        
                   $this->assertTrue(Security::database_is_ready()); 
        
               }

whilst I can't see exactly why it is failing, stepping through with a debugger should reveal what is going on.

GuySartorelli · 2021-08-03T21:39:48Z

I had seen that that was the test that was failing, and as I mentioned I replicated what it was doing outside of a test context and saw the expected values returned...
I don't have the infrastructure to step through tests with a debugger at the moment (if you can point me to a write up or docker image or similar for setting that up, it would be massively helpful but otherwise it'll just be a wee bit longer before I can get around to it).
I'll leave this in draft until I can get that sorted out.

GuySartorelli · 2022-02-18T00:29:42Z

Tests are passing! I've made the changes requested by @dhensby except the ones that I don't agree with - see discussion above.

src/ORM/DB.php

emteknetnz · 2022-03-03T03:55:20Z

src/Security/Security.php

-     * @var boolean If set to TRUE or FALSE, {@link database_is_ready()}
-     * will always return FALSE. Used for unit testing.
-     */
-    protected static $force_database_is_ready;


This is an API deletion, would require a new major

Just leave it in and deprecate it. Along with $database_is_ready below

For deprecating this, do you also want it to be populated when the method is called, and to affect the outcome of the checks? Or just leave the variable here unused? Same with $database_is_ready

Probably best to keep them functioning as they currently are if it's not too much extra work

src/ORM/DB.php

emteknetnz · 2022-03-03T04:04:10Z

src/Security/Security.php

+            Permission::class,
+        ];
+        foreach ($toClear as $class) {
+            DB::clear_database_is_ready($class);


I think have the list of 3 classes is correct, because that does mimick the previous behaviour because there was no way that a class other than Member, Group or Permission would be added to the $database_is_ready array

However it also previously had ClassInfo::dataClassesFor(Member::class) meaning subclasses of Member (though looks like it was assumed Group and Permission would never be subclassed, which seems fair enough, we've make a similar assumption about Folder in the assets module)

So you'll need to update this array to include all Member subclasses

emteknetnz · 2022-03-03T04:04:34Z

src/Security/Security.php

+            Permission::class,
+        ];
+        foreach ($toForce as $class) {
+            DB::force_database_is_ready($class, $isReady);


Ditto include Member subclasses

This effectively makes the functionality from Security::database_is_ready() reusable for any arbitrary DataObject subclass.

GuySartorelli · 2022-03-03T05:27:46Z

@emteknetnz requested changes made - there's a question there about how far to go with the deprecation and more discussion to be had perhaps about including Member subclasses.

src/ORM/DB.php

emteknetnz · 2022-04-06T04:05:45Z

src/ORM/DB.php

+     * @param string $class
+     * @return boolean
+     */
+    public static function databaseIsReady(string $class): bool


This is a bit of a misleading method name since it's for a specific dataobject, rather than the entire database. Maybe rename it to something like tablesCreated(string $class): bool. Also update the protected static property to match

That's a fair point. tablesCreated isn't quite right, since it's checking more than just if the tables are created (e.g. the table may be created but missing columns), but you're right that the current name isn't right either. I'll think of something more accurately descriptive.

emteknetnz · 2022-04-06T04:07:22Z

src/ORM/DB.php

+     * @param string $class The class to be forced as ready/not ready.
+     * @param boolean $isReady The value to force.
+     */
+    public static function forceDatabaseIsReady(string $class, bool $isReady)


This is method is pretty unecessary IMO and it just clutters up the API. I think you're better off just update the visibility of the property to public via Reflection in the unit test and then updating the property directly

Security::force_database_is_ready() is a public method, which is what's called by the test. I agree that ideally the method wouldn't exist in the new location... should I replace it with reflection in Security::force_database_is_ready() (so that method still does what it has always done)?
And then any new tests that might need that functionality can use reflection directly in the tests?

Leave the existing Security method as is and use reflection within the new unit test. That will minimise the amount of API changes

nvm I realised a much cleaner way to handle this, which goes in line with #10041 (comment)
Edit: Posted this without seeing your latest reply - yup that's exactly the thinking.

emteknetnz · 2022-04-06T04:08:09Z

src/Security/Security.php

-     * @var boolean If set to TRUE or FALSE, {@link database_is_ready()}
-     * will always return FALSE. Used for unit testing.
-     */
-    protected static $force_database_is_ready;


Probably best to keep them functioning as they currently are if it's not too much extra work

GuySartorelli · 2022-04-08T00:32:35Z

I've realised that DB isn't the best place for this - at least not in the way it has been declared. DB doesn't have any knowledge of DataObjects as it stands - but it does know about tables. I'm going to rethink this a bit.

GuySartorelli · 2022-04-08T01:53:31Z

Closing this in favour of #10276 as I no longer have write access to this PR's forked repo.

GuySartorelli marked this pull request as draft August 3, 2021 03:55

dhensby reviewed Aug 3, 2021

View reviewed changes

GuySartorelli mentioned this pull request Feb 16, 2022

--no-database schema build fails with various module configuration silverstripe/silverstripe-graphql#445

Open

1 task

GuySartorelli force-pushed the feat/reusable-db-check branch 3 times, most recently from 9ce74ed to 9c8f942 Compare February 17, 2022 23:58

GuySartorelli marked this pull request as ready for review February 18, 2022 00:29

GuySartorelli requested a review from dhensby February 18, 2022 00:29

emteknetnz requested changes Mar 3, 2022

View reviewed changes

NEW: Add method to DB to check if a class is ready for db queries.

014dc19

This effectively makes the functionality from Security::database_is_ready() reusable for any arbitrary DataObject subclass.

GuySartorelli force-pushed the feat/reusable-db-check branch from 9c8f942 to 014dc19 Compare March 3, 2022 05:24

GuySartorelli commented Mar 3, 2022

View reviewed changes

src/ORM/DB.php Show resolved Hide resolved

GuySartorelli requested a review from emteknetnz March 31, 2022 04:46

emteknetnz requested changes Apr 6, 2022

View reviewed changes

GuySartorelli mentioned this pull request Apr 8, 2022

NEW: Add method to check if a class is ready for db queries. #10276

Open

GuySartorelli closed this Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW: Add method to DB to check if a class is ready for db queries. #10041

NEW: Add method to DB to check if a class is ready for db queries. #10041

GuySartorelli commented Aug 3, 2021

GuySartorelli commented Aug 3, 2021 •

edited

dhensby Aug 3, 2021

GuySartorelli Aug 3, 2021

emteknetnz Mar 3, 2022

GuySartorelli Mar 3, 2022

dhensby Aug 3, 2021

GuySartorelli Aug 3, 2021

emteknetnz Mar 3, 2022

GuySartorelli Mar 3, 2022

dhensby commented Aug 3, 2021

GuySartorelli commented Aug 3, 2021

GuySartorelli commented Feb 18, 2022

emteknetnz Mar 3, 2022

GuySartorelli Mar 3, 2022

emteknetnz Apr 6, 2022

emteknetnz Mar 3, 2022

emteknetnz Mar 3, 2022

GuySartorelli commented Mar 3, 2022

emteknetnz Apr 6, 2022

GuySartorelli Apr 8, 2022 •

edited

emteknetnz Apr 6, 2022

GuySartorelli Apr 8, 2022

emteknetnz Apr 8, 2022

GuySartorelli Apr 8, 2022 •

edited

emteknetnz Apr 6, 2022

GuySartorelli commented Apr 8, 2022 •

edited

GuySartorelli commented Apr 8, 2022

NEW: Add method to DB to check if a class is ready for db queries. #10041

NEW: Add method to DB to check if a class is ready for db queries. #10041

Conversation

GuySartorelli commented Aug 3, 2021

GuySartorelli commented Aug 3, 2021 • edited

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

dhensby commented Aug 3, 2021

GuySartorelli commented Aug 3, 2021

GuySartorelli commented Feb 18, 2022

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

GuySartorelli commented Mar 3, 2022

Choose a reason for hiding this comment

GuySartorelli Apr 8, 2022 • edited

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

GuySartorelli Apr 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Apr 8, 2022 • edited

GuySartorelli commented Apr 8, 2022

GuySartorelli commented Aug 3, 2021 •

edited

GuySartorelli Apr 8, 2022 •

edited

GuySartorelli Apr 8, 2022 •

edited

GuySartorelli commented Apr 8, 2022 •

edited