wjones
3e5d29225a
- Removed unused classes and files related to SQLite database provider, including SqliteDesignTimeJellyfinDbFactory, ModelBuilderExtensions, PragmaConnectionInterceptor, AssemblyInfo, SqliteDatabaseProvider, DateTimeKindValueConverter. - Updated tests to remove dependencies on removed classes and adjusted mocking for configuration sections. - Added Microsoft.EntityFrameworkCore.Sqlite package reference to test project. - Improved string handling in tests for better consistency and clarity. - Refactored logging methods in JellyfinApplicationFactory for better readability and maintainability.
245 lines
15 KiB
Markdown
245 lines
15 KiB
Markdown
# EF Core Removal Analysis — pgsql-jellyfin Fork
|
||
|
||
**Date**: 2025
|
||
**Scope**: Full codebase analysis of whether EF Core can and should be removed from this PostgreSQL-only fork.
|
||
**Verdict**: **Do not remove EF Core.** Address specific friction points instead.
|
||
|
||
---
|
||
|
||
## 1. Usage Inventory
|
||
|
||
EF Core (`IDbContextFactory<JellyfinDbContext>`) is injected into **82 call sites across ~35 source files**, permeating every layer:
|
||
|
||
| Layer | Files |
|
||
|---|---|
|
||
| API | `DatabaseViewsController` |
|
||
| Repository (Item) | `BaseItemRepository`, `PeopleRepository`, `MediaStreamRepository`, `MediaAttachmentRepository`, `KeyframeRepository`, `ChapterRepository` |
|
||
| Repository (Other) | `ActivityManager`, `DeviceManager`, `DisplayPreferencesManager`, `UserManager`, `TrickplayManager`, `MediaSegmentManager`, `AuthenticationManager`, `AuthorizationContext`, `LibraryOptionsRepository`, `BackupService` |
|
||
| Scheduled Tasks | `PeopleValidationTask`, `CleanupUserDataTask`, `CleanDatabaseScheduledTask`, `UserDataManager` |
|
||
| Migration Routines | 13 routines: `MigrateActivityLogDb`, `MigrateAuthenticationDb`, `MigrateDisplayPreferencesDb`, `MigrateKeyframeData`, `MigrateLibraryDb`, `MigrateLibraryUserData`, `MigrateRatingLevels`, `MoveExtractedFiles`, `RefreshCleanNames`, `RefreshInternalDateModified`, `FixDates`, `CleanMusicArtist`, (+ more) |
|
||
| Infrastructure | `JellyfinMigrationService`, `PostgresDatabaseProvider`, `DbContextFactoryHealthCheck`, `Program.cs`, `ServiceCollectionExtensions` |
|
||
|
||
The `JellyfinDbContext` itself defines **~35 DbSets** spanning 5 schemas (`activitylog`, `authentication`, `displaypreferences`, `library`, `users`).
|
||
|
||
---
|
||
|
||
## 2. Query Complexity — What EF Core Actually Does
|
||
|
||
### 2.1 BaseItemRepository.cs — The Core Case
|
||
|
||
`BaseItemRepository.cs` is **3,583 lines / 152 KB**, the largest file in the repository. It is the performance-critical code path for all library browsing, search, and playback-related queries. It is built entirely on EF Core LINQ.
|
||
|
||
**Key EF-dependent sub-systems inside it:**
|
||
|
||
| Method | EF Feature Used | Purpose |
|
||
|---|---|---|
|
||
| `TranslateQuery()` (lines 2548+, ~500 lines) | LINQ predicate composition over `IQueryable<BaseItemEntity>` | Translates `InternalItemsQuery` (60+ filter parameters) to SQL WHERE clauses |
|
||
| `ApplyOrder()` (~80 lines) | `OrderBy` / `ThenBy` over EF expression trees, `OrderMapper.MapOrderByField()` | Translates `ItemSortBy` enum to typed SQL ORDER BY |
|
||
| `ApplyNavigations()` | Conditional `Include()` / `ThenInclude()` | Eagerly loads TvExtras, AudioExtras, LiveTvExtras, Provider, Images, UserData per DTO field set |
|
||
| `ApplyGroupingFilter()` | `DistinctBy()` → PostgreSQL `DISTINCT ON` | Deduplication for presentation unique keys |
|
||
| `GetNextUpSeriesKeys()` | `Join()` + `GroupBy()` + `Max()` | Complex TV "next up" episode query |
|
||
| `GetItemValues()` | Nested `Join()` + `SelectMany()` + correlated subquery pattern | Artist/Genre/Studio item-value lookups |
|
||
| `UpdateOrInsertItemsAsync()` | `CreateExecutionStrategy().ExecuteAsync()`, `SaveChangesAsync()`, `Attach().State = EntityState.Modified`, `AddRange()` | Transactional batch upsert |
|
||
| Entity state management | `context.Entry(placeholder).State = EntityState.Detached` | Upsert conflict resolution |
|
||
| `EF.Constant()` | PostgreSQL-specific plan hint | Prevents parameterization of the placeholder UUID to allow index scans |
|
||
|
||
**LINQ operator counts in BaseItemRepository.cs** (approximate from code review):
|
||
- `Where()`, `Select()`, `Join()`, `GroupBy()`, `Include()`, `ThenInclude()`, `OrderBy()`: **200+ call sites**
|
||
- `ExecuteDelete()` / `ExecuteUpdate()` bulk ops: **~25 call sites**
|
||
- `FromSqlRaw` / `FromSqlInterpolated`: **0** — reads are pure LINQ
|
||
- `ExecuteSqlAsync()` raw SQL upserts: **~8** (ON CONFLICT upserts for extension tables)
|
||
|
||
The LINQ query composition is not superficial mapping sugar — `TranslateQuery` conditionally applies 30+ predicate branches (user data filters, rating filters, type filters, ancestor traversal, search term scoring, tag inheritance, etc.) based on `InternalItemsQuery` flags. This is the equivalent of a 500-line dynamic SQL builder, expressed in type-safe C#.
|
||
|
||
### 2.2 Other Repositories
|
||
|
||
The other repositories (`ActivityManager`, `UserManager`, `DeviceManager`, etc.) use EF more simply — primarily `Where()` + `Select()` + `SaveChanges()` patterns — but collectively represent hundreds more LINQ call sites.
|
||
|
||
### 2.3 Hybrid Usage Already Present
|
||
|
||
`BaseItemRepository.UpdateOrInsertItemsAsync()` already uses a hybrid approach: EF Core for reads and change-tracking, **raw `ExecuteSqlAsync()` for ON CONFLICT upserts**. This tells us the team is already reaching EF's limits on writes.
|
||
|
||
---
|
||
|
||
## 3. Migration Infrastructure
|
||
|
||
### 3.1 EF Schema Migrations Are DISABLED
|
||
|
||
Per `JellyfinMigrationService.cs` (line ~275):
|
||
```csharp
|
||
// DISABLED for .NET 11 preview: EF Core database migrations don't work with preview SDK
|
||
// Database schema is initialized from SQL scripts in PostgresDatabaseProvider instead
|
||
/*
|
||
if (stage is JellyfinMigrationStageTypes.CoreInitialisation)
|
||
{
|
||
pendingDatabaseMigrations = migrationsAssembly.Migrations...
|
||
}
|
||
*/
|
||
```
|
||
|
||
**EF's primary justification — schema migration management — is already bypassed.** Schema is initialized via `sql/schema_init/create_database_schema.sql` + `psql`.
|
||
|
||
### 3.2 EF Is Still Used for Code Migration Tracking
|
||
|
||
`JellyfinMigrationService` still uses EF to track *which code routines have run* via `__EFMigrationsHistory`:
|
||
|
||
```csharp
|
||
var historyRepository = dbContext.GetService<IHistoryRepository>();
|
||
await historyRepository.CreateIfNotExistsAsync();
|
||
var appliedMigrations = await dbContext.Database.GetAppliedMigrationsAsync();
|
||
await dbContext.Database.ExecuteSqlRawAsync(
|
||
historyRepository.GetInsertScript(new HistoryRow(migrationId, version)));
|
||
```
|
||
|
||
This usage of `IHistoryRepository` is what required the `SnakeCaseHistoryRepository` workaround (custom class overriding `MigrationIdColumnName` and `ProductVersionColumnName`, using `#pragma warning disable EF1001` because the base class is internal EF infrastructure).
|
||
|
||
---
|
||
|
||
## 4. Known Friction Points
|
||
|
||
These are the specific problems that prompted this analysis:
|
||
|
||
### 4.1 Snake_case Naming Impedance (FIXED, but fragile)
|
||
|
||
EF Core defaults to PascalCase. This fork uses snake_case PostgreSQL conventions. Three workarounds are required:
|
||
|
||
1. **`SnakeCaseNamingConvention`** — `IPropertyAddedConvention` that converts all property names to snake_case at model build time
|
||
2. **`SnakeCaseHistoryRepository`** — custom `NpgsqlHistoryRepository` subclass overriding column name properties; requires `#pragma warning disable EF1001` (breaking internal EF API)
|
||
3. **Manual table names in model configurations** — 31 `IEntityTypeConfiguration` files; two (`LibraryOptionsEntityConfiguration`, `HomeSectionConfiguration`) had PascalCase table names that overrode the convention and caused `relation "library.LibraryOptions" does not exist` errors at runtime
|
||
|
||
**Risk**: Every future upstream EF Core upgrade may break `SnakeCaseHistoryRepository` since it depends on internal EF infrastructure.
|
||
|
||
### 4.2 Execution Strategy Transaction Friction (FIXED, but imposed on all writers)
|
||
|
||
`EnableRetryOnFailure` in the PostgreSQL retry strategy makes EF Core incompatible with user-initiated `BeginTransaction()`. All transactional code must wrap in `context.Database.CreateExecutionStrategy().Execute(...)`. This has already been fixed in `MigrateRatingLevels` and `MigrateKeyframeData` but is an ongoing constraint.
|
||
|
||
### 4.3 EF-PostgreSQL Translation Gaps
|
||
|
||
Multiple code comments document EF Core translation failures:
|
||
|
||
- `DistinctBy()` on complex types sometimes cannot be translated → fallback to `ApplyGroupingInMemory()` using **reflection on anonymous types**
|
||
- `GetItemValues()` in some paths falls back to `.AsEnumerable()` for in-memory filtering
|
||
- `GetLatestItemList()` had to be split into two queries to avoid untranslatable nested `Min()` subqueries
|
||
- A documented 165s query regression was caused by EF generating correlated subqueries — fixed by switching to `DistinctBy()` → `DISTINCT ON`
|
||
- `AsSingleQuery()` required explicitly to prevent N+1 for multi-table includes
|
||
|
||
### 4.4 Inconsistent Table Naming in Raw SQL
|
||
|
||
`UpdateOrInsertItemsAsync()` uses raw `ExecuteSqlAsync()` with PascalCase quoted names:
|
||
|
||
```csharp
|
||
$@"INSERT INTO library.""BaseItemProviders"" (""ItemId"", ""ProviderId"", ""ProviderValue"")..."
|
||
$@"INSERT INTO library.""BaseItemTvExtras"" (""ItemId"", ..."
|
||
```
|
||
|
||
These PascalCase quoted names (`"BaseItemProviders"`, `"BaseItemTvExtras"`) are inconsistent with the snake_case goal of the fork. The actual table names in PostgreSQL must match these quoted strings exactly — implying the schema init SQL creates them with PascalCase quoted names, not the snake_case convention applied to EF properties.
|
||
|
||
### 4.5 Known Performance Anti-Pattern in `GetItemValues`
|
||
|
||
```csharp
|
||
// TODO: This is bad refactor!
|
||
itemCount = new ItemCounts() {
|
||
SeriesCount = itemCountQuery!.Count(f => f.Type == seriesTypeName),
|
||
EpisodeCount = itemCountQuery!.Count(f => f.Type == episodeTypeName),
|
||
// ... 5 more COUNT queries
|
||
}
|
||
```
|
||
|
||
This executes 7 separate `COUNT` queries per item in a result set — a classic N+1 problem embedded in an EF projection. This is an EF translation limitation (aggregate counts in a nested `SELECT` project).
|
||
|
||
---
|
||
|
||
## 5. Can EF Core Be Removed?
|
||
|
||
**Technically: Yes. Practically: The cost is prohibitive.**
|
||
|
||
A full removal would require:
|
||
|
||
1. **Rewriting `BaseItemRepository.cs`** (3,583 lines) as raw SQL — either Dapper or direct `NpgsqlCommand`. The `TranslateQuery` method alone is a ~500-line dynamic query builder that would need to be rewritten as a SQL string builder. All `Include()` chains would become explicit JOINs. All `ExecuteUpdate`/`ExecuteDelete` calls would become parameterized SQL.
|
||
|
||
2. **Replacing 31 ModelConfiguration files** with SQL schema definitions (partially done via `sql/schema_init/`).
|
||
|
||
3. **Replacing the migration tracking system** (`IHistoryRepository`) with a simple SQL table + custom tracker.
|
||
|
||
4. **Rewriting all other repositories** (~15 files) as Dapper or raw Npgsql.
|
||
|
||
5. **Replacing all `IDbContextFactory` injection points** (~35 files, 82 sites) with new connection/command factories.
|
||
|
||
**Estimated effort: 3–6 person-months of careful rewrite, plus extensive testing coverage.**
|
||
|
||
Alternatives assessed:
|
||
|
||
| Alternative | Pros | Cons |
|
||
|---|---|---|
|
||
| **Full removal (Dapper + Npgsql)** | Full control of SQL; no EF friction; better PostgreSQL-native features | 3-6 month rewrite; no dynamic LINQ query composition; lose `ExecuteUpdate`/`ExecuteDelete`; high regression risk |
|
||
| **Hybrid (EF reads, Dapper writes)** | Eliminates write friction; keeps LINQ query composition | Two persistence paradigms; still need EF for reads; does not eliminate snake_case friction |
|
||
| **Keep EF, address friction points** | Minimal disruption; preserves LINQ query composition | Ongoing EF+PostgreSQL compatibility work; preview SDK risk |
|
||
| **Replace only migration tracking** | Eliminates `SnakeCaseHistoryRepository` hack | Small gain; does not address execution strategy or translation friction |
|
||
|
||
---
|
||
|
||
## 6. Should EF Core Be Removed?
|
||
|
||
**No — but specific friction points should be addressed deliberately.**
|
||
|
||
### 6.1 Why EF Core Should Stay
|
||
|
||
**The LINQ query composition in `TranslateQuery` / `ApplyOrder` is EF Core's core contribution to this codebase.** It provides:
|
||
|
||
- **Type safety** across 60+ filter parameters in `InternalItemsQuery`
|
||
- **Composability** — predicates are built incrementally, each branch independently testable
|
||
- **Maintainability** — 500 lines of C# expression trees is significantly more maintainable than equivalent raw SQL string building
|
||
- **`ExecuteUpdate`/`ExecuteDelete`** — these EF 7+ bulk operations map directly to `UPDATE ... WHERE` and `DELETE ... WHERE` without loading entities; replacing these with raw SQL would produce functionally identical code with more boilerplate
|
||
- **Navigation property loading** (`Include`) — the conditional navigation loading in `ApplyNavigations` maps cleanly to the DTO field selection pattern used by the API layer
|
||
|
||
EF Core's value in this codebase is almost entirely in the **read path** (LINQ-to-SQL translation) and **bulk operations** (`ExecuteUpdate`/`ExecuteDelete`). Its weaknesses are in the write path and infrastructure (migration history, naming conventions, execution strategy).
|
||
|
||
### 6.2 Recommended Targeted Actions
|
||
|
||
In priority order:
|
||
|
||
#### Priority 1 — Replace Migration History Tracking (Low effort, high safety gain)
|
||
|
||
The `SnakeCaseHistoryRepository` workaround depends on EF internal APIs (`#pragma warning disable EF1001`). Replace the code migration tracking with a simple dedicated table and Dapper/Npgsql:
|
||
|
||
```sql
|
||
CREATE TABLE IF NOT EXISTS public.jellyfin_applied_migrations (
|
||
migration_id VARCHAR(150) PRIMARY KEY,
|
||
applied_at TIMESTAMPTZ NOT NULL DEFAULT now(),
|
||
product_version VARCHAR(32) NOT NULL
|
||
);
|
||
```
|
||
|
||
Remove `JellyfinMigrationService`'s dependency on `IHistoryRepository` and `GetAppliedMigrationsAsync()`. This eliminates the most fragile EF workaround.
|
||
|
||
#### Priority 2 — Fix Raw SQL Table Names to Match snake_case Schema (Low effort, correctness)
|
||
|
||
`UpdateOrInsertItemsAsync` uses `library."BaseItemProviders"`, `library."BaseItemTvExtras"`, etc. If the fork's goal is snake_case naming, verify whether these tables exist as quoted PascalCase or as snake_case in PostgreSQL, and make the raw SQL consistent. Quoted PascalCase in PostgreSQL is case-sensitive and is a footgun.
|
||
|
||
#### Priority 3 — Replace Write Path with Raw SQL/Dapper (Medium effort, eliminates execution strategy friction)
|
||
|
||
The `UpdateOrInsertItemsAsync` method is already a hybrid (raw SQL for ON CONFLICT upserts + EF for the surrounding logic). Converting the remaining EF write operations (`SaveChangesAsync`, `Attach().State = EntityState.Modified`, `AddRange()`) in this method to direct Npgsql/Dapper calls would:
|
||
|
||
- Eliminate the `CreateExecutionStrategy().Execute()` wrappers
|
||
- Enable direct PostgreSQL `ON CONFLICT DO UPDATE` throughout
|
||
- Remove the impedance between EF change tracking and PostgreSQL's native upsert semantics
|
||
|
||
**Do not extend this to reads** — the LINQ query composition is worth keeping.
|
||
|
||
#### Priority 4 — Fix the N+1 in `GetItemValues` (Medium effort, performance)
|
||
|
||
The 7 separate `COUNT` queries per item should be replaced with a single `GROUP BY type, COUNT(*)` query executed once and pivoted in memory. This is a correctness/performance issue independent of whether EF is removed.
|
||
|
||
---
|
||
|
||
## 7. Summary
|
||
|
||
| Question | Answer |
|
||
|---|---|
|
||
| **Can EF Core be removed?** | Yes, technically. Estimated 3–6 person-months of work. |
|
||
| **Should EF Core be removed?** | No. The LINQ query composition in `BaseItemRepository` is EF's core value and cost-prohibitive to replace. |
|
||
| **Is EF Core being used optimally?** | No. The write path, migration tracking, and naming convention workarounds are sources of ongoing friction. |
|
||
| **What should be done instead?** | Replace migration tracking with a native SQL table; convert the write path in `BaseItemRepository` to raw SQL; keep EF for the LINQ read path. |
|
||
| **Biggest ongoing risk?** | `SnakeCaseHistoryRepository` uses internal EF APIs (`EF1001`) that may break on EF Core version bumps. Replacing migration tracking eliminates this. |
|
||
| **Second biggest risk?** | EF Core + Npgsql preview packages (`11.0.0-preview.1`) on `net11.0`. Schema migrations are already disabled because of this. Pin to stable as soon as net11.0 releases. |
|