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.

Sign up for GitHub

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

Jump to bottom

SendMessageCommand should be using FromBody #5628

Closed
BrianCArnold opened this issue Mar 26, 2021 · 6 comments · Fixed by #5631 · May be fixed by jellyfin-archive/jellyfin-apiclient-csharp#4
Closed

SendMessageCommand should be using FromBody #5628

BrianCArnold opened this issue Mar 26, 2021 · 6 comments · Fixed by #5631 · May be fixed by jellyfin-archive/jellyfin-apiclient-csharp#4

Comments

@BrianCArnold
Copy link
Contributor

All parameters for the API Endpoint are marked as [FromQuery] as of 3/26/2021

Permalink to current SessionController implementation:

jellyfin/Jellyfin.Api/Controllers/SessionController.cs

Line 312 in 6946e58

public ActionResult SendMessageCommand(

The current implementation of sendMessageCommand in apiClient.js is passing the options in the POST body, rather than sending the parameters as query string parameters.

Note: I have a commit to resolve this issue, which I will include in a pull request to resolve this.
BrianCArnold referenced this issue in BrianCArnold/jellyfin-apiclient-javascript Mar 26, 2021
@BrianCArnold
Fix to resolve issue with sendMessageCommand
d8cdd36 
incorrectly sending parameters in body,
rather than sending them in query string
as required by the SessionController API
on the server.

Resolves: #173
@heyhippari
Copy link
Contributor

I raised this internally with the server team after seeing this issue, but in my opinion we should keep them as part of the POST body and adjust the server so it can get the parameters from both the body and the query (for back-compatibility).

It doesn't make much sense to POST to an endpoint with a query string rather than a body, especially with a message of varying length, which is bound to cause issues since URLs have a limited length.

@cvium cvium transferred this issue from jellyfin-archive/jellyfin-apiclient-javascript Mar 26, 2021
@cvium cvium changed the title apiClient.sendMessageCommand is sending options parameters as POST body rather than as query parameters SendMessageCommand should be using FromBody Mar 26, 2021
@BrianCArnold
Copy link
Contributor Author

I originally created the pull request for the javascript side because the server and kotlin api seemed to agree with each other and tried to fix it with the smallest change necessary.
I'll make linked pull requests for the SessionController.cs, Kotlin API, and check all other API projects to make sure they all POST the information in the body. Thanks.

@crobibero
Copy link
Member

Just as a heads up, the Kotlin sdk is auto-generated, so changes aren't required there.

@BrianCArnold
Copy link
Contributor Author

Ahh, thanks, that would explain why they match. Just about done with the API change.

There's a few unrelated minor edge case bugs in the SessionController I've noticed, is that something I should submit a separate pull request for, or would lumping them in with this be acceptable?

BrianCArnold added a commit to BrianCArnold/jellyfin that referenced this issue Mar 26, 2021
@BrianCArnold
Changed SessionController.SendMessageCommand
e9bb80f 
implementation receive data in the POST body, as that is how the
jellyfin-web client currently posts the data to the server.

Resolves: jellyfin#5628
This was referenced Mar 26, 2021
Merged
Open
@BrianCArnold
Copy link
Contributor Author

BrianCArnold commented Mar 26, 2021 •
edited
Loading

My normal workflow is in GitLab or Azure DevOps. If I need to add anything to these pull requests, or if I've forgotten something, please let me know.

@BrianCArnold
Copy link
Contributor Author

These pull requests should resolve the issue for the API and all clients that implement that call to the APIs

BrianCArnold added a commit to BrianCArnold/jellyfin that referenced this issue Mar 28, 2021
@BrianCArnold
Changed SessionController.SendMessageCommand
81e3e5c 
implementation receive data in the POST body, as that is how the
jellyfin-web client currently posts the data to the server.

Resolves: jellyfin#5628
@BrianCArnold BrianCArnold mentioned this issue Mar 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@heyhippari @crobibero @BrianCArnold