Fix 404 text content-type for JSON controllers

[MichalLytek]

Solving #231.

I decided to throw NotFoundError when undefined is returned from an action. This should let error handler decide about formatting the JSON response for JSON controllers and text/plain or html for text responses.

However I think we should place the info about the route where the undefined has been returned. @pleerock @NoNameProvided do you have any idea about what info and how formated the error message should be? 😕

[NoNameProvided]

I know it has been before, but yet this is so bad naming. I would expect it to be number only. We should do some refactoring later.

[NoNameProvided]

I have read the code only (no testing) but it seems ok to me.

However I think we should place the info about the route where the undefined has been returned.

I really think we should add debugger package and do logging with proper namespacing. Both Koa and express uses debugger as default logger so it wont add dependency overhead, but will make easier to understand to see what happening inside routing-controllers. Some ideas:

• adding routes / middlewares
• returning undefined
• errors

Others? I wanted to draft a PR for this quite a while.

[MichalLytek]

I was thinking about message Error property - currently we only have stack and name NotFoundError. We should send the info like Cannot ${action.method} on ${action.url}. or sth like this. Pure express final handler do sth like this 😉

[NoNameProvided]

I was thinking about message Error property - currently we only have stack and name NotFoundError.

I dont know how the default errors look like. The first thing was to swap them for custom errors and add an error middleware. 😄 But yes a message property is definitely needed.

[pleerock]

hmm why node: stable failed?

[MichalLytek]

@pleerock See #243

[pleerock]

Looks like you removed this code. Why?

[pleerock]

Okay looks like it moved to the bottom?

[MichalLytek]

In this place I only left checking undefinedResultCode. However now it's not about set http status code but I decided to not move it to the bottom handler to keep consistency with the place of throwing undefinedResultCode and nullResultCode in the koa driver.

[pleerock]

!==

[MichalLytek]

result != null is an equivalent for result !== null || result !== undefined
I should change the order of the checks, so the last else would be for send regular result

[pleerock]

yeah it also equivalent of result != 0 and others. Better if you do result !== null || result !== undefined if its want you want to do

[MichalLytek]

yeah it also equivalent of result != 0 and others

nope 😃

[pleerock]

see? I thought it is! I wanted users to prevent from confusion but was already confused by myself. Please change to explicit checks.

[MichalLytek]

I've already done it (9b70134) + changed the order of if-else tree for more understandable code 😉

[pleerock]

Im afraid of these changes TBH. You changed the most fragile part of code. Are you sure about your changes? Have you tested everything?

[MichalLytek]

@pleerock It's just a refactor of this part. The only change in behavior is throwing NotFoundError when response in undefined to let the error handler format the error info to JSON or text (or custom error handler). All tests are green, I've also added new test cases.

[MichalLytek]

But yes a message property is definitely needed.

@NoNameProvided
Do you have any proposal of the error message or we should implement the:
Cannot ${action.method.toUpperCase()} ${action.url}
like the finalhandler in express:
msg = 'Cannot ' + req.method + ' ' + encodeUrl(parseUrl.original(req).pathname)
? 😉

[NoNameProvided]

Do you ask about the 404 error? We should create error message to every error type.

I would go with some more general one like The requested resource doesn't exist.. I dont see any value adding the url and method to the response, as it is known to the user because he/she made it.

[MichalLytek]

We should create error message to every error type.

Do you mean the default error message for every bulit-in http error subclass? It could be nice 😉 However I always prefer custom messages like:

const playlistItem = await this.playlistItemRepository.findByIdAndOwner(itemId, user);
if (!playlistItem) {
    throw new NotFoundError(`Playlist item with ID '${itemId}' doesn't exist!`);
}

But the default message The requested resource doesn't exist. for 404 error is a good idea. So I think we should leave this PR as it is with no params in constructor - the error message would be the default one added later.

So, @NoNameProvided please review this PR, and we will wait for pleerock's acceptation as he is "afraid of these changes TBH" 😉

[NoNameProvided]

Do you mean the default error message for every bulit-in http error subclass?

Yes

So I think we should leave this PR as it is with no params in constructor - the error message would be the default one added later.

Yes, its a good idea. It's own my todolist to make error handling swappable / extendable in routing-controllers. I just dont have time to do it.

@NoNameProvided please review this PR,

With just reading the code it looks good to me.

[MichalLytek]

With just reading the code it looks good to me.

So please click that damn button! 😆

[MichalLytek]

@NoNameProvided looks like @pleerock is gone - if you are not afraid of this potentially dangerous changes, please merge this PR 😉

[fabiob]

It looks like this PR broke the ability to write directly to the response:

https://github.com/pleerock/routing-controllers#using-request-and-response-objects

[MichalLytek]

@fabiob
There are some problems with manually handling the response. You have to wait for this:
#117 (comment)

There's now a race between routing-controllers action result handling and user's action code. We will fix this soon.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.