Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Fix the filtered_programs_for_user call #4724

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

rix0rrr
Copy link
Collaborator

@rix0rrr rix0rrr commented Nov 8, 2023

In #4644, the call signature of filtered_programs_for_user was changed to replace the function public_programs_for_user. The goal was to be able to supply the query get_many({ "username": "henk", "public": 1 }), with the goal to retrieve all public programs by user henk.

Unfortunately, there is no index on the pair (username, public). Instead, we have indexes on (username) and on (public, date).

The first is used to retrieve all programs by a single user, the second is used to retrieve all public programs (for the "Explore" page). To get the public programs for a single user, instead what we do is retrieve all programs for a single user, and filter them down in Python.

Because of a bug in the database layer, trying to retrieve programs by { "username": "henk", "public": 1 } uses the (public, date) index. However, the database layer does not detect that in that particular query the field date is missing, and that the field username is unnecessarily passed. The query incorrectly succeeds in the local database emulation mode so this goes undetected during testing, and when passed to an actual DynamoDB database the server returns an error.

This currently breaks the profile page, as well as some other pages probably.

Add public as an optional keyword argument to filtered_programs_for_user and do the filtering locally instead.

How to test

The following pages should work:

  • /my-profile
  • /programs
  • /class/<class_id>/programs/<username>

In #4644, the call signature of `filtered_programs_for_user` was
changed, and the call `public_programs_for_user` removed.

Unfortunately, there is no index on the pair `(username, public)`.
Instead, we have indexes on `(username)` and on `(public, date)`.

The first is used to retrieve all programs by a single user, the second
is used to retrieve all public programs (for the "Explore" page). To get
the public programs for a single user, instead what we do is retrieve
all programs for a single user, and filter them down in Python.

Because of a bug, trying to retrieve programs by `{ "username": "henk",
"public": 1 }` uses the `(public, date)` index. However, the database
layerbut does not detect that the field `date` is missing and that the
field `username` is unnecessarily passed. The query incorrectly succeeds
on the local database emulation layer, and when passed to an actual
DynamoDB database the server returns an error.

This currently breaks the profile page, as well as some other pages
probably.

Revert the changes from #4644 that lead to this problem, restoring
the `public_programs_for_user` method and undoing the change to
`filtered_programs_for_user`.
@ghost
Copy link

ghost commented Nov 8, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@rix0rrr rix0rrr changed the title [FIX] Revert the filtered_programs_for_user change [FIX] Fix the filtered_programs_for_user call Nov 8, 2023
Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix Rico 😄

Copy link
Contributor

mergify bot commented Nov 8, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 4c202a1 into main Nov 8, 2023
11 checks passed
@mergify mergify bot deleted the undo-filtered-programs-for-user-change branch November 8, 2023 22:03
@jpelay
Copy link
Member

jpelay commented Nov 8, 2023

Working correctly on Alpha 🥳

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants