Skip to content
This repository has been archived by the owner on Jan 15, 2023. It is now read-only.

How to return 404, 403 or any error code to renderer process ? #219

Closed
redplane opened this issue Jul 2, 2020 · 5 comments
Closed

How to return 404, 403 or any error code to renderer process ? #219

redplane opened this issue Jul 2, 2020 · 5 comments

Comments

@redplane
Copy link

redplane commented Jul 2, 2020

Hi,

I'm using chromely for my desktop app development.
Till now, everything seems to be OK.
However, when I try to return HttpStatusCode which is about Http errors, such as: 401, 403, 404, every time ajax is done, the response is always 200.

FYI, this is what I'm doing:

protected virtual async Task<ChromelyResponse> EstablishSerialPortConnectionAsync(ChromelyRequest request)
        {
            var model = JsonConvert.DeserializeObject<EstablishConnectionViewModel>(request.PostData.ToJson());
            var response = new ChromelyResponse();
            try
            {
                await _serialPortConnectionService.ConnectAsync(model);
            }
            catch (Exception exception)
            {
                response.Status = (int)HttpStatusCode.InternalServerError;

                if ((exception is ArgumentException))
                    response.StatusText = HttpResponseMessageCodes.InvalidSerialPortSettings;

                if (exception is IOException)
                    response.StatusText = HttpResponseMessageCodes.InvalidSerialPortState;

                if (exception is InvalidOperationException)
                    response.StatusText = HttpResponseMessageCodes.SerialPortAlreadyOccupied;
            }
           
            return response;
        }

Any tutorial about this please ?

@mattkol
Copy link
Member

mattkol commented Jul 2, 2020

@redplane this is a rather simple but somewhat complicated decision when you are trying to go with an approach. The question here is what should the default value be - OK (200) or some error code since most developers do not set this anyway? So what we have had from the beginning is to ignore that even if set by developer. So for simple processing it is either OK (200) or BadRequest (400).

response.Status = (int)System.Net.HttpStatusCode.OK;

Status = (int)System.Net.HttpStatusCode.BadRequest,

But I think it is high time we changed that. This will be revisited.

The way this has worked for other developers is to wrap the payload response itself as the Data.
Please see - #110 (comment)

private static ChromelyResponse Connect(ChromelyRequest request)
{
       var content = new SomeClass() {
	       Status = 404,
		StatusText = "Not Found",
		Data = "No Data"
       }
	return new ChromelyResponse(request.Id)
	{
		Data = content
	}
}

Thanks.

@redplane
Copy link
Author

redplane commented Jul 3, 2020

For now, I'm using your above workaround.
But I think it is necessary to return error http status code because many web application, developers handle the error using HttpStatusCode (at least the developers I know are doing that)

@mattkol mattkol closed this as completed Jul 3, 2020
@redplane
Copy link
Author

redplane commented Jul 3, 2020

Imo, this issue shouldnt be closed, because http status code is necessary for front end to handle exceptions

@mattkol
Copy link
Member

mattkol commented Jul 5, 2020

@redplane please read my comment closely.

But I think it is high time we changed that. This will be revisited.

You said the workaround works, right? The change is simple, but one needs to spend time to see what else will be impacted, so let us spend sometime to make that improvement. If you want to leave it opened until it is resolved, you can go ahead and do that.

Very soon I will create a master list of what will be included in next release. This will be part of it. You can check back to see when.

But no promises on timeline.

Thanks.

mattkol pushed a commit that referenced this issue Jul 5, 2020
@mattkol
Copy link
Member

mattkol commented Jul 5, 2020

@redplane this -760ef7c is the minimal change that I think is required, If you are able to help confirm, that it is all that is needed, that will be fine. But I am unable to re-confirm for now. If you cannot confirm it, I will revisit it later.

Thanks.

mattkol pushed a commit that referenced this issue Jul 5, 2020
Stelzi79 pushed a commit to Stelzi79/Chromely that referenced this issue Oct 27, 2020
Stelzi79 pushed a commit to Stelzi79/Chromely that referenced this issue Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants