Series cleanup

[nichwall]

This PR fixes #3207 by combining series that have identical names and the same libraryId. I originally only used the series name, but realized that the same series may exist in two different libraries.

This PR does not fix the "duplicate entries" causing problems for the InputDropdown component, and instead fixes the data in the database itself.

This is done by adding a cleanup function to server startup which updates all bookSeries entries that have duplicate series names and libraryIds to point to the newest series, and then deletes the now-unused duplicates.

This will help handle issues with series from previous server versions, but I am not sure of the root cause of duplicate series (whether from a scanner, matching, or manual editing of an item). I briefly looked at all occurrences of SeriesModel and did not see anything that looked like it could cause duplicate series like this, so it may be a leftover bug from an older server version.

[nichwall]

Actually, I will also need to update any Feeds which reference one of the deleted series.

[mikiher]

If you want to force the (libraryID, name) pair to be unique, it would be good to add a unique constraint to the series table.
This will cause an exception to be thrown if some present/future code tries to update the table in a way that results in a duplicate row.

[advplyr]

I haven't looked into this yet but at a glance I think we should call this from the cleanDatabase function in Database. At this stage we may want to break that out into a separate file/class.

[nichwall]

Oh, can we add the unique constraint after a column is created? I'm still not very familiar with table creation/modification.

Yeah, if we want to move this to another file for the database creation, we can close this PR. I spent some time last night trying to get the original query to sort by how many feeds reference the series ID, then feed creation time, then series creation time to ensure that if a feed exists for one of the series, we prioritize that one. I couldn't figure out the syntax.

[advplyr]

Unfortunately when I set up Sequelize I didn't take the time to add the migrations table to handle db updates.
I thought it would be something that could be added in later but it is much harder to do now.

Ideally when doing a table update you also supply a path to revert the changes when downgrading versions. Currently all the table updates are one-way and were all done around v2.3.0. I'd like to set up the migrations table before doing any more one-way table updates like that.

[mikiher]

Oh, can we add the unique constraint after a column is created? I'm still not very familiar with table creation/modification.

Yeah, that should be possible (at least in theory). You just add the following to your series model init:

  static init(sequelize) {
    super.init(
      {
        ...
      },
      {
        sequelize,
        modelName: 'series',
        indexes: [
          ...
          // New multi-column index
          { 
            unique: true, // Add the unique constraint
            fields: ['libraryId', 'name']
          }
        ]
      }
    );

But as @advplyr asks, it should be done as part of a two-way migration setup.

[mikiher]

@advplyr, @nichwall, would you like me to take a stab at creating a migration setup?

From what I read, I understand umzug is migration tool that is part of the sequelize umbrella, and it seems relatively easy to integrate at server startup to migrate the database up or down based on the server version. It stores executed migrations in the database in a special table, containing an entry for each executed migration. Each migration script has an up and down implementation that can be called by umzug.

I doubt we have many users still running v2.3.0 at this point, so it would probably be safe to start with the current DB schema as baseline - please let me know what you think.

[advplyr]

Yeah that would be great. I looked into it briefly and set up a test so I'm somewhat familiar with it. I agree that we can start with the current DB schema

[nichwall]

Yeah, that sounds like a good plan. Do these migrations work for arbitrary steps? Like if I said "Make a new table that contains all of the tags", could we create the new table and populate it from the JSON in the books table, and then when reverting it would delete that table and add the data back in the JSON? Or is it just adding/removing columns and tables that we can do?

[mikiher]

You can write whatever you want in the migration, and it's fully up to you tobdecide and implement the migration's up and down semantics.

…

On Sun, Aug 25, 2024, 18:13 Nicholas W ***@***.***> wrote: Yeah, that sounds like a good plan. Do these migrations work for arbitrary steps? Like if I said "Make a new table that contains all of the tags", could we create the new table and populate it from the JSON in the books table, and then when reverting it would delete that table and add the data back in the JSON? Or is it just adding/removing columns and tables that we can do? — Reply to this email directly, view it on GitHub <#3326 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMDFVS4VLT667IQAZNOT7TZTHYB7AVCNFSM6AAAAABM7MNGMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBYHA4TAOBZGM> . You are receiving this because you commented.Message ID: ***@***.***>

[mikiher]

@nichwall, @advplyr, can you please take a look at and review a small design doc I've written for this? This is a sensitive feature, so I want to make sure I'm doing it correctly.

I think the main question I'd like your input on is what to do on migration failures, but please let me know if you have any other comments or questions. I also have the code mostly ready as well - I'll send a PR once you've reviewed and approved the design.

[nichwall]

Now that the migrations are implemented in #3378, do you want me to update this PR to instead use the migration for making the column unique, or do y'all want to do the migration to get another example of how to do a simple database migration?

[mikiher]

From my perspective, please go ahead and update. It would be good to have someone other than myself try to write and test a migration script based on the instructions. I'll be happy to review the code.

[nichwall]

Sounds good. I'll try and get to that soon but it may be a few days.

[nichwall]

Well, looks like I merged the branches incorrectly. I will close this PR and open a new one in a few hours because I'll be making a lot of changes to how this is done anyway.