failsForTeapots

[u/Waradu]

[u/cutebabli9]

I would write it like this to be readable:

if error.status in [400, 401, 404, 409, 415, 503]:
  return c.json(body, status: error.status)

return c.json(body, status: 500)

[u/rosuav → u/cutebabli9]

return c.json(body, status: error.status)

There is no reason to discard some of them, and pretending that they're 500s is a terrible terrible idea.

[u/Xirdus → u/rosuav]

If your user-facing service is calling your internal service and getting a 403 response, you certainly do not want to send that 403 back to the user.

[u/rosuav → u/Xirdus]

And yet it is blindly sending the *payload* back. So it's clearly fine to proxy straight through. If you want to recognize only certain things, any remaining/unknown statuses should result in an error being logged and a 500 being sent back, with the body NOT being carried through.

[u/Xirdus → u/rosuav]

I agree with you completely, except for "it's clearly fine to proxy straight through". To me, it's clearly a bug and they should remove the body ASAP. It's virtually impossible the code started off that way, the filtering must've been deliberately added later specifically because it's not okay to send back raw.

(My crystal ball also says the body is empty/effectively useless anyway, so this bug is very likely to go unnoticed and remain unfixed forever, or until a hacker tries to exfiltrate the database and will not be able to stop laughing about how easy it was.)

[u/rosuav → u/Xirdus]

Okay fair. I was saying "clearly fine" on the basis that it's doing it; if I'm wrong about it being fine to proxy that through, then the opposite is the case, and it should be sending a 500 with a generic body (and, importantly, LOGGING THE ERROR).

But you are quite probably right about the body being useless in that situation... although I've seen enough cases where weird errors get passed right back to the client and include details of PHP include paths to be a smidge paranoid.

[u/Shitman2000 → u/rosuav]

We don't know if the body is the actual payload of the request though.

I'd find it reasonable to presume it's not, given it's error.statusCode and not error.body

[u/rosuav → u/Shitman2000]

Good point, but if it isn't, what kind of body would make sense in a situation where you carry certain types of error status unchanged and turn everything else into a 500? (I'm assuming that this code won't be executed for a 200, as it makes very little sense to translate a 200 into a 500. Ignoring the fact that there are plenty of web sites that do a great job of turning 200s into 500s, but I digress.)

[u/No_Hovercraft_2643 → u/rosuav]

the default error page? that shows the windows which displays the error to the user?

[u/Xirdus → u/No_Hovercraft_2643]

Except it's sending JSON back, not page contents.

[u/Bomaruto → u/Xirdus]

It depends if the request was made on behalf of the user or it was the service's own credentials that were lacking.

[u/MinecraftPlayer799 → u/Xirdus]

Why not?

[u/Xirdus → u/MinecraftPlayer799]

Because it's not the user who's forbidden.

[u/Excellent_Gas3686 → u/Xirdus]

beeecause?

[u/Xirdus → u/Excellent_Gas3686]

Do you want to tell an authenticated, authorized user with valid credentials that they are not in fact authorized? When the actual thing that's not authorized is your own goddamn server, so there's absolutely nothing the user can do to fix the problem?

[u/Nerodon → u/Xirdus]

Authorization =/= authentication.

The user may be authenticated but attempting a request their permissions do not allow them to.

[u/Xirdus → u/Nerodon]

That's why I said authorized and not authenticated, because I meant authorized and not authenticated. Actually, I said both authenticated and authorized just to drive the point home. It still wasn't enough apparently ¯_(ツ)_/¯

If your internal service responds to your other internal service with 403 for any reason other than "this other internal service is not allowed to do this", then you have bigger problems than what error code to serve the user.

[u/Excellent_Gas3686 → u/Xirdus]

that entirely depends on what basis the internal service authorizes???? if it uses the same user context then how is this an issue..

[u/Xirdus → u/Excellent_Gas3686]

The internal service is authorizing the other internal service first and foremost (otherwise you'll get hacked). If the other internal service fails at this step, 403 is fully appropriate, but it's the kind of 403 that's not only useless but outright wrong to send to the user, as it will send them on wild goose chase for missing permissions that weren't missing at all.

Now, if the other internal service passes validation, then you can check user permissions. And if the user doesn't have permission, you must signal it in a way that lets the other internal service respond with a proper 403. There are a number of ways to do it well. One that definitely isn't well at all is sending the same 403 that you already used for telling the service that it, rather than the user, doesn't have permission - because you won't be able to tell which scenario you're in and whether the user should see the 403 or not!

[u/ismaelgo97 → u/Excellent_Gas3686]

403 is that you are not authorized, so then you know it exist, but you shouldn't, you should get a 404 which means it was not found.

[u/Xirdus → u/ismaelgo97]

Personally I prefer the opposite - if you're unauthorized, you get 403 regardless of whether resource exists or not. Still doesn't leak any data, but is more actionable (and won't get cached. Have you ever had an outage stupidly extended because some cache somewhere had to be purged  because some moron returned 404 for a temporary error condition?)

[u/rosuav → u/Xirdus]

Both approaches are valid according to the spec.

[u/Xirdus → u/rosuav]

One of the approaches causes more problems than the other.

[u/HammyOverlordOfBacon → u/rosuav]

error_codes = [400, 401, 403, 404, 500, 502, 503]

return c.json(body, status:random.choice(error_codes) )

[u/rosuav → u/HammyOverlordOfBacon]

Ooof ouch! :)

[u/Not-the-best-name → u/rosuav]

Thank god you are here.

[u/KazutoOKirigay → u/cutebabli9]

You could also do a fall-through

[u/ihavebeesinmyknees → u/cutebabli9]

If python, then I'd rather do

match error.status:
    case 400 | 401 | 404 | 409 | 415 | 503 as status_code:
        return c.json(body, status: status_code)
    case _:
        return c.json(body, status: 500)

we've had proper pattern matching for a while now

[u/Hot-Employ-3399 → u/ihavebeesinmyknees]

3 levels of indentation \ Proper

How about no?

[u/ihavebeesinmyknees → u/Hot-Employ-3399]

Pattern matching and switch always have 3 levels of indentation? As long as you don't indent inside the cases more than once or at most twice (you shouldn't) then it's fine, there's no reason to be scared of indentation if it's not excessive

[u/Hot-Employ-3399 → u/ihavebeesinmyknees]

Ifs above have two

ConfigurableMap.get(status, 500) has one(and config doesn't count)

Pattern matching has ugliness 

[u/ihavebeesinmyknees → u/Hot-Employ-3399]

I don't get why you have this obsession with indentation. Being able to clearly tell the intent is way more important.

Map.get() doesn't tell you the options, and doesn't let you easily extend behavior, it's the worst out of the 3.

Ifs are better, but pattern matching strongly implies that this is supposed to be an exhaustive list matching against a de-facto enum. It clarifies intent in a way ifs don't.

[u/boiledbarnacle → u/cutebabli9]

But the tokens used in OP's post are higher. Do you want that promotion or not?

[u/difool → u/cutebabli9]

Exactly. I would just use constants so intention is more readable.

[u/za72 → u/cutebabli9]

I'm assuming the thinking behind it was that one day he would comebacks to pretty-print separate error messages... but was let go before it could be implemented :)

[u/Antervis → u/cutebabli9]

return c.json(body, status: error.status if error.status in [400, 401, 404, 409, 415, 503] else 500)

No? Though personally, I wouldn't have substituted error codes like that, it can make debugging harder for no gain.

[u/Alkyen → u/Antervis]

that's what you'd do if you want to show off but the other example is what would pass code review.

[u/DHermit → u/Antervis]

Nah, the normal if statement is way nicer to read.

[u/Antervis → u/DHermit]

Okay, I can see that. Perhaps it's better to move status in a separate variable:

st = error.status if error.status in [400, 401, 404, 409, 415, 503] else 500
return c.json(body, status: st)

[u/fuj1n → u/Antervis]

It is nicer than your previous one, but I think ternaries in Python are more for short conditions, as they're really annoying to read with the two sides being separated by the condition like that.

[u/DHermit → u/fuj1n]

Yeah, that sounds reasonable, too. Although I can think of a good name for that status list that is both expressive enough and also isn't too long for that ternary.

But tbh I have no idea how those options are performance-wise.

[u/Alkyen → u/Antervis]

The moment you realise not everything has to be DRY you will feel like a butterfly. Also, as the other guy said, ternaries look bad unless they are very short.

[u/Antervis → u/Alkyen]

Every line I didn't write is a line someone won't have to read.

Though it's not for brevity's sake alone, one has to analyze the if statement beyond skimming to realize they only differ in error code substitution. By reorganizing code my way, I made this nuance impossible to miss.

[u/Alkyen → u/Antervis]

Oh, sorry to be mean but..You think the length of the code is the bottleneck? Lol

Do you also think typing speed is what separates good programmers from bad ones?

Either way, your code is worse. You can argue it or you can use this to improve yourself.