Make LTI http service take an LTIRegistration #6596
Conversation
Until now all call to LTIA API happen on the context of a launch so it made sense to always scope them to the registration of that launch. In preparation for start fetching course rosters, likely on a celery task, outside the context of a launch, take an explicit registration for LTIA calls.
| @@ -47,6 +50,7 @@ def read_result(self, grading_id) -> GradingResult: | |||
|
|
|||
| try: | |||
| response = self._ltia_service.request( | |||
| self._lti_registration, | |||
There was a problem hiding this comment.
Pass the registration again on existing ltia_service calls.
| @@ -10,6 +10,7 @@ | |||
|
|
|||
| from typing import TypedDict | |||
|
|
|||
| from lms.models import LTIRegistration | |||
There was a problem hiding this comment.
This code is not really used in the code base yet, from the beginning on the same files (where GH doesn't allow me to post a comment):
Service to talk to the Name and Roles LTIA API.
We only implement this now as a way to obtain the LTI Advantage Complete
certification and it's not used anywhere in the codebase yet.
| @@ -97,7 +107,6 @@ def _get_new_access_token(self, scopes: list[str]) -> JWTOAuth2Token: | |||
|
|
|||
| def factory(_context, request): | |||
| return LTIAHTTPService( | |||
| request.lti_user.application_instance.lti_registration, | |||
There was a problem hiding this comment.
In general I don't like the existing pattern of scoping services to one request, AI or similar.
They become only useful in one context when they could be more useful just passing more parameters to the relevant functions.
Here we are planning to call this from a celery task, for many different registrations.
There was a problem hiding this comment.
Agreed. Services scoped to the request tend to accidentally become stateful, and later refactors or dependencies between services end up producing accidental crossed data or even memory leaks.
With truly stateless services where the contextual information is passed via method arguments, we could just have a single persistent instance shared by all requests, as you mentioned above.
| @@ -97,7 +107,6 @@ def _get_new_access_token(self, scopes: list[str]) -> JWTOAuth2Token: | |||
|
|
|||
| def factory(_context, request): | |||
| return LTIAHTTPService( | |||
| request.lti_user.application_instance.lti_registration, | |||
There was a problem hiding this comment.
Agreed. Services scoped to the request tend to accidentally become stateful, and later refactors or dependencies between services end up producing accidental crossed data or even memory leaks.
With truly stateless services where the contextual information is passed via method arguments, we could just have a single persistent instance shared by all requests, as you mentioned above.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
For:
Until now all call to LTIA API happen on the context of a launch so it made sense to always scope them to the registration of that launch.
In preparation for start fetching course rosters, likely on a celery task, outside the context of a launch, take an explicit registration for LTIA calls.
Testing
This should be just a pure refactor, lets sanity check the changes: