fix: Correct recursion error on load in JupyterHub

[mschroering]

Fixes #168

RequestHandler.current_user calls RequestHandler.get_current_user() when not set initially. This causes a recursive loop on load when running the extension in JupyterHub. Changed to use the login value which is being used for the other User fields.

[mschroering]

@JasonWeill @dlqqq This is my first PR in this project. Not sure if I need permissions to add reviewers and labels.

[3coins]

@mschroering
Thanks for working on this fix. I have added the label, will review shortly.

[dlqqq]

Hey @mschroering! Thank you for opening this PR! 🎉

I'm not sure if the recursion error is coming from our end; current_user is not a property we define, so self.current_user shouldn't invoke ChatHandler#get_current_user(). Otherwise, this would cause an error in all environments, not just JupyterHub. This seems to be an issue with JupyterHub's identity provider. Could you post the full stack trace here for us to investigate this a bit further?

My concern is that if calling self.current_user is causing JupyterHub to throw the RecursionError, then Jupyter AI won't work when collaborative mode is enabled. Look at the lines above:

if collaborative:
    return ChatUser(**asdict(self.current_user))

In the single-user scenario, using the shell login name for the username (which is more like a unique user ID) is perfectly fine, since any ID will work in the single-user case. My concern is that we may not be fixing the root issue.

[mschroering]

Yeah, current_user is defined on the base class RequestHandler. You can see it calls get_current_user which ChatHandler has overridden to invoke RequestHandler#current_user which starts the recursion loop.
https://github.com/tornadoweb/tornado/blob/stable/tornado/web.py#L1420

I included the full stack trace on the issue I reported: #168

And yeah, I think the same recursion error will happen with collaborative mode enabled. I did not test that.

[dlqqq]

@mschroering Ah, inheritance is really the gift that keeps on giving 😁. Thank you for digging that up. Seems like the real solution is to rename the method to something like get_chat_user() and document that it should never be named get_current_user(). Could you implement that?

[mschroering]

@dlqqq yes, I will work on getting those changes.

[mschroering]

@dlqqq I have made the updates.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️