hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
45 stars 14 forks source link

Paginated assignments API endpoint #6395

Closed marcospri closed 1 week ago

marcospri commented 1 week ago

For: https://github.com/hypothesis/lms/issues/6393

The main goal is to apply the same pagination method we just introduced for courses to a new entity and generalize the code around to avoid duplication.

And finally:

This is a barebones endpoint, it doesn't yet accept any query parameters for for example filter by course.

This PR doesn't address any issues with the data returned, nullable titles, (potential) duplicate in the data.

We have to solve those issues for all data returned in any endpoint so I'll fix those in parallel and apply any fixes in all endpoints.

Testing

robertknight commented 1 week ago

Setting limit=0 causes a crash:

web (stderr)         |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
web (stderr)         |   File "/Users/robert/hypothesis/lms/lms/views/dashboard/pagination.py", line 21, in _get_cursor_value
web (stderr)         |     last_element = items[-1]
web (stderr)         |                    ~~~~~^^^^
web (stderr)         | IndexError: list index out of range

limit=-1 returns an empty page. limit=-2 encounters a SQL error:

web (stderr)         | sqlalchemy.exc.DataError: (psycopg2.errors.InvalidRowCountInLimitClause) LIMIT must not be negative
web (stderr)         |
web (stderr)         | [SQL: SELECT assignment.id, assignment.resource_link_id, assignment.tool_consumer_instance_guid, assignment.copied_from_id, assignment.document_url, assignment.extra, assignment.is_gradable, assignment.title, assignment.description, assignment.deep_linking_uuid, assignment.created, assignment.updated
web (stderr)         | FROM assignment ORDER BY assignment.title, assignment.id
web (stderr)         |  LIMIT %(param_1)s]
marcospri commented 1 week ago

Setting limit=0 causes a crash:

:+1: Good catch, I'll validate that limit must be > 0 in the schema.

robertknight commented 1 week ago

It is also possible to cause a crash if the deserialized cursor is not an array.

http://localhost:8001/api/dashboard/assignments?limit=2&cursor=234 leads to:

web (stderr)         |   File "/Users/robert/hypothesis/lms/lms/views/dashboard/pagination.py", line 45, in get_page
web (stderr)         |     items_query = items_query.where(tuple(cursor_columns) > tuple(cursor_values))  # type: ignore
web (stderr)         |                                                             ^^^^^^^^^^^^^^^^^^^^
web (stderr)         | TypeError: 'int' object is not iterable

The issue also happens for the value true (but not false), NaN, floats (1e3).

marcospri commented 1 week ago

Handled the more obvious validation gaps in the PR.

If this PR looks good next step is students.