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

Fix various content type issues on LIO request helpers #297

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Nov 13, 2024

As pointed out internally.

Mind you, all this seems to have absolutely no impact on anything whatsoever and be 100% academical, because as far as I can see, on local environment the web LIO endpoint just responds with 204 anyways, with or without this PR. I'd expect 415 if it didn't like anything above, so I'm wagering it just didn't care or check and all of this had an impact of exactly none whatsoever.

- `plain/text` is exactly backwards; it's actually `text/plain`.
- The MIME type is redundant anyways. `StringContent` implicitly means
  `text/plain` by default until specified otherwise.
- And the only place that used the `StringContent` path was wrong
  anyway, it was supposed to be doing
  `application/x-www-form-urlencoded`. Which `FormUrlEncodedContent` is
  for, and is now used in the relevant context.
@nanaya
Copy link

nanaya commented Nov 14, 2024

the parameters are ignored when they're invalid so it has been a noop.

also might as well just pass it as json and remove support for the other type...

@bdach
Copy link
Collaborator Author

bdach commented Nov 14, 2024

also might as well just pass it as json and remove support for the other type...

Sorry, I'm going to ask for clarification, as this is ambiguous to me. Are you saying that the lio endpoint in question supports json? And thus we can just send json in a similar shape as the form?

@nanaya
Copy link

nanaya commented Nov 14, 2024

apart of the oauth, I think they all support all parameter types (as long it's valid)

and fwiw the same endpoint has been used with json for reindexing deleted scores recently

@nanaya
Copy link

nanaya commented Nov 14, 2024

oh and while at it maybe don't customize the request content if the post object is null...

(either that or serialize an empty object)

@peppy
Copy link
Member

peppy commented Nov 14, 2024

@peppy peppy merged commit f1a3f2d into ppy:master Nov 15, 2024
3 checks passed
@bdach
Copy link
Collaborator Author

bdach commented Nov 15, 2024

I was gonna still address the points above, but I guess now that this is merged I... won't? Should have marked as blocked I guess 🤷

@bdach bdach deleted the fix-bad-mime-type branch November 15, 2024 06:47
@peppy
Copy link
Member

peppy commented Nov 15, 2024

Seemed like it would have been a follow-up change, oops.

@bdach
Copy link
Collaborator Author

bdach commented Nov 15, 2024

Alright, I'll do a round 2 then.

bdach added a commit to bdach/osu-queue-score-statistics that referenced this pull request Nov 18, 2024
Follow up from ppy#297

- Removed support for both `text/plain` and
  `application/x-www-form-urlencoded`, leaving only `application/json`
- If the request is made with a null payload object, do not set
  `Content-Type`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants