Update service to use the new ext_lti_assignment_id

[marcospri]

• Added .exists() .get_for_canvas_launch() .merge_canvas_assignments()
• Get doesn't return None but raises NoResultFound

Updated PR from the last refactor.

[marcospri]

Args order changed. I think document_url makes sense as first or last argument but as I can't be mixed with other ones with default values and I don't think document_url=None makes sense I put it on front.

There's some changes on the launch views and test assertions because of this.

[seanh]

Two small suggestions: one tweak to an if and one to add a docstring. I'm gonna re-test it as well...

[seanh]

Suggested change

	if not resource_link_id:
	if ext_lti_assignment_id:

At this point since we've gotten past the two previous if's above I think we know that exactly one of resource_link_id or ext_lti_assignment_id is None. So I think if ext_lti_assignment_id is the same thing as if not resource_link_id but more direct

[marcospri]

Yes, it lines better with the method naming as well 👍

[seanh]

I think this method either returns a single assignment (which has either the matching resource_link_id or ext_lti_assignment_id or both) or two assignments (one with the matching resource_link_id and no ext_lti_assignment_id and the other with the matching ext_lti_assignment_id but no resource_link_id). Also the order_by() guarantees that when two assignments are returned the one with the resource_link_id comes first in the sequence. It'd be nice to document that here in get_for_canvas_launch(). Suggestion:

    def get_for_canvas_launch(
        self, tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id
    ):
        """
        Return the assignment(s) with resource_link_id or ext_lti_assignment_id.

        Return all the assignments in the DB that have the given tool_consumer_instance_guid and
        either the given resource_link_id or ext_lti_assignment_id or both. This could be:

        1. A single assignment in the DB that has either the given resource_link_id or
           ext_lti_assignment_id or both

        2. Or two assignments:

           i.  One with the matching resource_link_id and no ext_lti_assignment_id
           ii. And one with the matching ext_lti_assignment_id and no resource_link_id

           The assignment with the resource_link_id will always be first in the sequence.

        :rtype: sequence of either 1 or 2 models.Assignment objects

        :raise NoResultFound: if there are no matching assignments in the DB
        """

This is a bit duplicative because the canvas_db_configured_basic_lti_launch() view in #3127 also has some code comments and assertions about this. But I think it's probably based for get_for_canvas_launch() to have a docstring that fully explains its somewhat-complicated contract. The docstring is limited to just documenting what get_for_canvas_launch() returns whereas the comment in canvas_db_configured_basic_lti_launch() is more about why/how this happens and about merging the two assignments.

[seanh]

Mentioned in another comment above but I wonder if instead of:

assignments = (
    ...
).all()

if not assignments:
    return None

return assignments

This could just be:

return (
    ...
).all()

Then instead of:

:rtype: sequence of either 1 or 2 models.Assignment objects or None for not results.

the docstring would say:

:rtype: sequence of either 0, 1 or 2 models.Assignment objects

and I think it would make the calling code slightly simpler as list things like len() or iteration will always work even on an empty list.

[seanh]

I don't think the caching is working. Here's what I'm seeing when testing this PR (on its own, without the views PRs):

When launching a newly-created Canvas assignment for the first time (so there's no record of the assignment in the DB at all):

1. Pyramid calls the db_configured predicate which calls exists() which calls get(). get() raises NoResultFound and exists() returns False

2. Pyramid calls the configured predicate which calls the blackboard_copied predicate which calls the db_configured predicate again. Same thing happens: the db_configured predicate calls exists() which calls get() which raises NoResultFound and exists() returns False

3. This repeats several times with Pyramid calling predicates and predicates calling other predicates, with exists() and get() being called each time

4. The canvas_file_basic_lti_launch() view calls set_document_url() which calls get() which again raises NoResultFound

Basically I think the same DB query might be getting done several times over.

I suspect the reason might be that the @lru_cache on get() isn't working because get() is raising exceptions? I'm guessing that @lru_cache doesn't cache if you raise an exception.

Possible solutions:

• Put an @lru_cache on exists(). This'll reduce the number of calls to get() but it doesn't fully solve the problem as get() isn't always called by exists(), so I think get() will still be called more than once
• Put @lru_cache's on the three get_*() and _get_*() methods that contain the actual SQLAlchemy queries. But I think this would require changing these methods to return things instead of raising
• Change get() to return None instead of raising

[marcospri]

• Put an @lru_cache on exists(). This'll reduce the number of calls to get() but it doesn't fully solve the problem as get() isn't always called by exists(), so I think get() will still be called more than once
• Put @lru_cache's on the three get_*() and _get_*() methods that contain the actual SQLAlchemy queries. But I think this would require changing these methods to return things instead of raising
• Change get() to return None instead of raising

Thanks, well spotted.Yes lru_cache doesn't save invocations that raise which seems like a good decision.

I went for a bit of a mix of those solutions:

• Exists is now also cached. This fixes the immediate problem on the predictes usage of exists.
• Methods called from .get return None (changed .one() for .one_or_none(). This makes .get() cacheable on its own.
• Still kept the raise for the case when .get() finds duplicates. I don't think you wan't to just get None in that scenario and we are making that query repeatedly something else is wrong IMO.

[seanh]

I'm still seeing get() called twice when launching a Blackboard assignment.

I think the reason is that the predicates call exists() which calls get() like this:

lms/lms/services/assignment.py

Lines 90 to 92 in 84d3119

	self.get(
	tool_consumer_instance_guid, resource_link_id, ext_lti_assignment_id
	)

ext_lti_assignment_id is always None in the case of a Blackboard launch.

Then later the db_configured_basic_lti_launch() view calls get() like this:

lms/lms/views/basic_lti_launch.py

Lines 167 to 169 in 84d3119

	document_url = self.assignment_service.get(
	tool_consumer_instance_guid, resource_link_id
	).document_url

That's effectively the same call but the ext_lti_assignment_id (None) argument is missing so I'm guessing that @lru_cache considers the two calls to be different.

Fixable?

[marcospri]

Fixable?

I removed the optional values from get so now both calls are the same, we could still call .get with args vs kwargs and break the caching but that's true of any @lru_cache'd method.

edit: Cached the privated method instead, get might not get cached but the cache on those will prevent the hit to the DB which is we care about mostly.

I think the right answer here is to long term move to not doing queries on the predicates and do that logic on the view. Caching a value is strictly a side effect after all.

[seanh]

Tiny suggestion for a change to get_for_canvas_launch()'s contract.

I think get_for_canvas_launch() should have unit tests as well, but they could be added in a follow-up PR.

[seanh]

It might be possible for this to be slightly simpler if get_for_canvas_launch() would return an empty sequence instead of None:

Suggested change

	if assignments and len(assignments) > 1:
	if len(assignments) > 1:

http://thecodelesscode.com/case/6 :)

[marcospri]

Yep 👍 , I have had a bit of tunnel vision on these few last changes, changing the problematic line and not zooming out a bit.

[seanh]

Mentioned in another comment above but I wonder if instead of:

assignments = (
    ...
).all()

if not assignments:
    return None

return assignments

This could just be:

return (
    ...
).all()

Then instead of:

:rtype: sequence of either 1 or 2 models.Assignment objects or None for not results.

the docstring would say:

:rtype: sequence of either 0, 1 or 2 models.Assignment objects

and I think it would make the calling code slightly simpler as list things like len() or iteration will always work even on an empty list.

[seanh]

Since it's a public method with its own contract (and a pretty non-trivial contract) I think get_for_canvas_launch() should probably have its own direct unit tests as well. But we can add them in a follow-up PR