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: Handle HTTP NO CONTENT status code (204) to prevent null reference exceptions in HttpBase.cs #188

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

L-Naej
Copy link
Contributor

@L-Naej L-Naej commented Oct 7, 2021

NO_CONTENT is not considered as being an error in the HTTP standard, and therefore previous code would try to parse body data even with a NO_CONTENT being returned, leading to a parsing exception.

This is because as NO_CONTENT is not an error, returning an empty Response Body with a 204 status code does not return false for UnityWebRequest.isHttpError which is used in Extensions.IsValidRequest() to know if an exception must be thrown or not.

Various backend frameworks use NO_CONTENT as a way to gracefully indicates that a specific resource is empty (for instance, if you try to get the list of items in a player's inventory and the inventory is empty).

This PR just checks for the status code before trying to parse the response body in HttpBase.cs.

It is a very lightweight fix and should not break projects using the library because it's due to how UnityWebRequest handles HTTP codes (UnityWebRequest.isHttpError won't return true for a 204 NO_CONTENT).

exceptions. NO_CONTENT is not considered as being an error, and
therefore previous code would try to parse body data even with a
NO_CONTENT being returned. Various backend frameworks use NO_CONTENT as
a way to gracefully indicates that a specific resource is empty (for
instance, if you try to get the list of items in a player's inventory
and the inventory is empty).
@@ -8,6 +8,8 @@ namespace Proyecto26
{
public static class HttpBase
{
public static int HTTP_NO_CONTENT = 204;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be a const instead of static. Might be good to scope it private instead of public too, since it doesn't seem like anything outside of HttpBase will need to know about it.

Copy link
Member

Choose a reason for hiding this comment

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

@L-Naej hello mate, thanks for your contribution! Please check the @StevenGarberg's comment and let us know! 👍

Copy link
Member

Choose a reason for hiding this comment

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

Let me change that from develop branch, thanks for your help @StevenGarberg! <3

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're welcome! Thank you for making such a great library @jdnichollsc. I use it in all of my Unity projects to talk to my web APIs.

Copy link
Member

Choose a reason for hiding this comment

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

@StevenGarberg do you want to be a maintainer of this lib? <3

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jdnichollsc Absolutely I do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry guys, was completely off from internet for personal reasons, thank you very much for handling this!
Indeed I don't know what I was thinking with these public static, silly mistake...

Copy link
Member

Choose a reason for hiding this comment

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

haha no problem, thanks for your help! <3

@jdnichollsc jdnichollsc merged commit 2771499 into proyecto26:develop Dec 9, 2021
@jdnichollsc jdnichollsc mentioned this pull request Dec 10, 2021
github-actions bot pushed a commit that referenced this pull request Dec 25, 2021
Fix: Handle HTTP NO CONTENT status code (204) to prevent null reference exceptions in HttpBase.cs
github-actions bot pushed a commit that referenced this pull request Dec 28, 2021
Fix: Handle HTTP NO CONTENT status code (204) to prevent null reference exceptions in HttpBase.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants