Skip to content
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

Move userId in API from route to optional query parameter #11071

Closed

Conversation

nielsvanvelzen
Copy link
Member

This is a proof of concept, looking for feedback to see if this is the right way to fix this problem or if there are better methods.

Right now it changes 2 operations: GetUserViews and GetGroupingOptions, both part of UserViewsController. There are 37 more operations that will need a similar change. I've tested this with the unstable version of jellyfin-web and the old endpoints still work so this is not a breaking API change.

Changes

Move userId in API from route to optional query parameter by adding new endpoints (with new routes!!) that move the userId to a query parameter. The old endpoints are kept but made obsolete and hidden from the OpenAPI specification.

With this change it is no longer required to provide a user id in the API, together with #11024 and #11028. This makes is a lot easier to use our API/SDK's and develop clients, as you no longer need to keep track of the current user id to use the API.

Issues

Copy link

github-actions bot commented Feb 27, 2024

Changes in OpenAPI specification found. Expand to see details.

What's New


GET /UserViews

Get user views.

GET /UserViews/GroupingOptions

Get user view grouping options.

What's Deleted


GET /Users/{userId}/GroupingOptions

Get user view grouping options.

GET /Users/{userId}/Views

Get user views.

[FromRoute, Required] Guid userId,
[FromQuery] bool? includeExternalContent,
[FromQuery, ModelBinder(typeof(CommaDelimitedArrayModelBinder))] CollectionType?[] presetViews,
[FromQuery] bool includeHidden = false) => GetUserViews(userId, includeExternalContent, presetViews, includeHidden);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[FromQuery] bool includeHidden = false) => GetUserViews(userId, includeExternalContent, presetViews, includeHidden);
[FromQuery] bool includeHidden = false)
=> GetUserViews(userId, includeExternalContent, presetViews, includeHidden);

The only suggestion I have is to place the expression body on the next line- when I first read through this I thought it was magic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server is full of crazy linters that complain about all my formatting, so I auto-formatted using Rider.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like really, I don't want to start another tabs vs spaces like war but this is the only sane way to format it and somehow we decided to not do this.

91700002d0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't bring your kotlin into this 🥇

@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Mar 3, 2024
@jellyfin-bot
Copy link
Contributor

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@nielsvanvelzen nielsvanvelzen deleted the api-userid-optional3 branch March 3, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Merge conflicts should be resolved before a merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants