Skip to content

Don't store objects in the session when describing sounds #2163

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.

Sign up for GitHub

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

Open
alastair wants to merge 1 commit into
base: master
Choose a base branch
from
+186 −33
Open

Don't store objects in the session when describing sounds #2163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 147 additions & 6 deletions accounts/tests/test_upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
from sounds.forms import PackForm
from sounds.models import BulkUploadProgress, License, Pack, Sound
from tags.models import Tag
from utils.filesystem import File
from utils.test_helpers import (
create_test_files,
create_user_and_sounds,
Expand Down Expand Up @@ -99,7 +98,7 @@ def test_select_uploaded_files_to_describe(self):
)
self.assertListEqual(
sorted(
[os.path.basename(f.full_path) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]]
[os.path.basename(f["full_path"]) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]]
),
sorted([filenames[idx] for idx in sounds_to_describe_idx]),
)
Expand All @@ -122,7 +121,7 @@ def test_select_uploaded_files_to_describe(self):
)
self.assertListEqual(
sorted(
[os.path.basename(f.full_path) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]]
[os.path.basename(f["full_path"]) for f in self.client.session[f"{session_key_prefix}-describe_sounds"]]
),
sorted([filenames[idx] for idx in sounds_to_describe_idx]),
)
Expand Down Expand Up @@ -159,12 +158,12 @@ def test_describe_selected_files(self):
# Set license and pack data in session
session = self.client.session
session_key_prefix = "304298eb"
session[f"{session_key_prefix}-describe_license"] = License.objects.all()[0]
session[f"{session_key_prefix}-describe_license"] = License.objects.all()[0].id
session[f"{session_key_prefix}-describe_pack"] = False
session[f"{session_key_prefix}-len_original_describe_sounds"] = 2
session[f"{session_key_prefix}-describe_sounds"] = [
File(1, filenames[0], os.path.join(user_upload_path, filenames[0]), False),
File(2, filenames[1], os.path.join(user_upload_path, filenames[1]), False),
{"name": filenames[0], "full_path": os.path.join(user_upload_path, filenames[0])},
{"name": filenames[1], "full_path": os.path.join(user_upload_path, filenames[1])},
]
session.save()

Expand Down Expand Up @@ -385,3 +384,145 @@ def test_bulk_describe_state_closed(self):
self.client.force_login(user)
resp = self.client.get(reverse("accounts-bulk-describe", args=[bulk.id]))
self.assertContains(resp, "This bulk description process is closed")


class SessionJsonSafetyTests(TestCase):
"""Regression guard: every value the upload/describe/edit flow writes to request.session
must be JSON-serialisable, so SESSION_SERIALIZER can be flipped to JSONSerializer in a
follow-up PR. If this test goes red, something in the flow is putting a Python object
(Sound/License/Pack/File/...) back into the session."""

fixtures = ["licenses", "user_groups", "email_preference_type"]

def _assert_session_json_safe(self):
from django.contrib.sessions.serializers import JSONSerializer

serializer = JSONSerializer()
for key, value in self.client.session.items():
try:
serializer.dumps({key: value})
except TypeError as exc:
self.fail(f"Session key {key!r} holds a non-JSON-safe value (type={type(value).__name__!r}): {exc}")

def _assert_session_value(self, prefix, key, expected):
"""The key must be present AND equal to `expected`, so that a producer write which
is silently skipped (the key never appears) fails loudly instead of letting the
json-safety sweep above pass over a session that never got the value at all."""
full_key = f"{prefix}-{key}"
self.assertIn(full_key, self.client.session, f"producer never wrote {full_key!r}")
self.assertEqual(self.client.session[full_key], expected)

@override_uploads_path_with_temp_directory
def test_describe_flow_session_is_json_safe(self):
filenames = ["file1.wav", "file2.wav"]
user = User.objects.create_user("testuser", email="json@xmpl.com", password="testpass")
self.client.force_login(user)
user_upload_path = settings.UPLOADS_PATH + "/%i/" % user.id
os.makedirs(user_upload_path, exist_ok=True)
create_test_files(filenames, user_upload_path)

# Step 1: select files from pending_description -> writes describe_sounds, len_original_describe_sounds.
# generate_tree() assigns file ids starting at "file0".
resp = self.client.post(
reverse("accounts-manage-sounds", args=["pending_description"]),
{"describe": "describe", "sound-files": ["file0", "file1"]},
)
self.assertEqual(resp.status_code, 302)
session_key_prefix = resp.url.split("session=")[1]
describe_sounds = self.client.session[f"{session_key_prefix}-describe_sounds"]
self.assertTrue(
isinstance(describe_sounds, list)
and all(isinstance(f, dict) and set(f) == {"name", "full_path"} for f in describe_sounds),
f"describe_sounds should be a list of name/full_path dicts, got {describe_sounds!r}",
)
self._assert_session_value(session_key_prefix, "len_original_describe_sounds", 2)
self._assert_session_json_safe()

# Step 2: pick a license -> writes describe_license.
# Must be a license that is actually valid in LicenseForm's queryset (the
# describe_license view uses hide_old_license_versions=True). "Attribution" (4.0) is
# in the queryset and survives the 3.0 clean check, so the producer actually runs --
# using License.objects.all()[0] would (nondeterministically) pick "Sampling+", which
# is not a valid choice, the form would be invalid and the write silently skipped.
attribution = License.objects.get(name="Attribution")
resp = self.client.post(
reverse("accounts-describe-license") + f"?session={session_key_prefix}",
{"license": str(attribution.id)},
)
self.assertEqual(resp.status_code, 302)
self._assert_session_value(session_key_prefix, "describe_license", attribution.id)
self._assert_session_json_safe()

# Step 3: pick "no pack" -> writes describe_pack (= False sentinel)
resp = self.client.post(
reverse("accounts-describe-pack") + f"?session={session_key_prefix}",
{"pack-pack": PackForm.NO_PACK_CHOICE_VALUE, "pack-new_pack": ""},
)
self.assertEqual(resp.status_code, 302)
self._assert_session_value(session_key_prefix, "describe_pack", False)
self._assert_session_json_safe()

def test_describe_pack_id_branches_are_json_safe(self):
# describe_pack writes the …-describe_pack key independently of earlier steps, so we
# exercise each branch against its own fresh session prefix. The two `.id` branches
# (existing pack, new pack) are the ones that previously stored a Pack instance and
# are not reached by the "no pack" path in test_describe_flow_session_is_json_safe.
user = User.objects.create_user("packuser", email="pack@xmpl.com", password="testpass")
self.client.force_login(user)

# Existing pack -> stores data["pack"].id
existing_pack = Pack.objects.create(user=user, name="Existing pack")
resp = self.client.post(
reverse("accounts-describe-pack") + "?session=packex00",
{"pack-pack": str(existing_pack.id), "pack-new_pack": ""},
)
self.assertEqual(resp.status_code, 302)
self._assert_session_value("packex00", "describe_pack", existing_pack.id)
self.assertNotIsInstance(self.client.session["packex00-describe_pack"], bool)
self._assert_session_json_safe()

# New pack -> creates the pack and stores the new pack.id
resp = self.client.post(
reverse("accounts-describe-pack") + "?session=packnw00",
{"pack-pack": PackForm.NEW_PACK_CHOICE_VALUE, "pack-new_pack": "Brand new pack"},
)
self.assertEqual(resp.status_code, 302)
new_pack = Pack.objects.get(user=user, name="Brand new pack")
self._assert_session_value("packnw00", "describe_pack", new_pack.id)
self.assertNotIsInstance(self.client.session["packnw00-describe_pack"], bool)
self._assert_session_json_safe()

# No pack -> stores the False sentinel (must be the bool False, not an int id)
resp = self.client.post(
reverse("accounts-describe-pack") + "?session=packno00",
{"pack-pack": PackForm.NO_PACK_CHOICE_VALUE, "pack-new_pack": ""},
)
self.assertEqual(resp.status_code, 302)
self.assertIs(self.client.session["packno00-describe_pack"], False)
self._assert_session_json_safe()

def test_edit_flow_session_is_json_safe(self):
user, _packs, sounds = create_user_and_sounds(
num_sounds=2,
num_packs=0,
processing_state="OK",
moderation_state="OK",
)
self.client.force_login(user)

# Select sounds for editing from manage_sounds/published -> writes edit_sounds, len_original_edit_sounds
resp = self.client.post(
reverse("accounts-manage-sounds", args=["published"]),
{"edit": "edit", "object-ids": ",".join(str(s.id) for s in sounds)},
)
self.assertEqual(resp.status_code, 302)
# The edit redirect appends `&session=<prefix>` after `next=`, so split defensively.
session_key_prefix = resp.url.split("session=")[1].split("&")[0]
edit_sounds = self.client.session[f"{session_key_prefix}-edit_sounds"]
self.assertTrue(
all(isinstance(i, int) and not isinstance(i, bool) for i in edit_sounds),
f"edit_sounds should be a list of int ids, got {edit_sounds!r}",
)
self.assertEqual(sorted(edit_sounds), sorted(s.id for s in sounds))
self._assert_session_value(session_key_prefix, "len_original_edit_sounds", len(sounds))
self._assert_session_json_safe()
12 changes: 5 additions & 7 deletions accounts/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,9 +780,7 @@ def process_filter_and_sort_options(request, sort_options, tab):
session_key_prefix = str(uuid.uuid4())[
0:8
] # Use a new so we don't interfere with other active description/editing processes
request.session[f"{session_key_prefix}-edit_sounds"] = (
sounds # Add the list of sounds to edit in the session object
)
request.session[f"{session_key_prefix}-edit_sounds"] = [s.id for s in sounds]
request.session[f"{session_key_prefix}-len_original_edit_sounds"] = len(sounds)
return HttpResponseRedirect(
reverse("accounts-edit-sounds") + f"?next={request.path}&session={session_key_prefix}"
Expand Down Expand Up @@ -930,7 +928,7 @@ def sounds_pending_description_helper(request, file_structure, files):
0:8
] # Use a new so we don't interfere with other active description/editing processes
request.session[f"{session_key_prefix}-describe_sounds"] = [
files[x] for x in form.cleaned_data["files"]
{"name": files[x].name, "full_path": files[x].full_path} for x in form.cleaned_data["files"]
]
request.session[f"{session_key_prefix}-len_original_describe_sounds"] = len(
request.session[f"{session_key_prefix}-describe_sounds"]
Expand Down Expand Up @@ -963,7 +961,7 @@ def describe_license(request):
if request.method == "POST":
form = LicenseForm(request.POST, hide_old_license_versions=True)
if form.is_valid():
request.session[f"{session_key_prefix}-describe_license"] = form.cleaned_data["license"]
request.session[f"{session_key_prefix}-describe_license"] = form.cleaned_data["license"].id
return HttpResponseRedirect(reverse("accounts-describe-pack") + f"?session={session_key_prefix}")
else:
form = LicenseForm(hide_old_license_versions=True)
Expand All @@ -985,9 +983,9 @@ def describe_pack(request):
data = form.cleaned_data
if data["new_pack"]:
pack, created = Pack.objects.get_or_create(user=request.user, name=data["new_pack"])
request.session[f"{session_key_prefix}-describe_pack"] = pack
request.session[f"{session_key_prefix}-describe_pack"] = pack.id
elif data["pack"]:
request.session[f"{session_key_prefix}-describe_pack"] = data["pack"]
request.session[f"{session_key_prefix}-describe_pack"] = data["pack"].id
else:
request.session[f"{session_key_prefix}-describe_pack"] = False
return HttpResponseRedirect(reverse("accounts-describe-sounds") + f"?session={session_key_prefix}")
Expand Down
54 changes: 34 additions & 20 deletions sounds/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
from sounds.models import (
DeletedSound,
Download,
License,
Pack,
PackDownload,
PackDownloadSound,
Expand Down Expand Up @@ -420,9 +421,7 @@ def sound_edit(request, username, sound_id):
session_key_prefix = request.GET.get(
"session", str(uuid.uuid4())[0:8]
) # Get existing session key if we are already in an edit session or create a new one
request.session[f"{session_key_prefix}-edit_sounds"] = [
sound
] # Add the list of sounds to edit in the session object
request.session[f"{session_key_prefix}-edit_sounds"] = [sound.id]
request.session[f"{session_key_prefix}-len_original_edit_sounds"] = 1
return edit_and_describe_sounds_helper(request, session_key_prefix=session_key_prefix)

Expand Down Expand Up @@ -595,10 +594,17 @@ def update_edited_sound(sound, data):
for pack_to_process in packs_to_process:
pack_to_process.process()

files = request.session.get(
f"{session_key_prefix}-describe_sounds", None
) # List of File objects of sounds to describe
sounds = request.session.get(f"{session_key_prefix}-edit_sounds", None) # List of Sound objects to edit
files = request.session.get(f"{session_key_prefix}-describe_sounds", None)
sound_ids = request.session.get(f"{session_key_prefix}-edit_sounds", None)
# Back-compat shim for sessions written before the switch to JSON-safe values
# (pickled File / Sound instances). Remove once SESSION_SERIALIZER is flipped to
# JSONSerializer in the follow-up PR.
if files and not isinstance(files[0], dict):
files = [{"name": f.name, "full_path": f.full_path} for f in files]
if sound_ids and not isinstance(sound_ids[0], int):
sound_ids = [s.id for s in sound_ids]
# Preserve today's `is None` vs `== []` distinction: empty list ≠ missing key.
sounds = list(Sound.objects.ordered_ids(sound_ids)) if sound_ids is not None else None
if (describing and files is None) or (not describing and sounds is None):
# Expecting either a list of sounds or audio files to describe, got none. Redirect to main manage sounds page.
return HttpResponseRedirect(reverse("accounts-manage-sounds", args=["published"]))
Expand All @@ -617,17 +623,20 @@ def update_edited_sound(sound, data):
(len_original_describe_edit_sounds - len(all_remaining_sounds_to_edit_or_describe)) / forms_per_round + 1
)
files_data_for_players = [] # Used when describing sounds (not when editing) to be able to show sound players
preselected_license = request.session.get(
f"{session_key_prefix}-describe_license", False
) # Pre-selected from the license selection page when describing multiple sounds
preselected_pack = request.session.get(
f"{session_key_prefix}-describe_pack", False
) # Pre-selected from the pack selection page when describing multiple sounds
preselected_license_id = request.session.get(f"{session_key_prefix}-describe_license", False)
preselected_pack_id = request.session.get(f"{session_key_prefix}-describe_pack", False)
# Back-compat shim (bool is a subclass of int, so the int check covers False/True too).
if preselected_license_id and not isinstance(preselected_license_id, int):
preselected_license_id = preselected_license_id.id
if preselected_pack_id and not isinstance(preselected_pack_id, int):
preselected_pack_id = preselected_pack_id.id
preselected_license = License.objects.filter(id=preselected_license_id).first() if preselected_license_id else False
preselected_pack = Pack.objects.filter(id=preselected_pack_id).first() if preselected_pack_id else False

for count, element in enumerate(sounds_to_edit_or_describe):
prefix = str(count)
if describing:
audio_file_path = element.full_path
audio_file_path = element["full_path"]
duration = get_duration_from_processing_before_describe_files(audio_file_path)
if duration > 0.0:
processing_before_describe_base_url = get_processing_before_describe_sound_base_url(audio_file_path)
Expand All @@ -650,7 +659,7 @@ def update_edited_sound(sound, data):
form = SoundEditAndDescribeForm(
request.POST,
prefix=prefix,
file_full_path=element.full_path if describing else None,
file_full_path=element["full_path"] if describing else None,
explicit_disable=element.is_explicit if not describing else False,
hide_old_license_versions="3.0" not in element.license.deed_url if not describing else True,
user_packs=Pack.objects.filter(user=request.user if describing else element.user).exclude(
Expand Down Expand Up @@ -679,7 +688,7 @@ def update_edited_sound(sound, data):
form.cleaned_data["sources"]
) # Add sources ids to list so sources sound selector can be initialized
if describing:
form.audio_filename = element.name
form.audio_filename = element["name"]
else:
form.sound_id = element.id
else:
Expand All @@ -699,7 +708,7 @@ def update_edited_sound(sound, data):
)
else:
sound_sources_ids = []
initial = dict(name=os.path.splitext(element.name)[0])
initial = dict(name=os.path.splitext(element["name"])[0])
if preselected_license:
initial["license"] = preselected_license
if preselected_pack:
Expand All @@ -715,7 +724,7 @@ def update_edited_sound(sound, data):
)
form.sound_sources_ids = sound_sources_ids
if describing:
form.audio_filename = element.name
form.audio_filename = element["name"]
else:
form.sound_id = element.id
forms.append(form)
Expand Down Expand Up @@ -757,8 +766,13 @@ def update_edited_sound(sound, data):
else:
return HttpResponseRedirect(reverse("accounts-describe-sounds") + f"?session={session_key_prefix}")
else:
# Remove sounds successfully described from session data
request.session[f"{session_key_prefix}-edit_sounds"] = sounds[forms_per_round:]
# Remove sounds successfully edited from session data.
# CRITICAL: derive remaining ids from the *refetched* sounds list, not from
# the raw sound_ids. Sound.objects.ordered_ids drops deleted ids, so
# len(sounds) ≤ len(sound_ids); slicing sound_ids[forms_per_round:] would
# re-present sounds already processed in this round whenever any earlier id
# had been deleted — silent duplicate edits.
request.session[f"{session_key_prefix}-edit_sounds"] = [s.id for s in sounds[forms_per_round:]]

# If user was only editing one sound and has finished, clear session and redirect to the sound page
if len(forms) == 1 and len_original_describe_edit_sounds == 1:
Expand Down
Loading