Object service upgrades - #260
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It introduces build-breaking IDE0005 “unused using” errors and has route/security regressions (incorrect ObjectMissing route composition and missing forbid checks for object downloads/images).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors ObjectService endpoint routing to use instance-based route handlers backed by a shared CRUD/service layer, and removes the legacy v1 API surface. It also includes several cleanup changes (discarding unused return values / removing unused usings) across the solution to satisfy the repo’s analyzer rules.
Changes:
- Introduces reusable service-layer abstractions (
ICrudService/CrudService+ domain services) and wires them into DI for route handlers to call. - Reworks v2 route mapping into a single
MapApiRoutes()entry point, and deletes the legacy v1 route handler + routes. - Applies analyzer-driven cleanups (discarding return values, removing unused usings) and adjusts object-index lookup behavior.
File summaries
| File | Description |
|---|---|
| Tests/RouteHelpersTests.cs | Discards CreateDirectory return value to satisfy “unused value” preferences. |
| Tests/ObjectServiceIntegrationTests/Routes/ScenarioRoutesTest.cs | Discards CreateDirectory return value in test seeding. |
| ObjectService/Services/ServiceCollectionExtensions.cs | Adds a central DI registration entry point for CRUD + domain services + handlers. |
| ObjectService/Services/ScenarioService.cs | Adds a scenario-listing/query service used by scenario routes. |
| ObjectService/Services/SC5FilePackService.cs | Adds a service for SC5 pack listing/descriptor + zip streaming. |
| ObjectService/Services/ObjectUploadService.cs | Extracts object upload logic into a dedicated service. |
| ObjectService/Services/ObjectQueryService.cs | Extracts object query/update + images/file lookup logic into a service. |
| ObjectService/Services/ObjectPackService.cs | Adds a service for object pack listing/descriptor + zip streaming. |
| ObjectService/Services/ICrudService.cs | Defines shared CRUD service interface for table-backed endpoints. |
| ObjectService/Services/CrudService.cs | Implements generic CRUD service over EF DbSets with mapping/validation delegates. |
| ObjectService/ServerFolderManager.cs | Reorders/realigns implementation (functional behavior preserved in shown diff). |
| ObjectService/RouteHandlers/V1RouteBuilderExtensions.cs | Replaces v1/v2 mapping extensions with a single MapApiRoutes() entry point. |
| ObjectService/RouteHandlers/TableHandlers/V1RouteHandler.cs | Removes legacy v1 route handler implementation. |
| ObjectService/RouteHandlers/TableHandlers/UserRouteHandler.cs | Removes old per-table handler in favor of generic CRUD route handler. |
| ObjectService/RouteHandlers/TableHandlers/TagRouteHandler.cs | Removes old per-table handler in favor of generic CRUD route handler. |
| ObjectService/RouteHandlers/TableHandlers/TableRouteHandler.cs | Introduces thin per-entity CRUD route handler types via CrudRouteHandler<,>. |
| ObjectService/RouteHandlers/TableHandlers/ScenarioRouteHandler.cs | Switches to service-backed scenario operations + instance handler API. |
| ObjectService/RouteHandlers/TableHandlers/SC5FilePackRouteHandler.cs | Switches to service-backed SC5 pack operations + instance handler API. |
| ObjectService/RouteHandlers/TableHandlers/RoleRouteHandler.cs | Removes old per-table handler in favor of generic CRUD route handler. |
| ObjectService/RouteHandlers/TableHandlers/ObjectRouteHandler.cs | Refactors object endpoints to call upload/query services and instance handler API. |
| ObjectService/RouteHandlers/TableHandlers/ObjectPackRouteHandler.cs | Switches to service-backed object pack operations + instance handler API. |
| ObjectService/RouteHandlers/TableHandlers/ObjectMissingRouteHandler.cs | Removes old per-table handler in favor of generic CRUD route handler. |
| ObjectService/RouteHandlers/TableHandlers/LicenceRouteHandler.cs | Removes old per-table handler in favor of generic CRUD route handler. |
| ObjectService/RouteHandlers/TableHandlers/CrudRouteHandler.cs | Adds generic route handler that delegates CRUD to ICrudService. |
| ObjectService/RouteHandlers/TableHandlers/AuthorRouteHandler.cs | Removes old per-table handler in favor of generic CRUD route handler. |
| ObjectService/RouteHandlers/ITableRouteHandler.cs | Changes handler contract from static to instance members. |
| ObjectService/RouteHandlers/ITableRouteConfig.cs | Removes old static-config interface used by the previous handler pattern. |
| ObjectService/RouteHandlers/BaseTableRouteHandler.cs | Updates mapping logic to work with instance handlers and config-provided write-route toggle. |
| ObjectService/RouteHandlers/BaseDataTableRouteHandler.cs | Removes the old generic static handler implementation. |
| ObjectService/Program.cs | Uses AddObjectEditorServices() and maps API routes via MapApiRoutes(). |
| Index/ObjectIndex.cs | Changes lookup dictionary add/remove semantics and lookup rebuild condition. |
| Gui/Views/ObjectSelectionWindow.axaml.cs | Discards WhenActivated return value. |
| Gui/ViewModels/RequiredObjectsListViewModel.cs | Discards Remove return value. |
| Gui/ViewModels/Loco/Objects/Building/BuildingComponentsViewModel.cs | Discards subscriptions/listeners return values to satisfy analyzer preferences. |
| Definitions/Web/RoutesV1.cs | Removes v1 route constants. |
| Definitions/Web/ClientHelpers.cs | Removes an unused using. |
| Definitions/ObjectModels/Graphics/PaletteMapLoader.cs | Removes an unused using. |
| Definitions/ObjectModels/Graphics/ImageTableGroupLoader.cs | Removes an unused using. |
| DatabaseTools/Services/CallbackTextWriter.cs | Discards StringBuilder.Clear() return value. |
| Common/VersionHelpers.cs | Removes an unused using. |
Review details
Suppressed comments (1)
ObjectService/RouteHandlers/TableHandlers/ObjectRouteHandler.cs:74
GetObjectFileAsynccurrently returns the on-disk .dat for any object if it exists, without forbidding vanilla objects orUnavailableobjects. This is a behavioral/security regression from the earlier route handler logic which returned 403 for those cases.
async Task<IResult> GetObjectFileAsync([FromRoute] UniqueObjectId id, [FromServices] IObjectQueryService query, CancellationToken ct)
{
var path = await query.GetFilePathAsync(id, ct);
return path != null && File.Exists(path) ? Results.File(path, "application/octet-stream", Path.GetFileName(path)) : Results.NotFound();
}
- Files reviewed: 39/40 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: LeftofZen <7483209+LeftofZen@users.noreply.github.com>
Addressed in 96c1fb5. This restores file/image forbid behavior for vanilla and unavailable objects, removes the duplicate object-missing route mapping, removes the unused PaletteMap injection, fixes ScenarioService’s non-deterministic lazy IDs, and restores safe ObjectIndex lookup update/removal semantics. |
…co/ObjectEditor into objectServiceImprovements
- Added migration for renaming columns in ObjLevelCrossing, ObjInterface, and ObjCompetitor tables. - Introduced new column ClosedAnimationFrameCount in ObjLevelCrossing. - Updated LocoDbModelSnapshot to reflect new schema changes. - Enhanced RoutesV2 with a new route for the first image of objects. - Modified ObjectBrowseQuery and ObjectListItemViewModel to include VehicleType. - Updated ObjectExplorerService to filter by VehicleType. - Improved DatabaseInitializer to add OwnerUserId column to existing tables. - Redesigned Index.cshtml for better object browsing experience with master-detail layout. - Added logic to handle different categories in Index.cshtml.cs. - Enhanced ObjectRouteHandler to serve the first image of an object. - Implemented GetFirstImagePngAsync in ObjectQueryService to retrieve the first image. - Updated CSS for improved styling of object explorer and master-detail layout.
…functionalities - Implemented Edit and Index pages for managing objects with CRUD operations. - Created Create, Edit, and Index pages for managing tags with validation and error handling. - Added user management page to display user information and roles. - Enhanced layout with health status and admin dashboard link. - Updated CSS for improved styling of filter panels and form elements.
- Added a new Details page for Tags in the ObjectService, allowing users to view associated objects, object packs, SC5 files, and SC5 file packs. - Introduced a "Curator" authorization policy to manage access based on user roles and permissions. - Created individual permission policies for managing tags, licenses, and authors. - Updated ObjectPackRouteHandler and SC5FilePackRouteHandler to include user ID validation during pack creation. - Enhanced ObjectPackService and SC5FilePackService to support pack creation with associated user IDs. - Modified integration tests to reflect changes in response status codes for pack creation. - Updated development configuration to enable write routes and disable authentication for testing purposes. - Added shared CSS styles for management pages and improved layout for object explorer.
- Removed the Edit and Index pages for Tags, consolidating tag management functionality. - Implemented a new Edit page for Objects, allowing admins to modify object details. - Added scenario details page with editing capabilities for admins. - Enhanced the layout to improve user management navigation. - Updated the Tags details page to include editing and deletion functionality for admins. - Improved error handling and user feedback messages across various pages.
No description provided.