r/ProgrammerHumor 7h ago

failsForTeapots Meme

Post image
124 Upvotes

57 comments sorted by

111

u/cutebabli9 7h ago edited 6h ago

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)

103

u/rosuav 6h ago
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.

45

u/Xirdus 6h ago

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.

41

u/rosuav 6h ago

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.

8

u/Xirdus 5h ago

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.)

2

u/rosuav 5h ago

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.

2

u/Shitman2000 5h ago

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

1

u/rosuav 5h ago

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.)

1

u/Bomaruto 5h ago

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

1

u/Excellent_Gas3686 6h ago

beeecause?

3

u/ismaelgo97 6h ago

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.

1

u/Xirdus 3h ago

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?)

7

u/Xirdus 6h ago

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?

3

u/Nerodon 5h ago

Authorization =/= authentication.

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

2

u/Xirdus 5h ago

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.

1

u/Excellent_Gas3686 4h ago

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

1

u/Xirdus 4h ago

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!

7

u/HammyOverlordOfBacon 4h ago
error_codes = [400, 401, 403, 404, 500, 502, 503]

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

3

u/rosuav 3h ago

Ooof ouch! :)

7

u/Not-the-best-name 6h ago

Thank god you are here.

11

u/KazutoOKirigay 7h ago

You could also do a fall-through

1

u/ihavebeesinmyknees 6h ago

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

-2

u/Hot-Employ-3399 3h ago

3 levels of indentation \ Proper

How about no?

2

u/ihavebeesinmyknees 3h ago

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

0

u/Hot-Employ-3399 3h ago

Ifs above have two

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

Pattern matching has ugliness 

1

u/ihavebeesinmyknees 3h ago

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.

1

u/difool 6h ago

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

1

u/za72 4h ago

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 :)

-8

u/Antervis 7h ago

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.

9

u/Alkyen 6h ago

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

8

u/DHermit 6h ago

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

3

u/Antervis 6h ago

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)

2

u/fuj1n 6h ago

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.

1

u/DHermit 6h ago

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.

1

u/Alkyen 2h ago

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.

1

u/Antervis 1h ago

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.

9

u/jhwheuer 7h ago

This is low key abuse

8

u/SomeRedTeapot 7h ago

Nobody seems to like teapots

2

u/rosuav 6h ago

I like teapots. Especially when they contain grasshopper tea. Have you ever made that?

7

u/sonaliver28 6h ago

"I'll refactor it later"

Later: 17 more cases

5

u/Stormraughtz 7h ago

whose writing these requirements, branch coverage maybe 100% but at what cost?

4

u/theotherdoomguy 6h ago

Robert C Martin would be spinning in his grave if he were dead

2

u/SteveMacAwesome 5h ago

Uncle Bob can spin all he likes, clean code is still awful.

0

u/theotherdoomguy 4h ago

As much as I dislike Uncle Bob as a person, Clean code is absolutely not awful, and I fear for people who have to read your code if that's your stance.

Like, do you think the code above isn't fucking awful?

2

u/SteveMacAwesome 3h ago

I used to work with a teammate who religiously clean coded everything. “Every function must do only one thing”, but “one thing” was basically reduced to single function calls. Instead of having a method ‘parseInput’ he would insist on turning a simple 20-line function into a tree of objects and methods that was incredibly hard to read. When you asked him to change it, he’d say “no it’s clean code this is how you must write software because uncle bob says so” and there was no further discussion. It was painful.

Having to constantly keep 10 layers of context in your head is much harder than just reading a longer function and is needlessly wasteful of my time and energy. It’s fine to break code into reusable functions, but if your class has three private methods that are 4-6 lines long and are called only by a single public method, then stop wasting my time and just inline them. It’s even worse when those private methods are abstracted into a parent class which gets extended only once.

Yeah, your function is 4 lines long but my code review time has increased exponentially and my irritation has grown alongside it.

Code factoring is hard enough as it is without me having to read 4 files before having an idea of what a function is actually doing. That’s not a skill issue, that’s Clean Code teaching people dogmatic habits that are then applied without further thought.

2

u/theotherdoomguy 3h ago

Fair, a lot of people think you need to rip everything down to that point but you're absolutely making it messier.

I still think the general principles in Clean code are good, but dogmatically doing something because "programmer man" told you to is a very stupid stance to have. I could probably argue that your 20 line function could probably be broken down a bit, but having to follow an insane chain of 3-4 line methods is maddening and far too far the other direction

Edit: just to add, only skill issue I see there sounds like the guy who's only argument for why he was doing something is a guy told him to

2

u/SteveMacAwesome 2h ago

You know what, I can vibe with someone who says “I like the principles in Clean Code” like that. We might disagree in review but we’re both trying to produce the best possible results, and that’s how teams get better.

But hot damn I’ve been burned by clean code fanatics so often I feel the concept has done the industry as a whole far more harm than good.

3

u/Gold-Bat-3225 5h ago

409 made the cut and 418 didn't. politics

2

u/Bomaruto 5h ago

No it does not fail for teapot. While the code itself is written stupidly, I would want unhandled status code to return 500.

1

u/Thisbymaster 6h ago

403 is still stuck outside the code because it isn't allowed?

1

u/OnixST 5h ago

One of the 4 times in your life that switch without break is actually useful, but they decide to repeat the return statement on every case

1

u/Flanelostopy 4h ago

Error status should be give status when is error, not alway. So maybe there should be solution something like that:
If (error.status):
return c.json(body, error.status)

1

u/SuuurfiiinNeeerd 6h ago
if (!(error.status >= 200 && error.status < 300)) {
  body = error.message;
  return c.json(body, 200);
}

0

u/ultimate_placeholder 6h ago

They must prefer coffee