Assignment service uses new ext_lti_assignment_id column

[marcospri]

This code should behave exactly as without any changes until further PRs are merged.

More context in #3127

Testing

• No behavior changes, launch and configure URL and canvas file assignments.

[marcospri]

We need this because there are already canvas file assignments in the DB which have a resource_link_id.

After this, re-configured assignments will be created with ext_lti_assignment_id creating a duplicate.

[marcospri]

bddtests expect this behavior, ie not raising here.

[seanh]

Hmm. I think we may have conflicting uses of AssignmentService.get(). In the LTI launch predicates the code seems to deliberately call AssignmentService.get() with just a resource_link_id that might be None and handles the fact that get() might return None by calling bool on it. The code does that here:

lms/lms/views/predicates/_lti_launch.py

Lines 34 to 42 in c86dfae

	assignment_svc = request.find_service(name="assignment")
	resource_link_id = request.params.get("resource_link_id")
	tool_consumer_instance_guid = request.params.get("tool_consumer_instance_guid")
	has_document_url = bool(
	assignment_svc.get_document_url(
	tool_consumer_instance_guid, resource_link_id
	)
	)

and again here:

lms/lms/views/predicates/_lti_launch.py

Lines 114 to 116 in c86dfae

	previous_document_url = assignment_service.get_document_url(
	tool_consumer_instance_guid, original_resource_link_id
	)

In these cases I don't think you want to log an exception since this appears to be normal, expected behaviour. So an exception in the logs would just be confusing. And logging an exception will report the exception to Sentry as well.

In other places where get() is called the code seems to assume that it will not return None. This happens in the Canvas Files API views:

lms/lms/views/api/canvas/files.py

Lines 37 to 47 in c86dfae

	assignment = self.request.find_service(name="assignment").get(
	application_instance.tool_consumer_instance_guid,
	self.request.matchdict["resource_link_id"],
	)
	public_url = self.canvas.public_url_for_file(
	assignment,
	self.request.matchdict["file_id"],
	self.request.matchdict["course_id"],
	check_in_course=self.request.lti_user.is_instructor,
	)

And it also happens in two places in the basic LTI launch views:

lms/lms/views/basic_lti_launch.py

Lines 165 to 168 in c86dfae

	document_url = self.assignment_service.get_document_url(
	tool_consumer_instance_guid, resource_link_id
	)
	return self.basic_lti_launch(document_url)

lms/lms/views/basic_lti_launch.py

Lines 209 to 217 in c86dfae

	document_url = self.assignment_service.get_document_url(
	tool_consumer_instance_guid, original_resource_link_id
	)
	self.assignment_service.set_document_url(
	tool_consumer_instance_guid, document_url, resource_link_id=resource_link_id
	)
	return self.basic_lti_launch(document_url)

I think these three are assuming that the return value is never None?

So there are two conflicting use cases. So I think you might want to split this into two different methods:

class AssignmentService:
    def get(self, ...):
        # This will raise an exception if both resource_link_id and ext_lti_assignment_id are None.
        # This will also raise an exception if there's no matching assignment.
        # I don't *think* we expect these exceptions to ever actually get raised so we can just
        # allow the app to crash with a 500.

    def exists(self, ...):
        # This one always returns either True or False, never raises.
        # Alternatively this could return an assignment or None. But I think True or False might suffice.
        try:
            self.get(*args, **kwargs)
        except ValueError:
            return False
        else:
            return True

[marcospri]

There are two issues here:

• Not passing any of the ids to .get(). This is not expected behavior I think, we should detect that early and issue a 400 or similar. The bddtest fail if I raise here but I think only because they are trying weird cases around the lti spec.

  Based on this I think we don't 4xx on those (missing resource_link_id) because we do some sort of lti redirect. https://github.com/hypothesis/lms/blob/onedrive-auth-page/lms/validation/_lti_launch_params.py#L52

• Not checking for none on get. The views on basic_lti_launch assume there's a document and don't check for None because they come from the db_configured predicate that checks exactly that (

  lms/lms/views/basic_lti_launch.py

  Line 160 in c86dfae

  # The ``db_configured=True`` view predicate ensures that this view

  )

  Then use on the predicate itself, here get_document_url is checking two things, that there's a row in the db and that it has a document. Refactoring it with exists() will mean doing a query to check if there's an assignment and another to get it and check the document. We could return the assignment on exists then but then we really wound't have an use for the get, will be just renaming it and between get -> Assignment and exist -> Assignment I prefer the first one.

[seanh]

Yeah it's doing a redirect instead of 404ing in order to be compliant with the LTI spec. It was necessary in order to get LTI certification.

I think the code that's calling get(resource_link_id=None) is doing so deliberately. It looks to me like it's doing resource_link_id = request.params.get("resource_link_id") instead of just request.params["resource_link_id"] deliberately so that resource_link_id will be None if it's not in the request.params.

Hmm. Ok, so I think it's actually get_document_url() rather than get() that has two different use-cases?

1. The predicates are calling get_document_url(), possibly passing it resource_link_id=None, or just a resource_link_id that doesn't exist in the DB, and they expect it to return a document URL or None. In this case you don't want to raise if resource_link_id is None or if there's no matching assignment, you just want to return None.

2. The views are calling get_document_url() and assuming (because the predicates must have already run) that it's going to return a document URL and not None. In this case you could raise if resource_link_id is None or if there's no matching assignment, because this should never happen.

So you might want separate get_document_url() and has_document_url() methods?

class AssignmentService:
    @lru_cache
    def get(self, ...):
        # Raise if both resource_link_id and ext_lti_assignment_id are None.
        # Raise if there's no matching assignment.

    def get_document_url(self, ...):
        # Calls get() so it will raise if both resource_link_id and ext_lti_assignment_id are None
        # or if there's no matching assignment.
        # Also raise if there is a matching assignment but it has no document_url.

    def has_document_url(self, *args, **kwargs):
        try:
            self.get_document_url(*args, **kwargs)
        except WhateverError:
            return False
        else:
            return True

Note that these won't result in multiple DB queries because both get_document_url() and has_document_url() work by calling get() which has @lru_cache on it.

The purpose of all this is to allow get() and get_document_url() to raise if both resource_link_id and ext_lti_assignment_id are None or if there's no matching assignment, because we never expect that to happen. But in the predicates, where we need it to not raise, those can call has_document_url() instead.

I don't think get() can do log.exception() as you have it currently because I think that will report exceptions to Sentry during expected behaviour, the edge cases that those BDD tests are covering.

[seanh]

The point of all this (in case it's not clear) is to allow the get() method to look something like this:

    @lru_cache
    def get(self, tool_consumer_instance_guid, resource_link_id=None, ext_lti_assignment_id=None):
        assert resource_link_id or ext_lti_assignment_id, "Can't get an assignment without a resource_link_id or ext_lti_assignment_id"

        if resource_link_id and ext_lti_assignment_id:
            return self._get_for_canvas_launch(resource_link_id, ext_lti_assignment_id, ...)

        if resource_link_id:
            return self._get_by_resource_link_id(resource_link_id, ...)

        return self._get_by_ext_lti_assignment(ext_lti_assignment_id, ...)

You might raise something else rather than an assert. But we can make it crash so that we'll get notified if anything is calling get() or get_document_url() without a resource_link_id or an ext_lti_assignment_id. At the same time the predicates can use the has_document_url() method to avoid any crash

[marcospri]

This conditional and the query are a bit verbose but trying to be clever here makes it a bit opaque.

[marcospri]

.get look less daunting now although all the code is still here (._get_canvas and _merge_canvas_assignments). I think is a good solution without a more deep refactoring.

[seanh]

Would you mind trying to split up the get() method some more?

I think your _get_canvas() is currently dealing with three separate cases (not resource_link_id and ext_lti_assignment_id , resource_link_id and ext_lti_assignment_id, and when both are None) and then the top-level get() deals directly with the remaining case (resource_link_id and not ext_lti_assignment_id). I think it might split things up more if get() began by directing each one of the three different cases to a different method.

I think we might want to consider making _merge_canvas_assignments() a public method as well. Partially just to further split up the code into smaller pieces. And partially because I'm not sure that a get() method should have a side-effects like merging things in the DB. I'm wondering under exactly what circumstances _merge_canvas_assignments() actually happens? But I think it might be better if the service method actually returns two assignments and the view detects that and calls merge_canvas_assignments().

[marcospri]

Would you mind trying to split up the get() method some more?

I went a bit on that direction and also moved the private methods to the bottom of the file.

I think your _get_canvas() is currently dealing with three separate cases (not resource_link_id and ext_lti_assignment_id , resource_link_id and ext_lti_assignment_id, and when both are None) and then the top-level get() deals directly with the remaining case (resource_link_id and not ext_lti_assignment_id). I think it might split things up more if get() began by directing each one of the three different cases to a different method.

Yes, I split the public get based on what you are trying to do as a caller and expanded a bit more the comments on what you might find on the db on the canvas method.

I think we might want to consider making _merge_canvas_assignments() a public method as well. Partially just to further split up the code into smaller pieces. And partially because I'm not sure that a get() method should have a side-effects like merging things in the DB. I'm wondering under exactly what circumstances _merge_canvas_assignments() actually happens? But I think it might be better if the service method actually returns two assignments and the view detects that and calls merge_canvas_assignments().

I don't think having side effects in the get is good but I don't having it return more than one row is better. Get doesn't get called directly, only from set_document/get_document so it happens directly on the predicate (even if it appears to happen on the view).

I tried a new is_canvas predicate and duplicated the db_configured view to have a canvas specific one at that point we could use either a separate service or different methods on the same service but would also have to do the same thing on the db_configure predicate and I didn't like the end result.

For me the fact that get hides theses complexity trumps both the ugliness of the side effect query method and mixing canvas an non canvas logic in the service. I could see some clever service dispatch (eg the factory returning one instance of the service or another depending on from which LMS you are calling) could work without having to do a lot if if is_canvas all over the place.

[seanh]

I wonder if the query can be simplified using in_()? I think it might be something like:

assignments = self._db.query(Assignment).filter(
    Assignment.tool_consumer_instance_guid == tool_consumer_instance_guid,
    Assignment.resource_link_id.in_([resource_link_id, None]),
    Assignment.ext_lti_assignment_id.in_([ext_lti_assignment_id, None])
).all()

I think we can omit the explanatory comments from the query and put them in the Python code that follows it instead, as the Python code is where you can see how each case is handled.

[marcospri]

The in behaves weirdly with null as anything that compares against it (select 1 where null = null;) so I think we need to use is null (sqlalcehmy converts x == null to x is null so that will work too) explicitly.

I did remove the comments and removed the intermediate query variable.

[seanh]

Ok, so I think assignments can only be empty, length 1, or length 2. It's not possible to have three matching assignments. For example you couldn't have this:

1. A row with both the resource_link_id and the ext_lti_assignment_id
2. A row with the matching resource_link_id and no ext_lti_assignment_id
3. A row with the matching ext_lti_assignment_id but no resource_link_id

Because there are unique constraints on both resource_link_id and ext_lti_assignment_id, so 1 and 2 can't exist at the same time and 1 and 3 also can't exist at the same time. You either get none of them, 1 alone, 2 alone, 3 alone, or 2 and 3 together.

[seanh]

Here you're implicitly assuming that one of the assignments has a resource_link_id and no ext_lti_assignment_id and the other has an ext_lti_assignment_id and no resource_link_id. I wonder if it might be nice to make that assumption explicit? Both because it might make the code more readable to explicitly state the assumption, and because it'll mean that we get notified if something's wrong.

This code also looks like it could assign the same object to old_assignment and assignment in cases where the above assumption has failed. It might be good to just make it so that it can't possibly assign the two to the same object. Again both for readability and safety.

Maybe something like:

if assignments[0].resource_link_id:
    old_assignment, new_assignment = assignments
else:
    new_assignment, old_assignment = assignments

assert not old_assignment.ext_lti_assignment_id
assert not new_assignment.resource_link_id

I think assertions might actually be fine and appropriate for this but could raise some other exception instead if you prefer.

[marcospri]

Yes good point, much more readable. Assertions are 👍

edit: I added an order by clause to the query to avoid this (and having to add a no cover or a test which forces the rows to come in a different order). Kept the assertions as a sanity check.

[seanh]

Is the flush really necessary here?

[marcospri]

Yes, otherwise the old and new rows conflict on having the same resource_link_id before the delete takes effect

This could be solved in other ways I think (deferrable constraint? CTEs?) but it all looks more complicated than a flush.

[seanh]

Weird. I would think that if you're both deleting an object and updating another object in the same unit of work SQLAlchemy or Postgres would be smart enough to process the deletes first, but I guess not

[seanh]

Okay, so I guess "merging" the two assignments pretty much means deleting the old one. That might be fine. For example I guess it's fine to delete the old assignment's document_url as the new assignment has its own document_url.

But what about assignment.extra? I'm afraid we may need to copy that over to the new assignment. And I'm afraid that could be quite awkward if the new assignment could also potentially have a non-empty extra.

[marcospri]

I don't think we have to worry about extra. Existing canvas DB assignments don't have anything stored there and after all we only care about those in all this merge business.

That's was wrong, it might contain data for course copy.

[marcospri]

One thing we could do for canvas assignments is store a list of all the document_urls a assignment had, might be useful? But I rather do that after all of this is done.

[seanh]

Can we move the delete() and flush() to the bottom of the method? I don't know if it actually makes a difference, but it seems more intuitive to copy old_assignment's attributes to new_assignment and then delete old_assignment, rather than the other way round

[marcospri]

No, that would create a duplicate, added a comment on the flush.

[seanh]

Don't you want to do assignment.resource_link_id = old_assignment.resource_link_id?

[marcospri]

That already happens on the calling function but added it here to have a more independent function

[seanh]

Could this method be clearer if we changed the order of the cases? if ... and not ... and if not ... and ... are relatively complex and hard to read, and I think we might be able to avoid them and use only simpler if's if we do them in a different order:

if not any([resource_link_id, ext_lti_assignment_id]):
    raise ValueError(...)

if resource_link_id and ext_lti_assignment_id:  # Or: if all([resource_link_id, ext_lti_assignment_id])
    return self._get_for_canvas_launch(...)

if not resource_link_id:
    return self._get_by_ext_lti_assignment_id(...)

return self._get_by_resource_link_id(...)

If execution gets past both the if not any([resource_link_id, ext_lti_assignment_id]) and the if resource_link_id and ext_lti_assignment_id then either one of resource_link_id or ext_lti_assignment_id must be None but not both, so only an if not resource_link_id is needed.

[seanh]

I'm unsure about the naming here. As far as I know ext_lti_assignment_id's are Canvas-only so _get_by_ext_lti_assignment() is Canvas-specific but we have no "canvas" in the name. But then we do have "canvas" in the name of _get_for_canvas_launch() below.

Should we pick a side of the fence, in terms of whether or not we're using Canvas-specific naming whenever ext_lti_assignment_id is involved?

Is the solution to rename _get_by_ext_lti_assignment() to something with "canvas" in it? Or to rename _get_for_canvas_launch() to something without Canvas in it, like _get_by_resource_link_id_and_ext_lti_assignment_id() (which is very long but very explicit) or _get_by_both() or something else?

[marcospri]

Went for _get_for_canvas_assignment_config

[seanh]

Suggested change

	if resource_link_id and not assignment.resource_link_id:
	if not assignment.resource_link_id:

_get_for_canvas_launch() is never called without a resource_link_id so we can simplify the if.

You could put assert resource_link_id and ext_lti_assignment_id at the top of _get_for_canvas_launch() if you wanted to make this more explicit.

[seanh]

I think we should move _merge_canvas_assignments() further down in the file, below the _get_for_canvas_launch() that calls it. It's a Canvas-specific edge case so we can hide it down below all the more general _get_*()'s

[seanh]

I think we better add a docstring. Maybe something like this:

def _merge_canvas_assignments(self, old_assignment, new_assignment):
    """
    Merge two Canvas assignments into one and return the merged assignment.

    Merge old_assignment into new_assignment, delete old_assignment, and return the updated
    new_assignment.
    """

[seanh]

I think we're going to have to change this so that the method that the predicates call (currently AssignmentService.exists()) can never have any side effects. The reason is this:

1. Create a new assignment in Canvas. Canvas launches us with an ext_lti_assignment_id but no resource_key. We call _get_for_canvas_assignment_config() and it finds no matching assignment in the DB so we save a new assignment with the ext_lti_assignment_id and no resource_key

2. Now launch the newly-created Canvas assignment for the first time. This time Canvas launches us with both a resource_link_id and an ext_lti_assignment_id. We call _get_for_canvas_launch(). It finds the assignment that we created in step 1 above and sets the existing assignment's resource_link_id.

   But look at the call stack when the assignment.resource_link_id = resource_link_id line gets executed: AssignmentService is being called by one of the view predicates. At this point in the request processing the request has not been authorized. The _permits() function in security.py hasn't been called yet. Could an unauthorized request cause the resource_link_id's of assignments to be set? Or cause two existing assignments to get merged?

In fact I think it turns out that unauthorized requests cannot modify the DB: when authorization fails we fail the request (with a 404 or something) and that causes the DB transaction to not get committed. But I don't think we want to rely on that as the one thing preventing us from making potentially unauthorized changes to the DB.

Nonetheless, I don't think we want to do this: view predicates get called by Pyramid before the view gets selected, before authorization happens, I think we're asking for trouble by letting them make changes to the DB.

[seanh]

Suggested change

	# Legacy Canvas assignments in the DB were only ever saved with
	# Legacy Canvas assignments in the DB were only ever saved with

[marcospri]

Refactored again in another PR #3158