dmitry-viskov / pylti1.3

LTI 1.3 Advantage Tool
MIT License
125 stars 68 forks source link

Add support for FastAPI (Starlette) #115

Closed orlovmyk closed 8 months ago

orlovmyk commented 1 year ago

As discussed in this PR https://github.com/dmitry-viskov/pylti1.3/pull/47

If someone is interested in this too, this .tgz gives high-level idea on how this can be achieved lti_advantage.tar.gz

FeldrinH commented 8 months ago

I'm also looking into adding async support but for aiohttp server instead of Starlette.

@dmitry-viskov You noted that the current implementation contains "non-async cache usage":

For now the current library contains some http calls (using requests library) + non-async cache usage so it can't be used in case of asynchronous views.

Could you clarify what you meant by non-async cache usage? I took a quick look at the pylti1.3 code and could not find any cache usage that would be incompatible with async.

dmitry-viskov commented 8 months ago

hi @FeldrinH.

Could you clarify what you meant by non-async cache usage? I took a quick look at the pylti1.3 code and could not find any cache usage that would be incompatible with async.

  1. This library uses requests library for HTTP-communications (it isn't async).

  2. This library originally was created for Django/Flask so it uses built-in caching mechanisms of this non-async frameworks. Example: https://github.com/dmitry-viskov/pylti1.3-flask-example/blob/master/game/app.py#L72

  3. I don't see any point in supporting async in this library. FastAPI is oriented primarily on creating REST APIs. LTI is very specific protocol that is based on templates/html/cookies and other things. All this is not about REST API. Also i believe that build projects using asyncio/etc specially for LTI - 100% bad idea.

  4. As i mentioned previously LTI as protocol couldn't be implemented separately from web framework (because it uses request/response objects, built-in mechanisms for redirects and etc). Current library provides some abstractions that could be used for implementing LTI using your own framework but anyway I don't want to support all this custom adapters in scope of this library. Current adapters for Django/Flask is enough. If you want to support asyncio or other things you may just create fork or create your own adapter as the separate project

FeldrinH commented 8 months ago

Thank you for the response!

I'm not sure I understand why using an async web framework/asyncio for LTI would be a bad idea, but since this library won't be getting new features in the foreseeable future, there probably isn't too much value in debating the merits of async here.