ContentsHandler return 404 rather than raise exc

[bloomsa]

this addresses #1328. This works based on my testing but I'm more than happy to make structural changes to clean the code up. I'm admittedly not very well versed in tornado. Thanks in advance for feedback :)

[bloomsa]

I see one of the test actions, build (windows latest 3.9), failed but I'm not sure it's related since the code change doesn't seem to touch the affected test case

[blink1073]

I see one of the test actions, build (windows latest 3.9), failed but I'm not sure it's related since the code change doesn't seem to touch the affected test case

That is a known flaky test.

[blink1073]

Changes look good! Can you please add a test in https://github.com/jupyter-server/jupyter_server/blob/main/tests/services/contents/test_manager.py that tries to get a file that doesn't exist and ensures that a 404 is raised?

[bloomsa]

Changes look good! Can you please add a test in https://github.com/jupyter-server/jupyter_server/blob/main/tests/services/contents/test_manager.py that tries to get a file that doesn't exist and ensures that a 404 is raised?

done in my latest commit. couple of questions though

• do you know what the "w" argument is on cm.get here? I'm kind of confused about it as "w" doesn't really make sense for the content argument typed as a bool (ref) is it maybe legacy from a previous iteration of the get method?
• I tweaked the structure of the assertion a bit compared to the previous two assertions in the test case because I couldn't get the with pytest.raises(HTTPError) as excinfo: to be satisfied. I don't really understand how the assertions above it pass okay - my understanding is that the except should catch the exception and not trickle it up to the with. Do you want me to reformat those other assertions too? (or am I doing something totally wrong here 😅 )

[blink1073]

Hey @bloomsa, I agree the w is a logic error currently. I also think the previous blocks probably suffered from the move from nose to pytest are incorrect as well. The previous one isn't actually testing anything. 😅

[bloomsa]

I'm working on fixing the test cases for windows (actually led me to a bug in the tests that I'll fix 👍 )
would you like me to fix these as part of this PR? I can do it no problem @blink1073

I agree the w is a logic error currently. I also think the previous blocks probably suffered from the move from nose to pytest are incorrect as well.

[blink1073]

Yes please!

[bloomsa]

when the "setup" was contained within the 'with' an exception is thrown when creating the file since allow_hidden is False. The fix was to allow hidden while creating the test file, and then disallow during the actual method test.

[Wh1isper]

It seems like it could be extracted into a function for clean error response like _finish_model: https://github.com/bloomsa/jupyter_server/blob/78394daec886b9b323b4801380569bd8ef966fc9/jupyter_server/services/contents/handlers.py#L86

I haven't figured out how to name this function yet though, FYI 😄

async def _finish_error(self, code, msg):
  self.set_status(code)
  self.write(msg)
  await self.finish()

[bloomsa]

good suggestion 👍

[Wh1isper]

I would suggest adding some comments here to explain why a web.HTTPError needs to be caught here, as it doesn't make any difference to the client

Mostly in #1328, we don't want a meaningless stack throw, which means that the situation here is fully under control.

[blink1073]

LGTM, thanks!

[blink1073]

@Wh1isper, I'll wait for you to take another look.

[Wh1isper]

@blink1073 LGTM, thx