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

Exception with status code #17

Closed
caneva20 opened this issue May 9, 2018 · 15 comments
Closed

Exception with status code #17

caneva20 opened this issue May 9, 2018 · 15 comments

Comments

@caneva20
Copy link

caneva20 commented May 9, 2018

Hi, I've been using the package for a week now, it's pretty good actually but there's one thing that bothers me, Exception does not have a status code and/or the description from the server.
I've been looking into your code and found that you return the error from UnityWebRequest which does not have neither the status code or a description from the server, in case my server return a 400 (BadRequest) with a description why it has failed. Both of those thing can be gathered pretty ease since you are creating a RequestException and have a RequestHelper in your disposal, which has both of those things.
To make it a little more clear why it's a good "feature" I'll leave a snippet below.

RestClient.Get($"{URL}/test")
            .Then(response => {
                //Do something
                return RestClient.Post($"{URL}/token", model);
            }).Then(response => {
                //Here I can can use the status code and/or the "descrition" from server
                switch (response.statusCode) {
                    case 200:
                        //Success
                        break;
                    case 401:
                        //Unauthorized
                        break;
                    default:
                        //All the others
                        break;
                }
            }).Finally(() => {
                //Do something
            }).Catch(Debug.LogException);

If something goes wrong here (return RestClient.Post($"{URL}/token", model);) the second Then() won't be called, and that's fine, but catching the exception does not help me 'cause it does not say what really happened there

*EDIT

After posting this I realized that even this change won't be enough for some cases. Using my own scenario, when opening the app, the user should log in, he/she might type the wrong email/password, if that is the case, the server will return a 401 (Unauthorized) (or you may want a 400 (BadRequest)), none of those are really an error, at least not a error to be "catchched" (in the Catch()), i would like to have them in the Then(), my case 401: (in the switch) will never be called, since server returned errors are treated as generic errors.

The EDIT part are just some thoughts, I'm not trying to suggest anything, is just, I don't know, my case is "special", (is it?)

@jdnichollsc
Copy link
Member

Hi @caneva20, thanks for report this issue!

Check the validations that I have https://github.com/proyecto26/RestClient/blob/master/src/Proyecto26.RestClient/HttpActions/HttpBase.cs#L17

What do you think? What changes do you propose to improve the RequestException class?

Other option is change the access of the Unity Request to be public to get the info https://github.com/proyecto26/RestClient/blob/master/src/Proyecto26.RestClient/Utils/RequestHelper.cs#L67

RequestHelper requestToken;
RestClient.Get($"{URL}/test").Then(response => {
    //Do something
    requestToken = new RequestHelper { 
        url = $"{URL}/token"
    };
    return RestClient.Post(requestToken, model)
}).Then(response => {
    switch (response.statusCode) {
        case 200:
            break;
    }
}).Finally(() => {
//Do something
 }).Catch(error => {
    if (requestToken != null) {
        //requestToken.request => To get the info of the Unity request if it is public
    }
    Debug.LogException(error);
 })

I prefer to add more information to the RequestException, but let me know what you think
Best Regards, Nicholls

@caneva20
Copy link
Author

caneva20 commented May 9, 2018

Hi @jdnichollsc , I've just forked the project and I'm changing things here and there to make it work for me.

I haven't thought about adding more things to RequestException, but I agree that this is a great solution, and probably the best the way, what I've done now was just a quick fix, (I'm appending the code and desc the to error description, hehe).

One other change that I've made to this class was here https://github.com/proyecto26/RestClient/blob/master/src/Proyecto26.RestClient/HttpActions/HttpBase.cs#L17

Instead of
if (request.isDone && string.IsNullOrEmpty(request.error))
I'm using
if (request.isDone && !request.isNetworkError)

That way no changes need to be made to RequestException and only if there's a NetworkError an error will be returned.

BUT this may not desired by everyone, I don't know.

What you think?

@jdnichollsc
Copy link
Member

jdnichollsc commented May 11, 2018

Ok, what you think if we add a new property called IgnoreHttpException from RequestHelper class to ignore http exceptions?
Thanks @caneva20

@caneva20
Copy link
Author

Humm, I think that's is pretty great actually, and it will be fine for everyone.

@jdnichollsc
Copy link
Member

@caneva20 sounds great, let me publish a new version of the plugin this weekend
Thanks for your help!

@caneva20
Copy link
Author

@jdnichollsc I should thank you for the attention.

Thanks!

@jdnichollsc
Copy link
Member

Any comment from the unity store is really appreciated! 👍 https://assetstore.unity.com/packages/tools/network/rest-client-for-unity-102501

@jdnichollsc jdnichollsc added this to In Progress in Development May 15, 2018
@jdnichollsc
Copy link
Member

jdnichollsc commented May 15, 2018

@caneva20 please validate the changes before generating a new release e287f94

Thanks in advance 👍

@caneva20
Copy link
Author

Hi @jdnichollsc I've tested it with my server and it works fine, now I can have more control over each request and do what I need.

One thing that you may have not realized is that the demo does not work anymore, 'cause you renamed some variables, quickly you'll fix it, I'm saying just to let you know.

Thanks man! :)

@jdnichollsc
Copy link
Member

Hi @caneva20 :)
Ohh yes, let me fix the demo, review the code once again and publish a new release!
Thanks for the help

@jdnichollsc
Copy link
Member

@caneva20 thanks for your comments from the Unity store! 👍
A moment ago, I reverted all the changes from the master branch to create a pull request to compare with the previous version of the plugin, I'm going to add a property BodyString from the RequestHelper class to send your JSON if you want to use other tool to serialize the code, so let me do that change and improve the code, any comments are really apreciated

Pull request => #18

Regards, Nicholls

@jdnichollsc
Copy link
Member

Check my friend the files modified, thanks for your help and patience! 💯 https://github.com/proyecto26/RestClient/pull/18/files

@jdnichollsc jdnichollsc moved this from In Progress to Done in Development May 19, 2018
@jdnichollsc
Copy link
Member

@caneva20 any good comment from the Unity Asset Store would be really appreciated :)

@caneva20
Copy link
Author

caneva20 commented Oct 24, 2018

Hello @jdnichollsc, I've updated my comment there, I just don't know how to update the "on previous version 1.2.2" and since I'm already here I would like to suggest one more thing, but that time is pretty simple, as you might have noticed, I don't like JSONUtility and the .BodyString solved this problem, I would like to ask you to add a section to the README with others JSON Serializers, and how it would be used/implemented (I can send you my wrapper), that way will be easier to newcomers.

I can recommend two libs that work nicely with Unity:

But that is really up to you. :)

@jdnichollsc
Copy link
Member

ohh any pull request is welcome my friend!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants