refactor: Rename tables & columns to snake_case - #797
Conversation
dialmaster
left a comment
There was a problem hiding this comment.
Looks pretty good, but I found a few things that need to be addressed:
- Without
ANSI_QUOTESin sql_mode (we don't set it), MySQL/MariaDB treats"..."as a string literal, not an identifier. Using it in the alias definition (AS "timeCreated") is fine, but referencing it asORDER BY "timeCreated" DESCsorts by a constant string (i.e. doesn't sort at all), andHAVING "timeCreated" IS NOT NULLis always true. No error; it just silently stops sorting/filtering.
This hits autoRemovalQueries.getRecentVideoIds / getChannelKeepRecentIds (the keep-recent guards would pick N arbitrary videos instead of the N most recent, so auto-removal could delete exactly the videos they're supposed to protect) and videoDeletionModule.getOldestVideos (free-space cleanup would delete an arbitrary subset instead of the oldest). The other two candidate queries have the same pattern but no LIMIT, so it's mostly harmless there.
It's also not portable in the other direction: Postgres doesn't allow output-column aliases in HAVING at all. HAVING MAX(<expr>) IS NOT NULL + ORDER BY MAX(<expr>) DESC works on both dialects; a lowercase unquoted alias (AS time_created ... ORDER BY time_created) also works for the ORDER BY cases. The updated tests assert the quoted strings, so they need to change along with it.
-
In
videosModule.jsthe query now returnsyoutube_channel_name, but the consumer a few lines down still readsrow.youTubeChannelName, so channels that only exist in the videos table silently drop out of the Videos page filter dropdown. Easiest fix isAS "youTubeChannelName"like the other queries. (The test mocks that query's result as[], which is why nothing caught it.) -
migrations/20260830191259-repair-jobs-uuid-fk-collation.jswas merged to dev after you branched, and it runs ALTERs with hardcodedJobs/JobVideosnames (viamigrations/lib/jobsUuidCollation.js). Migrations execute in filename-timestamp order and your...101917sorts before...191259, so on a DB that applies both in one boot the rename runs first, and the repair'sALTER TABLE Jobs ...then fails on any database that still needs it (table names are case-sensitive on Linux). Since the repair is already merged I can't re-timestamp that one, so: can you rebase on dev and bump the rename migration's timestamp to sort after191259? Then every migration sees the names it was written against, on both fresh and upgraded DBs. -
docs/TROUBLESHOOTING.mdstill has manual SQL using the oldJobVideos/JobVideoDownloadsnames.
5d8f73d to
1f475ae
Compare
|
1. Changing the name to 4. I had noticed these uses of these table names but because these troubleshooting sections pertain to issues with migrations which will run before this rename I though it would be best to not touch these since any user needing this section will have a database in which these items have not yet been renamed. |
|
I went through this pretty carefully since it renames basically every table an existing install has.
Overall... this looks pretty good, but I'm always wary about rolling out something like this since it if anything goes wrong it will completely break Youtarr for existing users and make fixing it a difficult/manual process :) |
1f475ae to
2abf3d8
Compare
|
I've added tests that explicitly cover all 3 LCTN variants and have adjusted the migration to work in all these cases (which includes the two-step rename but also adds a check whether the table exists in the column renames as otherwise downgrading from a partial migration might not work). I understand you being careful about this change and I appreciate the thorough reviews & you testing this on a much more representative dataset than I possess. I've also include an SQL script below that one can use to rollback this migration manually so that we have a fallback in case any users do end up having issues. For someone with a partial migration this'll probably throw some errors for the items that already have the old names but that's fine since that just means that those statements weren't needed anyway for that database. We could include this in the troubleshooting docs if you'd like, but I'll leave that decision up to you. ALTER TABLE `apikeys` RENAME TO `apikeys-tmp`;
ALTER TABLE `apikeys-tmp` RENAME TO `ApiKeys`;
ALTER TABLE `jobvideodownloads` RENAME TO `jobvideodownloads-tmp`;
ALTER TABLE `jobvideodownloads-tmp` RENAME TO `JobVideoDownloads`;
ALTER TABLE `jobvideos` RENAME TO `jobvideos-tmp`;
ALTER TABLE `jobvideos-tmp` RENAME TO `JobVideos`;
ALTER TABLE `jobs` RENAME TO `jobs-tmp`;
ALTER TABLE `jobs-tmp` RENAME TO `Jobs`;
ALTER TABLE `sessions` RENAME TO `sessions-tmp`;
ALTER TABLE `sessions-tmp` RENAME TO `Sessions`;
ALTER TABLE `videos` RENAME TO `videos-tmp`;
ALTER TABLE `videos-tmp` RENAME TO `Videos`;
ALTER TABLE `channels` CHANGE `last_fetched_by_tab` `lastFetchedByTab` TEXT;
ALTER TABLE `channelvideos` CHANGE `published_at` `publishedAt` VARCHAR(255) DEFAULT NULL;
ALTER TABLE `Jobs` CHANGE `job_type` `jobType` VARCHAR(255) NOT NULL;
ALTER TABLE `Jobs` CHANGE `time_created` `timeCreated` DATETIME NOT NULL;
ALTER TABLE `Jobs` CHANGE `time_initiated` `timeInitiated` DATETIME NOT NULL;
ALTER TABLE `media_server_users` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `media_server_users` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
ALTER TABLE `playlist_sync_state` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `playlist_sync_state` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
ALTER TABLE `playlists` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `playlists` CHANGE `last_fetched` `lastFetched` DATETIME DEFAULT NULL;
ALTER TABLE `playlists` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
ALTER TABLE `playlistvideos` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `playlistvideos` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
ALTER TABLE `Sessions` CHANGE `created_at` `createdAt` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
ALTER TABLE `Sessions` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
ALTER TABLE `subfolders` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `subfolders` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
ALTER TABLE `video_watch_status` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `video_watch_status` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
ALTER TABLE `Videos` CHANGE `audio_file_path` `audioFilePath` VARCHAR(500) DEFAULT NULL;
ALTER TABLE `Videos` CHANGE `audio_file_size` `audioFileSize` BIGINT(20) DEFAULT NULL;
ALTER TABLE `Videos` CHANGE `file_path` `filePath` VARCHAR(500) DEFAULT NULL;
ALTER TABLE `Videos` CHANGE `file_size` `fileSize` BIGINT(20) DEFAULT NULL;
ALTER TABLE `Videos` CHANGE `original_date` `originalDate` VARCHAR(255) DEFAULT NULL;
ALTER TABLE `Videos` CHANGE `youtube_channel_name` `youTubeChannelName` VARCHAR(255) NOT NULL;
ALTER TABLE `Videos` CHANGE `youtube_video_name` `youTubeVideoName` VARCHAR(255) NOT NULL;
ALTER TABLE `Videos` CHANGE `youtube_id` `youtubeId` VARCHAR(255) NOT NULL;
ALTER TABLE `watch_status_sync_cursors` CHANGE `created_at` `createdAt` DATETIME NOT NULL;
ALTER TABLE `watch_status_sync_cursors` CHANGE `updated_at` `updatedAt` DATETIME NOT NULL;
DELETE FROM `SequelizeMeta` WHERE `name` = '20260830201917-lowercased-table-column-names.js' |
As discussed in Discord. This changes all table & column names to be lowercase/snake_cased (with the exception of the Sequelize metadata table which we don't manually interact with in any way).
This is done in part because it'll make adding PostgreSQL support a bit easier (since that automatically lowercases any such names unless quoted), but it also makes things more consistent since new tables/columns already follow this format.
Some notes:
underscoredoption for models will automatically use snake_cased names for the camelCased attributes, so thefieldkey is only specified for the two attributes that would have ayou_tube_...column following this format as that seemed undesirable.underscoredoption for models is also applied to thecreatedAtandupdatedAtattributes that are added by thetimestampsoption.--lower_case_table_names=1and verified that worked as expected.The raw queries are the majority of the changes here. I've done my best to trigger the code paths containing these to verify them on a running instance, but I'm sure I've missed some (permutations) so these should be reviewed carefully.