-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor library.db into jellyfin.db and EFCore #12798
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast-path initial review
Jellyfin.Server.Implementations/Item/MediaAttachmentRepository.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Cody Robibero <cody@robibe.ro>
Changes in OpenAPI specification found. Expand to see details.What's Changed
|
ABI Difference |
…yfin into feature/EFUserData
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
| if (path is null) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably throw if it is null instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path can be null for some items, this ensures its not throwing on that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a purely functional perspective, this gets my LGTM. I've tested it with an import of my existing large library several times and it seems to cover all the bases and nothing major looks broken at this point. I have not reviewed the code itself however; I leave that to @jellyfin/server.
In terms of outstanding bugs, #13047 contains the one major one I've observed so far, but that can be fixed in a subsequent PR as it does not (I believe) affect the actual data migrations, as can any optimizations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First half of a review
This comment was marked as off-topic.
This comment was marked as off-topic.
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Migrated all data from old library.db into new tables inside jellyfin.db.
Note:
All EFCore repositories make libraral use of the null-forgiving operator
!for navigation properties. This needs to be done as those are expressions that are never actually run client side but only interpreted for SQL query generation. But as the dotnet compiler cannot know that and we have strict nullables enabled it will complain otherwise. I am strictly against putting a note like this on each query therefor this is your warning.Changes
ToDo's
Issues
All of them