Modifying existing table structure - best practices?

mtschirs

I need your advice on how to best deal with the following situation:

Often, plugins enhance the existing Vanilla application by e.g. adding additional data to a discussion.
As far as I know, there are three ways to do this:

1. Modify the structure of the Discussion table on plugin setup:
   public function Setup() { $Structure = Gdn::Structure(); $Structure->Table('Discussion') ->Column('AdditionalData', int, true, 'index') ->Set(); }
2. Add an independent new table with foreign key = DiscussionID
3. Make use of the foreignID field in the Discussion table

Now, I have seen and used approach No. 1 myself. It is fast, but if one deactivates and activates the Vanilla application again, all additional plugin data is removed from the table by default.

Approach No. 2 is more robust, but requires an additional database query each time a discussion is loaded (or perhaps a join can be added to the existing query via event?).

Approach No. 3 is equally performant as No. 1, but should probably remain reserved for plugins offering new discussion types such as the poll discussion type (hosted Vanilla).

Is there a 'best practice' solution?

hgtonight

public function Structure() {
    Gdn::Structure()->Table('Discussion')
        ->Column('AdditionalData', int, true, 'index')
        ->Set();
}

public function Setup() {
    $this->Structure();
}

Put your changes in a public method called Structure. If the class implements the Gdn_IPlugin interface, this method will be called on db structure updates if the plugin is enabled. This should cover the weird use-case of preserving data during Vanilla application deactivation.

If you are really worried about data integrity, use a foreign key in a new table.

The last thing I would mention is to make sure your fields are nullable if at all possible.

Linc

You can also use the Attributes field on the Discussion table. For data you don't need to query on, that's preferred. Otherwise, I'd prefer columns.

Using ForeignID is dicey because it's typically used to "attach" the discussion to external-to-Vanilla content, not for joining. I don't recommend that approach.

If I had a plugin that would selectively add quite a bit of data to discussions (as in, only some discussions, and lots of data I'd need to query on) I might favor the separate table approach, but probably not outside that use case. That's how we do Polls, for instance, but not Q&A, which uses new columns.

Linc

Also want to call out @hgtonight's info as very important. Disabling things in Vanilla should be non-destructive.

R_J

@hgtonight said:
The last thing I would mention is to make sure your fields are nullable if at all possible.

@Linc said:
Also want to call out hgtonight's info as very important. Disabling things in Vanilla should be non-destructive.

Does that mean new columns should accept null values? Wouldn't setting a default value be also okay? If not, why?

Linc

@R_J said:
Does that mean new columns should accept null values? Wouldn't setting a default value be also okay? If not, why?

Setting a default is also fine, yes, but typically additional columns are optional, meaning they are not set for some records, and the preferred way to represent blank/unset data in Vanilla's database is a null value.

mtschirs

@hgtonight schrieb:
Put your changes in a public method called Structure. If the class implements the Gdn_IPlugin interface, this method will be called on db structure updates if the plugin is enabled. This should cover the weird use-case of preserving data during Vanilla application deactivation.

@hgtonight: Are you sure? I couldn't find any such mechanism in the Vanilla 2.2 code. However, it seems there might be a way to preserve the additional columns by overwriting the 'BeforeSet' event of Gdn_DatabaseStructure...

Linc

structure() is indeed a special case. To my knowledge @hgtonight was correct.

Bleistivt

@mtschirs The magic happens here:
https://github.com/vanilla/vanilla/blob/fd5346/applications/dashboard/controllers/class.utilitycontroller.php#L336-L342

Also note that DatabaseStructure::Set() doesn't remove columns or drop tables by default (unless you pass true to both arguments).

mtschirs

hm, the magic you mention seems to be a very theoretical concept. I disabled and enabled the Vanilla application and all data of my 'Discussion Event' plugin was deleted from the Discussion table.

Clicking on the "Enable" button causes a GET request to be send to /dashboard/settings/testaddon/Application/Vanilla/XXXTransientKeyXXX?DeliveryType=VIEW

PHP returns the following error for this request:

ERROR IN: PHP.trigger_error();
"Can't DROP 'IX_Discussion_DiscussionEventDate'; check that column/key exists"

LOCATION: /library/database/class.database.php

BACKTRACE:
[/library/database/class.database.php] PHP::Gdn_ErrorHandler();
[/library/database/class.database.php 412] PHP::trigger_error();
[/library/database/class.databasestructure.php 344] Gdn_Database->query();
[/library/database/class.mysqlstructure.php 637] Gdn_DatabaseStructure->query();
[/library/database/class.databasestructure.php 408] Gdn_MySQLStructure->_modify();
[/applications/vanilla/settings/structure.php 130] Gdn_DatabaseStructure->set();
[/applications/vanilla/settings/class.hooks.php 767] PHP::include();
[/library/core/class.applicationmanager.php 262] VanillaHooks->setup();
[/applications/dashboard/controllers/class.settingscontroller.php 885] Gdn_ApplicationManager->testApplication();
[/applications/dashboard/controllers/class.settingscontroller.php 885] SettingsController->testAddon();
[/library/core/class.dispatcher.php 329] PHP::call_user_func_array();
[/index.php 40] Gdn_Dispatcher->dispatch();

As a result, the DiscussionEventDate column has been dropped from the Discussion table. Also, there seems to be an error in Gdn_MySQLStructure::_modify() since it tries to delete the column twice, probably due to the column being an index and Gdn_MySQLStructure::_modify() not handling dropping of indices correctly.

@Bleistivt schrieb:
Also note that DatabaseStructure::Set() doesn't remove columns or drop tables by default (unless you pass true to both arguments).

Are you sure? If the first argument 'explicit' is set to true (and it is set to true by almost? all applications and plugins), it calls Gdn_MySQLStructure::_modify() which drops these additional columns.

mtschirs

Confirmed: The flaw lies with Gdn_ApplicationManager::testApplication(): This method does not respect the 'magic'

...
// Call the application's setup method
$hooks = $applicationName.'Hooks';
if (!class_exists($hooks)) {
    $hooksPath = PATH_APPLICATIONS.DS.$ApplicationFolder.'/settings/class.hooks.php';
    if (file_exists($hooksPath)) {
        include_once $hooksPath;
    }
}
if (class_exists($hooks)) {
    /* @var Gdn_IPlugin $hooks The hooks object should be a plugin. */
    $hooks = new $hooks();
     if (method_exists($hooks, 'setup')) {
        $hooks->setup();
    }
}
...

Bleistivt

Yes. It is definitely non-destructive for plugins that do it like in @hgtonight 's example.

I have never disabled the vanilla application though. If that is destructive it should be changed.

mtschirs

Just created two github issues for this, the later (https://github.com/vanilla/vanilla/issues/2931) should be fixed quickly since user data is at risk...

Shadowdare

@Linc said:
You can also use the Attributes field on the Discussion table. For data you don't need to query on, that's preferred. Otherwise, I'd prefer columns.

Are there currently methods in the DiscussionModel to get and set attributes on discussions, or must the field be retrieved, then unserialized and modified from there?

Bleistivt

@Shadowdare said:
Are there currently methods in the DiscussionModel to get and set attributes on discussions, or must the field be retrieved, then unserialized and modified from there?

In Gdn_Model there is

• SaveToSerializedColumn
• expandAttributes and collapseAttributes
• getRecordAttribute and setRecordAttribute

And getID unserializes attributes automatically.

R_J

@rbrahmson said:
@Bleistivt wrote in Modifying existing table structure - best practices?

Also note that DatabaseStructure::Set() doesn't remove columns or drop tables by default (unless you pass true to both arguments).

Not sure what arguments you refer to.

Look in /library/database/class.databasestructure.php:

    /**
     * Creates the table and columns specified with $this->Table() and
     * $this->Column(). If no table or columns have been specified, this method
     * will throw a fatal error.
     *
     * @param boolean $Explicit If TRUE, and the table specified with $this->Table() already exists, this
     * method will remove any columns from the table that were not defined with
     * $this->Column().
     * @param boolean $Drop If TRUE, and the table specified with $this->Table() already exists, this
     * method will drop the table before attempting to re-create it.
     */
    public function set($Explicit = false, $Drop = false) {

As you can see here, set does not allow to drop a table without recreating it.

@rbrahmson said:
Basically I want to know how does one uses the model to delete a table in the Ondisable hook of a plugin (in a case where one does want to do so rather than leave data behind)?

The model interacts with the database but in Vanilla it doesn't change the structure of tables. This is done with Gdn::structure().
The structure methods have similar names like the corresponding sql commands. Deleting a table in sql is done by DROP TABLE TableName. In Vanilla you would need Gdn::structure()->table('TableName')->drop();

R_J

@rbrahmson said:
Ah!, so it's not implemented internally (suspected so from https://github.com/vanilla/vanilla/blob/master/library/database/class.databasestructure.php#L304-L308)

This is misleading. The function is not implemented in databasestructure but in mysqlstructure which extends databasestructure. That means, if you would like to support another db, you would have to implement the drop() function, otherwise you would see the error message you found in databasestructure...

rbrahmson

So "Gdn::structure()->table('TableName')->drop();" will drop the table and then recreate it???If so, not useful in the OnDisable hook.

R_J

Nope, but you also asked about the parameters of set()

This will add a column to the discussion table:

Gdn::structure()
  ->table('Discussion')
  ->column('IsSave', 'tinyint(1)')
  ->set(false, false);

This will remove every column in Discussion table and add the column:

Gdn::structure()
  ->table('Discussion')
  ->column('IsSave', 'tinyint(1)')
  ->set(true, false);

This will drop/delete the Discussion table and then recreate it with only what is specified below:

Gdn::structure()
  ->table('Discussion')
  ->column('IsSave', 'tinyint(1)')
  ->set(true, true);

But all those set() examples have in common, that at the end of the call there is still a table "Discussion".

Gdn::structure()->table('TableName')->drop(); will kill your table once and for all ("Discussion" wouldn't be wise )