Jellyfin: Move to ASP.NET Web API

[ZadenRB]

This issue is just meant to track the move of services to ASP.NET. Numbers in brackets are the number of routes each service has. If I missed any services that need to be migrated (or just formatting suggestions for this tracker), let me know!

Emby.Dlna/Api

• [x] DlnaServerService [19] (#2957)
• [x] DlnaService [6] (#2956)

Emby.Notifications/Api

• [x] NotificationsService [7] (#2876)

MediaBrowser.Api

• [x] BrandingService [3] (#3419)
• [x] ChannelService [5] (#2967)
• [x] ConfigurationService [6] (#2962)
• [x] DisplayPreferencesService [2] (#2926)
• [x] EnvironmentService [7] (#2976)
• [x] FilterService [2] (#3269)
• [x] ItemLookupService [12] (#3271)
• [x] ItemRefreshService [1] (#3333)
• [x] ItemUpdateService [3] (#3273)
• [x] LocalizationService [4] (#3274)
• [x] PackageService [4] (#2983)
• [x] PlaylistService [5] (#3275)
• [x] PluginService [8] (#3280)
• [x] SearchService [1] (#3226)
• [x] SuggestionsService [1] (#3382)
• [x] TvShowsService [4] (#3421)
• [x] UserService [14] (#3363)
• [x] VideosService [3] (#3399)
  /Attachments
• [x] AttachmentService [1] (#2931)
  /Devices
• [x] DeviceService [7] (#2933)
  /Images
• [x] ImageByNameService [6] (#2944)
• [x] ImageService [44] (#3385)
• [x] RemoteImageService [4] (#2946)
  /Library
• [x] LibraryService [25] (#2984)
• [x] LibraryStructureService [8] (#3321)
  /LiveTv
• [x] LiveTvService [39] (#3405)
  /Movies
• [x] CollectionService [3] (#3440)
• [x] MoviesService [1] (#3381)
• [x] TrailersService [1] (#3493)
  /Music
• [x] AlbumsService [2] (#3441)
• [x] InstantMixService [7] (#3453)
  /Playback
• [x] MediaInfoService [5] (#3462)
• [x] UniversalAudioService [4] (#3783)
  /Hls
• [x] DynamicHlsService [8] (#3775)
• [x] HlsSegmentService [5] (#3718)
• [x] VideoHlsService [1] (#3764)
  /Progressive
• [x] AudioService [4] (#3592)
• [x] VideoService [40] (#3691)
  /ScheduledTasks
• [x] ScheduledTaskService [5] (#2929)
  /Sessions
• [x] ApiKeyService [3] (#3286)
• [x] SessionService [16] (#3324)
  /Subtitles
• [x] SubtitleService [7] (#3288)
  /SyncPlay
  
  
  
  • [x] TimeSyncService [1] (#3564)
  
  
  • [x] SyncPlayService (#3564)
    
    /System
  
  
• [x] ActivityLogService [1] (#3281)
• [x] SystemService [10] (#3285)
  /UserLibrary
• [x] ArtistsService [3] (#3484)
• [x] GenresService [2] (#3504)
• [x] ItemsService [3] (#3493)
• [x] MusicGenresService [2] (#3507)
• [x] PersonsService [2] (#3487)
• [x] PlaystateService [9] (#3460)
• [x] StudiosService [2] (#3451)
• [x] UserLibraryService [10] ( #3449)
• [x] UserViewsService [2] (#3447)
• [x] YearsService [2] (#3433)

MediaBrowser.WebDashboard/Api

• [x] DashboardService [6] (#3393)

[ZadenRB]

This may be a helpful reference for those working on migrating services:
https://docs.microsoft.com/en-us/aspnet/core/mvc/controllers/routing?view=aspnetcore-3.1#http-verb-templates

[mark-monteiro]

I have been working on adding integration tests to our test suite in https://github.com/jellyfin/jellyfin/pull/2945.

I'd like to propose that before any API class can be migrated to the new ASP framework, it must first have some basic integration tests defined with the old framework so that we can be confident about not breaking compatibility when migrating. Or at least we can be more explicit about where compatibility is changing since the associated test will also have to be changed.

So the process to migrate an endpoint or class to the new framework would be done in two PRs:

1. Define integration test(s) for the old framework endpoint
2. Migrate endpoints to new framework and move the integration test from MediaBrowser.Api.Tests to Jellyfin.Api.Tests (making changes as necessary)

[mark-monteiro]

@joshuaboniface You mentioned yesterday we should create a feature branch for this task so we can start merging stuff even without all the other required infrastructure in place.

What do we need to do to get that feature branch created? There are a few PRs here that are probably ready to merge soon, but they'll need to be retargeted.

[crobibero]

What method should we aim for when creating the APIs? I saw discussion in a few of the PRs but we should get it straightened out here instead of fragmented.

[mark-monteiro]

What method should we aim for when creating the APIs? I saw discussion in a few of the PRs but we should get it straightened out here instead of fragmented.

Definitely a good idea! @ZadenRB @crobibero @fruhnow You all have open PRs regarding this now, perhaps you can provide some feedback on the conventions you'd like to formalize for the project or areas where you've found there isn't enough guidance. This is a new endeavor so there aren't really existing guidelines in the project. And perhaps once we've decided on some guidelines we can finalize them by adding them to jellyfin-docs :)

Some things I can think of:

• There shouldn't be a need to add general exception handlers to each method in the controller. There is a middleware defined to handle exceptions: https://github.com/jellyfin/jellyfin/pull/2952. Thanks @crobibero for this contribution!
• [ProducesResponseType(StatusCodes....)] should be used on endpoints that can return multiple response codes so they are documented correctly. If the method only returns a 200 response this doesn't do anything, but it might not be a bad idea to include this anyways for consistency.
• Use of IActionResult. To me this should only be used when necessary (i.e. multiple response codes are possible). In other cases where only a 200 response is possible I think the object should just be returned directly which returns a 200 response by convention. I could also see it being easier/clearer to just use this across the board for all endpoints so I would also be open to that convention if there is more support for that.
• As much as possible the migrated endpoints should match the existing endpoints. We need to be careful about breaking compatibility with clients so it would be better to put breaking changes in into a follow-up PR after the initial migration
• Defining basic integration tests for the existing endpoints would be nice to do before migrating so that we can validate that behavior hasn't changed when migrating. But this might not be as necessary if we are merging into a feature branch as suggested by @joshuaboniface

[crobibero]

Return Types reference: https://docs.microsoft.com/en-us/aspnet/core/web-api/action-return-types?view=aspnetcore-3.1

According to the docs a new return type was released that I missed.
I think we should document the return types, and use ActionResult<T>.

[HttpGet("{id}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public ActionResult<Product> GetById(int id)
{
    if (!_repository.TryGetProduct(id, out var product))
    {
        return NotFound();
    }

    return product;
}

For methods that don't have a return value:

/// <summary>
/// Get test.
/// </summary>
/// <returns>Value.</returns>
[HttpPost("DoSomething")]
[ProducesResponseType(StatusCodes.Status200OK)]
public ActionResult DoSomething()
{
    // snip  
    return Ok();
}

Functionally this is the same as:

/// <summary>
/// Get test.
/// </summary>
/// <returns>Value.</returns>
[HttpPost("DoSomething")]
[ProducesResponseType(StatusCodes.Status200OK)]
public void DoSomething()
{
    // snip  
}

but I prefer explicitly returning a status.

Also, I think we should add [Produces("application/json")] to BaseJellyfinApiController and override when needed. This helps generate an openapi doc that includes the correct Accepts header

[ZadenRB]

I definitely like the idea of having everything explicitly documented in the code as well. I think we should either go with

/// <summary>
/// Get test.
/// </summary>
/// <returns>Value.</returns>
[HttpPost("DoSomething")]
[ProducesResponseType(StatusCodes.Status200OK)]
public ActionResult DoSomething()
{
    // snip  
    return Ok();
}

where everything is documented inline, with both return Ok() and ProducesResponseType annotations even where not strictly necessary, or

/// <summary>
/// Get test.
/// </summary>
/// <returns>Value.</returns>
[HttpPost("DoSomething")]
public void DoSomething()
{
    // snip  
}

with no inline documentation unless necessary, leaving the work to the API generator. I prefer the former, for both consistency between endpoints (many endpoints will have other return paths and need to use the annotations & ActionResult<T> anyways) and so that for new contributors to the API it's immediately clear what each endpoint they're modifying does without pulling up the docs.

I'd also like to ask people's thoughts on cases where an endpoint directly returns the output of another method. I personally think that its best to use interfaces for the endpoint's return type IEnumerable<T> instead of List<T> to be more flexible with changes to the method being returned, but I'm not sure how that will affect the generation of the OpenAPI documentation.

I agree with adding a Produces annotation to BaseJellyfinApiController, since most(all?) endpoints are JSON anyways, and the more work that can be removed from individual controllers and generalized to all of them the better.

[crobibero]

Recommended way to return IEnumerable<T>: https://docs.microsoft.com/en-us/aspnet/core/web-api/action-return-types?view=aspnetcore-3.1#return-ienumerablet-or-iasyncenumerablet

[ZadenRB]

Defining basic integration tests for the existing endpoints would be nice to do before migrating so that we can validate that behavior hasn't changed when migrating. But this might not be as necessary if we are merging into a feature branch as suggested by @joshuaboniface

I'm in favor of integration tests whether merging into a feature branch or not. Increasing test coverage is always good, and even if we merge to a feature branch, that will _eventually_ be merged into master, and it will be easier to write those tests on a per-controller basis rather than doing it in bulk just before merging to master.

It would be great to have some resources or guidelines available on writing integration tests for Jellyfin if needed, so that contributors who have limited testing experience (such as myself) can still write those tests and work on the API migration.

[fruhnow]

I like the idea of relying on Annotations, be it in the baseclass for Content-Type header or the [ProducesResponseType(StatusCodes.Status200OK)]annotation to be clear with the possible returns.

Making integrationtests mandatory will make this project a lot more time consuming but is the right measure to make sure nothing will break and being able to maintain quality in the future.

When it comes to IActionResult vs ActionResult<T> i'd prefer ActionResult<T> because i like being able to simply return the corresponding object without the need to box it into a StatusCode object. Thats not really a strong opinion tho.

[mark-monteiro]

Okay, let me try to summarize the discussion so far! I can edit this comment with changes or new ideas so let me know if I missed anything.

• A new feature branch has been created for this task, api-migration. All PRs for this task should target this branch
• Endpoints should always return ActionResult or ActionResult<T>, which will make things more consistent and also more clear about the response codes being returned
• [ProducesResponseType(StatusCodes....)] annotation(s) should be added to each endpoint, even if it is only a 200 response, for consistency and clarity

• IAsyncEnumerable<T> or IEnumerable<T> should be preferred for return types instead of concrete collection types like List<T>. These should still be wrapped in an ActionResult<T> to maintain consistency with other endpoints. Note that it is necessary to use use return Ok(enumerable) instead of returning the object directly because implicit casting is not supported for interfaces.

  An exception is that if the controller method makes use of yield return to generate the enumerable, then it is fine to return the enumerable directly instead of wrapped with ActionResult<T>

• The new endpoints should match the authentication/authorization restrictions of the old endpoints. In general, each controller should have the [Authorize] attribute applied at the class level, with overrides on each method where necessary. The table below summarizes how to migrate the auth attributes.

  |Old Attribute|New Attribute|
  |--------------|----------------|
  |[Authenticated(Roles = "Admin")]|[Authorize(Policy = Policies.RequiresElevation)]|
  |[Authenticated(AllowBeforeStartupWizard = true)]|[Authorize(Policy = Policies.FirstTimeSetupOrElevated)]|
  |[Authenticated(EscapeParentalControl = true)]|Not Implemented Yet|
  |[Authenticated(AllowLocal = true)]|Not Implemented Yet|

• Integration tests should be added for each controller/endpoint. These should target the old endpoints/framework and be merged into master. Then an associated PR targeting the api-migration should be opened up that updates the integration tests to target the new API. In this way we can see exactly what has changed between the old and new API in terms of testing

Some advice on triple-slash XML comments for controller methods:

• There is no need to prefix the summary description with "Endpoint for", etc. Just describe what the method does.
  xml <summary>Get all notifications for the current user</summary>
• Use <response> tags to document what each response code means. This doesn't need to be a full sentence.
  xml <response code="404">User not found.</response>
• For the <returns> tag, when returning an ActionResult:
  xml <returns>An <see cref="OkResult"/> on success, or a <see cref="NotFoundResult"/> if the file could not be found.</returns>
  Or this when returning an ActionResult<T>:
  xml <returns>An <see cref="OkResult"/> containing the list of notifications.</returns>
  Or when returning an object directly:
  xml <returns>The list of notifications</returns>
  For asynchronous endpoints returning a Task<>, document the task being returned, but also include the details as specified in the examples above for the task result:
  xml <returns> A <see cref="Task" /> that represents the asynchronous operation to ________. The task result contains an <see cref="OkResult"/> indicating success, or .... </returns>
• The <remarks> tag should be used to add additional information about the endpoint above-and-beyond the basic functionality. This should supplement the <summary> description, but users should still be able to understand what the endpoint does from the <summary> description alone

Here is an example of a fully-documented endpoint:

```c#
///

/// Gets a user by id.
///

/// The id of the user to retrieve.
/// User found.
/// User not found.
/// An on success containing the user, or a if the user could not be found.
[HttpGet("User/{Id}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public ActionResult GetUser([FromRoute, BindRequired] Guid id)
{
var user = _userManager.GetUserById(id);
if (user == null)
{
return NotFound();
}

return Ok(user);

}

~~In addition, `[Produces("application/json")]` should be added to the API base class, so that it is the default. Maybe someone can whip up a quick pr for that :)~~ Handled in #2960

`#pragma` should not be added to new code, use `SupressMessage` instead:
```c#
[SuppressMessage("Microsoft.Performance", "CA1801:ReviewUnusedParameters", MessageId = "unusedParam1", Justification = "Imported from ServiceStack")]
[SuppressMessage("Microsoft.Performance", "CA1801:ReviewUnusedParameters", MessageId = "unusedParam2", Justification = "Imported from ServiceStack")]
public ActionResult Get(
    [FromQuery] bool unusedParam1,
    [FromQuery] string unusedParam2,
    [FromQuery] string usedParam)
{
    return Ok(usedParam);
}

[ZadenRB]

So it turns out that we may need concrete collection types if we want to use ActionResult<T>, according to the documentation of controller action return types:

C# doesn't support implicit cast operators on interfaces. Consequently, conversion of the interface to a concrete type is necessary to use ActionResult

We can achieve this either by just calling .ToList() on IEnumerables, or modifying methods which return an interface to instead return a concrete type. I'm in favor of just using ToList() since I don't want to cause problems by changing return types of methods that may be used elsewhere in the code. Based on those docs, the return types of our actual controller actions should remain as ActionResult<IEnumerable<T>>, we just have to convert whatever object we return to have a concrete type.

[crobibero]

Is it possible to make the project editable by us so we can claim endpoints?

[mark-monteiro]

Based on those docs, the return types of our actual controller actions should remain as ActionResult>, we just have to convert whatever object we return to have a concrete type.

Thanks @ZadenRB! I've added that as a tip in my summary comment.

Is it possible to make the project editable by us so we can claim endpoints?

Instead of this, how about creating a new issue to claim the endpoint? Then when starting on a new controller, you can check if someone has already made an issue for it.

[crobibero]

I propose we add [Authorize] to BaseJellyfinApiController, and explicitly [AllowAnonymous] on endpoints that don't require authorization.
Doesn't work with the FirstTimeSetupOrElevated policy.

Authentication Policies:
[Authenticated(Roles = "Admin")] -> [Authorize(Policy = Policies.RequiresElevation)] [Authenticated(AllowBeforeStartupWizard = true)] -> [Authorize(Policy = Policies.FirstTimeSetupOrElevated)] [Authenticated(EscapeParentalControl = true)] [Authenticated(AllowLocal = true)]

[crobibero]

Should we make a AuthorizedJellyfinApiController that requires authorization, or have Authorization on each controller?

[ZadenRB]

I think we should specify on a controller-by-controller basis, same as we're doing with response codes.

[mark-monteiro]

Doesn't work with the FirstTimeSetupOrElevated policy.

Authentication Policies:

[Authenticated(Roles = "Admin")] -> [Authorize(Policy = Policies.RequiresElevation)]
[Authenticated(AllowBeforeStartupWizard = true)] -> [Authorize(Policy = Policies.FirstTimeSetupOrElevated)]
[Authenticated(EscapeParentalControl = true)]
[Authenticated(AllowLocal = true)]

@crobibero Could you expand on this? I'm not super familiar with the authorization attributes. Why can't a default [Authorize] be overridden with [Authorize(Policy = Policies.FirstTimeSetupOrElevated)] where necessary?

[crobibero]

Authorization attributes are ANDed together, but the first time setup also requires no authorization

On Apr 23, 2020, at 6:27 PM, Mark Monteiro [e-mail] wrote:


Doesn't work with the FirstTimeSetupOrElevated policy.

Authentication Policies:

[Authenticated(Roles = "Admin")] -> [Authorize(Policy = Policies.RequiresElevation)]
[Authenticated(AllowBeforeStartupWizard = true)] -> [Authorize(Policy = Policies.FirstTimeSetupOrElevated)]
[Authenticated(EscapeParentalControl = true)]
[Authenticated(AllowLocal = true)]
@crobibero Could you expand on this? I'm not super familiar with the authorization attributes. Why can't a default [Authorize] be overridden with [Authorize(Policy = Policies.FirstTimeSetupOrElevated)] where necessary?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[mark-monteiro]

Authorization attributes are ANDed together, but the first time setup also requires no authorization

Ah okay, that makes sense. I wonder if it would be possible to create a custom authorization attribute to replace the FirstTimeSetupOrElevated policy, maybe something derived from [AllowAnonymous].

If that's not possible (or if it's not worth the effort to figure out), then I think adding [Authorize] on each controller makes the most sense to me.

@crobibero Is this a summary of how to migrate the authentication atributes? So old framework attribute -> new framework attribute? If yes, I can add it to my summary comment

[Authenticated(Roles = "Admin")] -> [Authorize(Policy = Policies.RequiresElevation)]
[Authenticated(AllowBeforeStartupWizard = true)] -> [Authorize(Policy = Policies.FirstTimeSetupOrElevated)]
[Authenticated(EscapeParentalControl = true)]
[Authenticated(AllowLocal = true)]

[crobibero]

@crobibero Is this a summary of how to migrate the authentication atributes? So old framework attribute -> new framework attribute? If yes, I can add it to my summary comment

[Authenticated(Roles = "Admin")] -> [Authorize(Policy = Policies.RequiresElevation)]
[Authenticated(AllowBeforeStartupWizard = true)] -> [Authorize(Policy = Policies.FirstTimeSetupOrElevated)]
[Authenticated(EscapeParentalControl = true)]
[Authenticated(AllowLocal = true)]

Yes, these are the current mappings. The two without mappings still have to be created

[mark-monteiro]

Yes, these are the current mappings. The two without mappings still have to be created

Okay, thanks for putting the summary together! I updated my main summary comment with those details

[crobibero]

identifies actions that don't entirely document their responses (warnings)

https://docs.microsoft.com/en-us/aspnet/core/web-api/advanced/analyzers?view=aspnetcore-3.1

[mark-monteiro]

identifies actions that don't entirely document their responses (warnings)

https://docs.microsoft.com/en-us/aspnet/core/web-api/advanced/analyzers?view=aspnetcore-3.1

Oh, great find! If you opened a PR for that I'm sure it would get approved/merged pretty quickly. I think you could target master with that one instead of the feature branch since it wouldn't have any breaking changes

[crobibero]

identifies actions that don't entirely document their responses (warnings)
https://docs.microsoft.com/en-us/aspnet/core/web-api/advanced/analyzers?view=aspnetcore-3.1

Oh, great find! If you opened a PR for that I'm sure it would get approved/merged pretty quickly. I think you could target master with that one instead of the feature branch since it wouldn't have any breaking changes

I can't seem to get it to actually generate warnings, not sure what that's about

[mark-monteiro]

I'd like to review and come to an agreement on how to write the XML comments for the controller methods. I did a quick scan of the open PRs and noticed some inconsistencies so I'd like to review here and agree on some standards that we can try to follow.

• I've seen some <summary> tags prepending "Endpoint for ..." to each method. This seems unnecessarily repetitive to me. So for example,
  <summary>Endpoint for getting a server's branding settings.</summary>
  would become
  <summary>Get the server's branding settings</summary>.
• I've seen lots of <return> tags that only say "Status.". None of these methods ever return a status code directly so this seems incorrect to me. IMO what would be correct is something like this when returning an ActionResult:
  xml <returns>An <see cref="OkResult"/> on success, or a <see cref="NotFoundResult"/> if the file could not be found.</returns>
  Or this when returning an ActionResult<T>:
  xml <returns>An <see cref="OkResult"/> containing the list of notifications.</returns>
  Or when returning an object directly:
  xml <returns>The list of notifications</returns>

Feel free to comment with additional thoughts or criticisms on those suggestions! Once it seems like we have a general agreement on this I will update my reference comment above again.

[crobibero]

@mark-monteiro- this change doesn't seem to be reflected in the openapi spec, so whatever standard you propose works for me.

One thing I did just learn is if you document an endpoint as such:
```c#
///

/// Gets user.
///

/// User Id.
/// User found and returned.
/// User not found.
/// An on success containing the user, or a if the user could not be found.
[HttpGet("User/{Id}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public ActionResult GetUserTest([FromRoute, BindRequired] Guid id)
{
var user = _userManager.GetUserById(id);
if (user == null)
{
return NotFound();
}

return user;

}
```

the swagger displays:

and redoc displays:

Adding these pieces of documentation will make the process longer but I feel that having descriptions tied to the response code will be beneficial in the long run. I'm flexible with what the description actually says, I just threw something in there quickly for a demo.

This was tested on my branch with the documentation changes (#2925), so it may be beneficial to merge that PR into the feature branch soon so we can test the documentation properly.

[crobibero]

We can also add
c# /// <remarks> These are the remarks. </remarks>
To add documentation before the parameters:

[ZadenRB]

I definitely like the addition of <response> documentation @crobibero. I think overall that example you gave is a good template to follow, using <remark>s if any further detail is needed for an endpoint.

[crobibero]

I definitely like the addition of <response> documentation @crobibero. I think overall that example you gave is a good template to follow, using <remark>s if any further detail is needed for an endpoint.

I agree, remarks should only be added if the endpoint needs additional documentation on usage. Most endpoints won't need it.

[mark-monteiro]

I like the use of the <response> and <remarks> tags as well! I've updated my reference comment with those suggestions as well as mine. I've also added the full code example from @crobibero as a reference.

Any comments/criticisms are welcome!

Really hoping to put some time aside soon™️ to start reviewing and merging these into the feature branch :D

[crobibero]

I think we should return IEnumerable<T> if the endpoint supports it, based on https://docs.microsoft.com/en-us/aspnet/core/web-api/action-return-types?view=aspnetcore-3.1#return-ienumerablet-or-iasyncenumerablet

[mark-monteiro]

I think we should return IEnumerable<T> if the endpoint supports it, based on https://docs.microsoft.com/en-us/aspnet/core/web-api/action-return-types?view=aspnetcore-3.1#return-ienumerablet-or-iasyncenumerablet

My takeaway from the docs is that there's no longer any downside to using IAsyncEnumerable in ASP.NET Core 3.0. Can you clarify why you think we should stick to IEnumerable? I would have thought we want to use the async version wherever possible

[crobibero]

I think we should return IEnumerable<T> if the endpoint supports it, based on https://docs.microsoft.com/en-us/aspnet/core/web-api/action-return-types?view=aspnetcore-3.1#return-ienumerablet-or-iasyncenumerablet

My takeaway from the docs is that there's no longer any downside to using IAsyncEnumerable in ASP.NET Core 3.0. Can you clarify why you think we should stick to IEnumerable? I would have thought we want to use the async version wherever possible

We should be using IAsyncEnumerable, but everything returns IEnumerable currently so we can't use it yet. I think this can be changed after the move to EF Core

[mark-monteiro]

We should be using IAsyncEnumerable, but everything returns IEnumerable currently so we can't use it yet. I think this can be changed after the move to EF Core

Ah yes okay, I understand. So for now, just return what the underlying service provides without making changes (which will be an IEnumerable in all cases), and we can convert to async later if necessary.

Was there anything regarding that you wanted me to update in my comment above?

[crobibero]

We should be using IAsyncEnumerable, but everything returns IEnumerable currently so we can't use it yet. I think this can be changed after the move to EF Core

Ah yes okay, I understand. So for now, just return what the underlying service provides without making changes (which will be an IEnumerable in all cases), and we can convert to async later if necessary.

Was there anything regarding that you wanted me to update in my comment above?

this bit here:

IAsyncEnumerable or IEnumerable should be preferred for return types instead of concrete collection types. Note that the implicit casting used by ActionResult is not supported for interfaces like IEnumerable so controller methods will need to make sure they call ToList() before returning. See the ASP documentation for more information about this.

If returning an enumerable object the value should not be ToList()ed

[ZadenRB]

The only situation we need ToList() is when an underlying service just returns IEnumerable, not a concrete type which implements it. Although in that case it may make sense to use ToListAsync() and return ActionResult<IEnumerableAsync>.

[crobibero]

The only situation we need ToList() is when an underlying service just returns IEnumerable, not a concrete type which implements it. Although in that case it may make sense to use ToListAsync() and return ActionResult<IEnumerableAsync>.

Can you give an example of what you're referring to?

[ZadenRB]

Yeah. Here in the NotificationsController: https://github.com/jellyfin/jellyfin/pull/2876/files#diff-2f187cd21b2926e9fbd91c43edbf570cR90
_notificationManager.GetNotificationServices() has the signature IEnumerable<NameIdPair> GetNotificationServices();. So we have to call ToList() as stated:

Note that the implicit casting used by ActionResult is not supported for interfaces like IEnumerable so controller methods will need to make sure they call ToList() before returning.

[crobibero]

with #2925 merged, the swagger document has been moved.
New locations:
OpenApi Spec:
{baseUrl}/api-docs/openapi.json

Swagger UI:
{baseUrl}/api-docs/swagger/index.html

Redoc UI:
{baseUrl}/api-docs/redoc/index.html

[crobibero]

If the openapi.json file returns:
The argument 'name' is null, empty or consists only of white-space. (Parameter 'name')
the <response> xml doc is missing the code attribute.

eg:
/// <response>This is the response.</response> -->
/// <response code="200">This is the response.</response>

[z3ntu]

Kind of unrelated question but does the current Jellyfin Docker image serve the swagger docs? Trying to access $myjellyfin/swagger/index.html results in "Unable to find the specified file.".

[crobibero]

I can access the current swagger from https://jellyfin.domain.tld/swagger/index.html
Do note that the old one does not obey base url settings.

[nielsvanvelzen]

Would it be possible to add the operationId property to the OpenAPI output? This is needed if we want to generate the apiclient code.

[crobibero]

Would it be possible to add the operationId property to the OpenAPI output? This is needed if we want to generate the apiclient code.

They should already be included, but if we want to override the generated operationId: https://github.com/domaindrivendev/Swashbuckle.AspNetCore#assign-explicit-operationids

[nielsvanvelzen]

They are not, only the old swagger spec (current stable) contains them. The api-migration branch didn't include them when I looked this morning.

[crobibero]

They are not, only the old swagger spec (current stable) contains them. The api-migration branch didn't include them when I looked this morning.

I should stop trying to read docs before fully waking up- missed the part "Swashbuckle omits the operationId by default."

I'll create a PR to add this in.

[nielsvanvelzen]

Some more issues with the OpenAPI output;

• The responses include 5 different mime types. I think this should only include application/json
• The schema's don't handle enums properly:
  json "NotificationLevel": { "enum": [ 0, 1, 2 ], "type": "integer", "format": "int32" },
  I think we want to use the names instead of integer values here.

[crobibero]

Some more issues with the OpenAPI output;

• The responses include 5 different mime types. I think this should only include application/json

• The schema's don't handle enums properly:

    "NotificationLevel": {
  
      "enum": [
  
        0,
  
        1,
  
        2
  
      ],
  
      "type": "integer",
  
      "format": "int32"
  
    },
  
  

  I think we want to use the names instead of integer values here.

I originally had the entire being returned as a string but wasn’t sure of the previous implementation.

Where are you seeing multiple response types?

[nielsvanvelzen]

The multiple response types are for pretty much all responses:

        "responses": {
          "200": {
            "description": "Notifications returned.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/NotificationResultDto"
                }
              },
              "application/json; profile=\"PascalCase\"": {
                "schema": {
                  "$ref": "#/components/schemas/NotificationResultDto"
                }
              },
              "application/json; profile=\"CamelCase\"": {
                "schema": {
                  "$ref": "#/components/schemas/NotificationResultDto"
                }
              },
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/NotificationResultDto"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/NotificationResultDto"
                }
              }
            }
          }

[crobibero]

Swashbuckle removed support for filtering the generated docs in favor of the built in Produces and Consumes attributes. As far as I can tell, removing text/plain and text/json require removing the built in SystemTextJsonOutputFormatter and StringOutputFormatter

I just tested with NSwagStudio, and the ApiClient was generated only using application/json

[nielsvanvelzen]

I noticed a lot of duplicate operation id's in the current OpenAPI output. According to the specs a OperationId should be unique across the whole document.

Duplicate operation id: GetAudioStream in HEAD /Audio/{itemId}/{stream}.{container}
Duplicate operation id: GetAudioStream in GET /Audio/{itemId}/stream
Duplicate operation id: GetAudioStream in HEAD /Audio/{itemId}/stream
Duplicate operation id: GetBrandingCss in GET /Branding/Css.css
Duplicate operation id: GetMasterHlsAudioPlaylist in HEAD /Audio/{itemId}/master.m3u8
Duplicate operation id: GetMasterHlsVideoPlaylist in HEAD /Videos/{itemId}/master.m3u8
Duplicate operation id: GetHlsAudioSegmentLegacy in GET /Audio/{itemId}/hls/{segmentId}/stream.mp3
Duplicate operation id: GetArtistImage in HEAD /Artists/{name}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetGenreImage in HEAD /Genres/{name}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetItemImage in HEAD /Items/{itemId}/Images/{imageType}
Duplicate operation id: GetItemImage in GET /Items/{itemId}/Images/{imageType}/{imageIndex}
Duplicate operation id: SetItemImage in POST /Items/{itemId}/Images/{imageType}/{imageIndex}
Duplicate operation id: DeleteItemImage in DELETE /Items/{itemId}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetItemImage in HEAD /Items/{itemId}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetItemImage2 in HEAD /Items/{itemId}/Images/{imageType}/{imageIndex}/{tag}/{format}/{maxWidth}/{maxHeight}/{percentPlayed}/{unplayedCount}
Duplicate operation id: GetMusicGenreImage in HEAD /MusicGenres/{name}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetPersonImage in HEAD /Persons/{name}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetStudioImage in HEAD /Studios/{name}/Images/{imageType}/{imageIndex}
Duplicate operation id: GetUserImage in HEAD /Users/{userId}/Images/{imageType}/{imageIndex}
Duplicate operation id: PostUserImage in POST /Users/{userId}/Images/{imageType}/{index}
Duplicate operation id: DeleteUserImage in DELETE /Users/{userId}/Images/{itemType}/{index}
Duplicate operation id: GetItems in GET /Users/{uId}/Items
Duplicate operation id: GetSimilarItems in GET /Artists/{itemId}/Similar
Duplicate operation id: GetSimilarItems in GET /Items/{itemId}/Similar
Duplicate operation id: PostUpdatedMovies in POST /Library/Movies/Updated
Duplicate operation id: PostUpdatedSeries in POST /Library/Series/Updated
Duplicate operation id: GetSimilarItems in GET /Movies/{itemId}/Similar
Duplicate operation id: GetSimilarItems in GET /Shows/{itemId}/Similar
Duplicate operation id: GetSimilarItems in GET /Trailers/{itemId}/Similar
Duplicate operation id: GetChannels in GET /LiveTv/Channels
Duplicate operation id: GetPrograms in POST /LiveTv/Programs
Duplicate operation id: GetFirstUser in GET /Startup/User
Duplicate operation id: GetSubtitle in GET /Videos/{itemId}/{mediaSourceId}/Subtitles/{index}/Stream.{format}
Duplicate operation id: Play in POST /SyncPlay/Play
Duplicate operation id: PingSystem in POST /System/Ping
Duplicate operation id: UpdateUser in POST /Users/{userId}
Duplicate operation id: GetVideoStream in HEAD /Videos/{itemId}/{stream}.{container}
Duplicate operation id: GetVideoStream in GET /Videos/{itemId}/stream
Duplicate operation id: GetVideoStream in HEAD /Videos/{itemId}/stream

_This log does not show the path of the first find for the shown operation ids_

I also found 2 operations with weird names:

Weird operation id: Post in POST /Items/{itemId}/Refresh
Weird operation id: Get in GET /Search/Hints

[crobibero]

I noticed a lot of duplicate operation id's in the current OpenAPI output. According to the specs a OperationId should be unique across the whole document.

Duplicate operation id: GetAudioStream in HEAD /Audio/{itemId}/{stream}.{container}

Duplicate operation id: GetAudioStream in GET /Audio/{itemId}/stream

Duplicate operation id: GetAudioStream in HEAD /Audio/{itemId}/stream

Duplicate operation id: GetBrandingCss in GET /Branding/Css.css

Duplicate operation id: GetMasterHlsAudioPlaylist in HEAD /Audio/{itemId}/master.m3u8

Duplicate operation id: GetMasterHlsVideoPlaylist in HEAD /Videos/{itemId}/master.m3u8

Duplicate operation id: GetHlsAudioSegmentLegacy in GET /Audio/{itemId}/hls/{segmentId}/stream.mp3

Duplicate operation id: GetArtistImage in HEAD /Artists/{name}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetGenreImage in HEAD /Genres/{name}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetItemImage in HEAD /Items/{itemId}/Images/{imageType}

Duplicate operation id: GetItemImage in GET /Items/{itemId}/Images/{imageType}/{imageIndex}

Duplicate operation id: SetItemImage in POST /Items/{itemId}/Images/{imageType}/{imageIndex}

Duplicate operation id: DeleteItemImage in DELETE /Items/{itemId}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetItemImage in HEAD /Items/{itemId}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetItemImage2 in HEAD /Items/{itemId}/Images/{imageType}/{imageIndex}/{tag}/{format}/{maxWidth}/{maxHeight}/{percentPlayed}/{unplayedCount}

Duplicate operation id: GetMusicGenreImage in HEAD /MusicGenres/{name}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetPersonImage in HEAD /Persons/{name}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetStudioImage in HEAD /Studios/{name}/Images/{imageType}/{imageIndex}

Duplicate operation id: GetUserImage in HEAD /Users/{userId}/Images/{imageType}/{imageIndex}

Duplicate operation id: PostUserImage in POST /Users/{userId}/Images/{imageType}/{index}

Duplicate operation id: DeleteUserImage in DELETE /Users/{userId}/Images/{itemType}/{index}

Duplicate operation id: GetItems in GET /Users/{uId}/Items

Duplicate operation id: GetSimilarItems in GET /Artists/{itemId}/Similar

Duplicate operation id: GetSimilarItems in GET /Items/{itemId}/Similar

Duplicate operation id: PostUpdatedMovies in POST /Library/Movies/Updated

Duplicate operation id: PostUpdatedSeries in POST /Library/Series/Updated

Duplicate operation id: GetSimilarItems in GET /Movies/{itemId}/Similar

Duplicate operation id: GetSimilarItems in GET /Shows/{itemId}/Similar

Duplicate operation id: GetSimilarItems in GET /Trailers/{itemId}/Similar

Duplicate operation id: GetChannels in GET /LiveTv/Channels

Duplicate operation id: GetPrograms in POST /LiveTv/Programs

Duplicate operation id: GetFirstUser in GET /Startup/User

Duplicate operation id: GetSubtitle in GET /Videos/{itemId}/{mediaSourceId}/Subtitles/{index}/Stream.{format}

Duplicate operation id: Play in POST /SyncPlay/Play

Duplicate operation id: PingSystem in POST /System/Ping

Duplicate operation id: UpdateUser in POST /Users/{userId}

Duplicate operation id: GetVideoStream in HEAD /Videos/{itemId}/{stream}.{container}

Duplicate operation id: GetVideoStream in GET /Videos/{itemId}/stream

Duplicate operation id: GetVideoStream in HEAD /Videos/{itemId}/stream

_This log does not show the path of the first find for the shown operation ids_

I also found 2 operations with weird names:

Weird operation id: Post in POST /Items/{itemId}/Refresh

Weird operation id: Get in GET /Search/Hints

After everything is merged in I'll go through and make sure the validator is happy