Normalize the database

[jonasraoni]

Describe the problem you would like to solve
Normalize the database in order to have better expectations (e.g. ensure a record is always identifiable by a single ID), improve the performance, decrease the amount of rows and also improve the data integrity.

Describe the solution you'd like

1. DONE: Add an ID field to all tables and discard composite primary keys. Change database tables schema for Clustering #3573

2. Move non-localized fields from the *_settings tables to their origin table (e.g. user_settings > user), I think we have few cases, I just remember of seeing an issueId somewhere. (filed for issueId)

3. Move dynamic localized fields from the *_settings tables to a better typed table

• The moved field will be strongly typed instead of just a string, which will speed up queries against it
• Simplify the queries (instead of having a join for each field or an extra query returning all the settings, a single join will make all the localized fields available)
• Allow for customized indexes in specific fields
• Easier to upgrade (renaming a field vs updating the setting_name for N rows)
• Less prone for leaving orphan data behind
• The *_setting table might be left (reserved for plugins) or removed altogether (plugins will deal with their data directly).

4. Remove duplicated fields from dependent entities (e.g. remove the review_round_files.stage_id given that stage_id exists in its parent entity review_rounds.stage_id)

5. Replace the columns assoc_type/assoc_id by "real" columns with integrity checks.

Who is asking for this feature?
Developer

---

Draft for the conversion process for the item 3

Taking the user_settings as an example, where each user have at least two rows in the user_settings (one for the givenName and another for the familyName).

• Create a table user_locale (id int, user_id int, locale varchar, given_name varchar, family_name varchar, ...)`, with all the localized fields (unique key index on the bolded fields), this way we'll have 1 record for each locale + user.
• Update the queries to consider the new structure
• Create migration scripts

Extra
If we're going to keep the *_settings for something else, perhaps we can use the JSON aggregate functions to return the data instead of having it loaded in a loop.

[asmecher]

2. Move non-localized fields from the *_settings tables to their origin table (e.g. user_settings > user), I think we have few cases, I just remember of seeing an issueId somewhere.

I think you're referring to the issueId setting in publication_settings. Yes, this should definitely be normalized.

For the rest, I don't have a problem with what you're proposing per se, but it would be a pretty significant development effort. If it turns out that we're hitting e.g. a performance limit, then I could see prioritizing it -- but we'd need to establish that first.

[asmecher]

I've filed this for issueId in publication_settings: #10157

[asmecher]

Also for category IDs: #10158

[jonasraoni]

Yeah, it was the issueId! I understand it's tough and will introduce several breaking changes, I just wanted to leave the idea saved somewhere.

[NateWr]

I like the idea but my big concern with normalizing the _settings tables is that we need to continue to have an extensible data structure. If we normalize some fields (eg - given_name/family_name), we won't be able to get rid of the _settings table because plugins might make use of it to attach additional data.

Because of this, normalizing the core localized fields might lead to more complex SQL queries, because we would then have to join across an additional table (users, user_locale and user_settings).

I think probably our biggest performance issue is around searching/filtering. I wonder if it makes sense -- where we identify a performance issue -- to create a normalized table specifically for searching/filtering. That could really inflate the size of our DB, though.

[jonasraoni]

Yeah! I was updating a plugin and saw this feature being used =]

• I'm totally ok to switch an extra join by data integrity! It's also easier to see what fields are available in the database, with the settings it's not possible to know it until some data is written or someone inspect the code.

• About removing the settings: My understanding is that a plugin might have a data structure that may not fit well into a setting table, so I'd be inclined to, in the long run, let them manage their data in a separated place and just offer a way to extend the queries (to return data in a single query)... But this should be discussed in another issue, for this one, I'd stick to the data normalization.

ps: I'll avoid discussing the real performance issues here, but I'm open for receiving pointers/data 😁

[jonasraoni]

Another issue related to our generic tables, is data corruption, on this screenshot from a user, it's possible to see a missing setting_type and an invalid JSON (missing the []), how did it happen? No idea, but a good shaped database would make it impossible to happen.

[asmecher]

About the settings tables, here are (in my opinion) our requirements/motivations:

• We need a way to support extensible data without adding new tables every time:
  • We use this internally to allow apps to extend pkp-lib data. As long as this isn't used for foreign keys, it's convenient; however, it's not a firm requirement as we have other ways of achieving it.
  • However, this is a very useful feature for plugins. Forcing plugins to create/maintain their own tables to augment built-in data is not an option IMO. Plugins are free to add tables if they want, but for simple data, it's way too much overhead.
• It shouldn't be hard to support localized data. If it is, developers will get lazy and introduce single-language fields.

Options for maintaining these features while moving to Eloquent models:

• @Vitaliy-1 suggests exploring Laravel global scopes for allowing Eloquent models to augment their data with contents from the settings table.
• Spatie's schemaless attributes for Eloquent might be an option.
• We could keep "purist" and create an Eloquent model for the settings too? (Would this be performant?)
• Others?

[jonasraoni]

We use this internally to allow apps to extend pkp-lib data. As long as this isn't used for foreign keys, it's convenient; however, it's not a firm requirement as we have other ways of achieving it.

Hmm, I just don't agree with this specific usage. It's convenient, but it's not cheap (leaves trash data behind, non-indexable, non-typed, non-discoverable, ...). If we need an author name, and we know it's a localized string, then we should have a physical and typed place to accommodate it, so I'd rather have what I specified in the description:

author_locale (id int, author_id int, locale varchar, given_name varchar, family_name varchar, ...)

Spatie's schemaless attributes for Eloquent might be an option.

I've just skimmed this one, it sounds better than keeping a side table (e.g. _settings), as the data will live on the same row, this is the type of thing I've used in the past.

The implementation of this package seems to be simple (I didn't flag interesting optimizations, such as an incremental JSON parser or usage of JSON functions from the database), but it should be enough for us.

The only drawback would be a possible plugin abuse (e.g. inserting an image), but it's out of our business to control abuses from a plugin. A long text field is enough to store a lot of side data...

Others

I normally try to follow the all-or-nothing thinking. So I'd have a *_settings table (or a data column) for all entities or something else that fits everything, like a single settings table (which is probably the worst option, due to the lack of foreign keys).

[jonasraoni]

Also, there's a clear path to migrate the existing settings into the schemaless attributes. The JSON format supports all of our "internal data types" (just the date type will need some maintenance).

[Vitaliy-1]

I've looked at adding support for settings table in Eloquent. What I've tried so far:

1. Idea with Global scopes. It's possible to modify the query before Model is initialised, e.g., considering two tables with one to one relationship:

class NewScope implements Scope
{
    public function apply(Builder $query, Model $model)
    {
        $query->leftJoin(
            table2,
            table2.something_id,
            '=',
            table1.something_id
        );
    }
}

This will initialise the model with attributes derived from columns of these two tables. The idea was to create a virtual columns where settings_name value represents a column's name but I forgot that we have localized data and this structure doesn't fit to the concept.
2. Another idea is using a separate model for the settings table. I found the library that implements the similar thing: laravel-translatable. According to the documentation, their table structure for localizations is different then ours:

Schema::create('post_translations', function(Blueprint $table) {
    $table->increments('id');
    $table->integer('post_id')->unsigned();
    $table->string('locale')->index();
    $table->string('title');
    $table->text('content');

    $table->unique(['post_id', 'locale']);
    $table->foreign('post_id')->references('id')->on('posts')->onDelete('cascade');
});

That also will require additional code adaptations on our side but, at least, it already has an interface to interact with localized data. This method shows that they are initialising the new model for the translation table: https://github.com/Astrotomic/laravel-translatable/blob/main/src/Translatable/Translatable.php#L177
3. We can extend the functionality of the Query Builder and Model by ourselves. The scope of work to extend the Model is going to be similar to what laravel-translatable does. For the Query Builder, to support settings table, we will need to add methods to interact with settings, to do queries with WHERE clause, UPDATE/INSERT values, etc. I haven't found examples in the wild for such work. This will break the concept of one Model per table, maybe that's why no one tried that. I'd say that in our scenario it's acceptable.

[Vitaliy-1]

After another look, I'd say that the best approach for preserving settings table is definitely creating a new Model and a custom Relation for it. That's a huge amount of work but the solution is definitely more extensible than dumping that into JSON.

[Vitaliy-1]

Found 2 libraries that might be useful. They mimic settings table structure and create the way to interact with settings as attributes of the main Model. Both needed to be extended to support multilingual values: https://github.com/kodeine/laravel-meta/tree/2.0 and https://github.com/kolossal-io/laravel-multiplex/tree/main

[asmecher]

Good spotting, @Vitaliy-1! There is also plank/laravel-metable, which is very similar to kolossal-io/laravel-multiplex.

I think kodeine/laravel-meta is the closest parallel, as it uses paired tables to go with the entities, where the other two use one global metadata table with polymorphic relationships to the Eloquent models, and is focused on timestamping metadata for versioning, which isn't important for us. I don't think a global table would scale well.

You're right that we have a further wrinkle in multilingual support. But the library is relatively simple, and very close to what we need, so both extending it and taking it as a pattern for a new trait are options. Additions to support repeated elements might even be an option for upstream contribution, as at a glance the library is neutral about repeated keys (I don't see a unique index on the example schema's key) and permitting these has huge benefits for searchability of elements.

[Vitaliy-1]

Looking closer to those libraries I've decided that it would be easier to extend Eloquent than adapting those to fit our settings table model.
The branch I'm working on: https://github.com/Vitaliy-1/pkp-lib/tree/laravel_permission
The examples can be tested using this API endpoint: https://github.com/Vitaliy-1/pkp-lib/blob/83233345bd540d5fd6e58f8104e329ca7972453a/api/v1/_submissions/PKPBackendSubmissionsController.php#L523

What I'm suggesting is to override Eloquent Builder, which pairs the Model with the Query Builder.

• When retrieving or inserting the Model, it runs additional query to get/insert matching results from/to the settings table
• When updating , it appends a series of SQL conditional statements on the settings table to update values: SET setting_value CASE WHEN setting_name='...' AND locale='...' THEN '...'. Does it sound as a good idea? If not, it could be just a series of updates.
• This requires additional functionality to allow selecting models based on values from the settings table, something like $user->whereSettings('firstName', 'David') or even ->where('firstName', 'David')

[asmecher]

@Vitaliy-1, this is looking very good! If you'd like to open a draft PR I can comment directly on the work in progress there. But at a glance, a few early thoughts...

• The extension of the Eloquent builder can be done as a trait in the model class. This can also add the requirement for getSettings to be implemented. This will make it easy for other classes to add settings table support.
• I'd like us to think a little about the future of the JSON schema stuff as we start to migrate some of the major entities over to Eloquent. If we're going to move to Eloquent API resources in the future, I see two paths:
  • Maybe we could have an implementation e.g. of getSettings that draws from the JSON schema rather than reiterating the list of settings, or
  • Maybe we deprecate the JSON schema and just start coding things right into the Eloquent models and resource classes.

[asmecher]

@Vitaliy-1, I've also pinged @touhidurabir to have a look at this -- he's run into the need for a settings table solution in his work with #10292, and has a lot of Laravel experience that'll be helpful in adding to my review. When you're ready for him to have an in-depth look, could you open a PR for ease of commenting?

[Vitaliy-1]

I've switched from figuring out a general concept to implementation on an announcement entity: #10328. I'll make a PR once it more or less ready. Besides other things, I'm integrating schema into the Eloquent, decided to preserve it.

[asmecher]

That sounds great -- thanks, @Vitaliy-1!

[Vitaliy-1]

In general, we would need to keep some information from schema in the Model. I've created a property, which holds, for now, a setting name and boolean, whether it's multilingual. Settings not listed here, won't be processed. It's a place for potential plugin hook.

protected array $settings = [
    'familyName' => true,
    'givenName' => true,
    'affiliation' => true,
];

We might extend this to support data type, validation

'familyName' => [
    'multilingual' => true,
    'type' => 'string',
    ....
]

Or specify type in $casts Laravel's property of the Model...

The examples of what is already supported:

// retrieves Model instance with settings as normal attributes
$user = UserModel::find(2);

// sets a multilingual setting value
$user->givenName = [ 
    'en' => 'David'
];

// creates a new model and saves it in the database
$newUser = new UserModel();
$newUser->fill([
    'email' => '[email protected]',
    'givenName' => [
        'en' => 'Skywaker',
        'fr_CA' => 'Cielmarcheur'
    ],
    'username' => 'lskywalker',
    'password' => Str::random(10),
]);


// delete a model with associated settings
$newUser->delete();

What is missing so far, are filters for Models, which involve settings, e.g., search filter that we use in the User entity, something like:

UserModel::whereSetting('givenName', 'LIKE', '...');

Also, interface to work with multilingual settings should be better.

[Vitaliy-1]

I've added the ability for Models to execute select statements, like:

UserModel::where('givenName', 'Waca')->first()
UserModel::whereIn('givenName', ['Daniel', 'Greg'])->get()

Under the hood such calls are first delegated to the Eloquent Builder and then to the Query Builder. And in my scenario I'm just extending Eloquent Builder by adding a subquery to look for results in the setting table. I don't think we need more intervene in the Eloquent Builder logic (and I'm still considering whether overriding these 2 methods is necessary). Instead, additional filters/specifiers is better to handle on the Model's side as scopes by case per case basis. E.g., I'm looking currently at the complex case in PKP\user\Collector::buildSearchFilter(), where we have joins with the user_settings table, user_interests and controlled_vocab*.

[Vitaliy-1]

Keeping a note here that we would need to improve type casts for multilingual values. For now I'm omitting it as implementation might conflict with eloquence library, which we use for transforming Model's attributes to camel case

[touhidurabir]

Just putting as reference, Issue #10476 to handle the casting of multilingual values .

[jonasraoni]

Not sure if this is covered by the current code, but extra care might be needed with this type of cruft (which might still exist):

Therefore, the long-term fix for me still revolves around eliminating the settings table (which can be left for really generic user/plugin settings), the performance (which will probably be way better with typed columns) isn't even important when compared with the data integrity issues.