From dcdcbfd83391314db93cb2cdbbf3cc12619d084e Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Fri, 3 Jan 2025 16:56:29 -0800 Subject: [PATCH] [ENG-2157] [Refix] Allow `rx.download` to resolve `rx.get_upload_url` (#4470) * [ENG-2157] [Refix] Allow `rx.download` to resolve `rx.get_upload_url` Update the special case in a way that works with the new Var system and its unstoppable determination to concatenate strings instead of using template string This has been broken since 0.6.0, so a regression test was added to avoid future inadvertant breakage. Fix #2812 (again) * use safer indirect eval --- reflex/.templates/web/utils/state.js | 13 ++- tests/integration/test_upload.py | 160 +++++++++++++++++++++------ 2 files changed, 138 insertions(+), 35 deletions(-) diff --git a/reflex/.templates/web/utils/state.js b/reflex/.templates/web/utils/state.js index 608df084a..41dbee446 100644 --- a/reflex/.templates/web/utils/state.js +++ b/reflex/.templates/web/utils/state.js @@ -208,11 +208,16 @@ export const applyEvent = async (event, socket) => { if (event.name == "_download") { const a = document.createElement("a"); a.hidden = true; + a.href = event.payload.url; // Special case when linking to uploaded files - a.href = event.payload.url.replace( - "${getBackendURL(env.UPLOAD)}", - getBackendURL(env.UPLOAD) - ); + if (a.href.includes("getBackendURL(env.UPLOAD)")) { + a.href = eval?.( + event.payload.url.replace( + "getBackendURL(env.UPLOAD)", + `"${getBackendURL(env.UPLOAD)}"` + ) + ); + } a.download = event.payload.filename; a.click(); a.remove(); diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 156cf0e45..0331c15d6 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -6,12 +6,16 @@ import asyncio import time from pathlib import Path from typing import Generator +from urllib.parse import urlsplit import pytest from selenium.webdriver.common.by import By +from reflex.constants.event import Endpoint from reflex.testing import AppHarness, WebDriver +from .utils import poll_for_navigation + def UploadFile(): """App for testing dynamic routes.""" @@ -23,7 +27,7 @@ def UploadFile(): class UploadState(rx.State): _file_data: Dict[str, str] = {} - event_order: List[str] = [] + event_order: rx.Field[List[str]] = rx.field([]) progress_dicts: List[dict] = [] disabled: bool = False large_data: str = "" @@ -50,6 +54,15 @@ def UploadFile(): self.large_data = "" self.event_order.append("chain_event") + async def handle_upload_tertiary(self, files: List[rx.UploadFile]): + for file in files: + (rx.get_upload_dir() / (file.filename or "INVALID")).write_bytes( + await file.read() + ) + + def do_download(self): + return rx.download(rx.get_upload_url("test.txt")) + def index(): return rx.vstack( rx.input( @@ -123,6 +136,34 @@ def UploadFile(): on_click=rx.cancel_upload("secondary"), id="cancel_button_secondary", ), + rx.heading("Tertiary Upload/Download"), + rx.upload.root( + rx.vstack( + rx.button("Select File"), + rx.text("Drag and drop files here or click to select files"), + ), + id="tertiary", + ), + rx.button( + "Upload", + on_click=UploadState.handle_upload_tertiary( # type: ignore + rx.upload_files( + upload_id="tertiary", + ), + ), + id="upload_button_tertiary", + ), + rx.button( + "Download - Frontend", + on_click=rx.download(rx.get_upload_url("test.txt")), + id="download-frontend", + ), + rx.button( + "Download - Backend", + on_click=UploadState.do_download, + id="download-backend", + ), + rx.text(UploadState.event_order.to_string(), id="event-order"), ) app = rx.App(state=rx.State) @@ -164,6 +205,24 @@ def driver(upload_file: AppHarness): driver.quit() +def poll_for_token(driver: WebDriver, upload_file: AppHarness) -> str: + """Poll for the token input to be populated. + + Args: + driver: WebDriver instance. + upload_file: harness for UploadFile app. + + Returns: + token value + """ + token_input = driver.find_element(By.ID, "token") + assert token_input + # wait for the backend connection to send the token + token = upload_file.poll_for_value(token_input) + assert token is not None + return token + + @pytest.mark.parametrize("secondary", [False, True]) @pytest.mark.asyncio async def test_upload_file( @@ -178,11 +237,7 @@ async def test_upload_file( secondary: whether to use the secondary upload form """ assert upload_file.app_instance is not None - token_input = driver.find_element(By.ID, "token") - assert token_input - # wait for the backend connection to send the token - token = upload_file.poll_for_value(token_input) - assert token is not None + token = poll_for_token(driver, upload_file) full_state_name = upload_file.get_full_state_name(["_upload_state"]) state_name = upload_file.get_state_name("_upload_state") substate_token = f"{token}_{full_state_name}" @@ -204,6 +259,19 @@ async def test_upload_file( upload_box.send_keys(str(target_file)) upload_button.click() + # check that the selected files are displayed + selected_files = driver.find_element(By.ID, f"selected_files{suffix}") + assert Path(selected_files.text).name == Path(exp_name).name + + if secondary: + event_order_displayed = driver.find_element(By.ID, "event-order") + AppHarness._poll_for(lambda: "chain_event" in event_order_displayed.text) + + state = await upload_file.get_state(substate_token) + # only the secondary form tracks progress and chain events + assert state.substates[state_name].event_order.count("upload_progress") == 1 + assert state.substates[state_name].event_order.count("chain_event") == 1 + # look up the backend state and assert on uploaded contents async def get_file_data(): return ( @@ -217,16 +285,6 @@ async def test_upload_file( normalized_file_data = {Path(k).name: v for k, v in file_data.items()} assert normalized_file_data[Path(exp_name).name] == exp_contents - # check that the selected files are displayed - selected_files = driver.find_element(By.ID, f"selected_files{suffix}") - assert Path(selected_files.text).name == Path(exp_name).name - - state = await upload_file.get_state(substate_token) - if secondary: - # only the secondary form tracks progress and chain events - assert state.substates[state_name].event_order.count("upload_progress") == 1 - assert state.substates[state_name].event_order.count("chain_event") == 1 - @pytest.mark.asyncio async def test_upload_file_multiple(tmp_path, upload_file: AppHarness, driver): @@ -238,11 +296,7 @@ async def test_upload_file_multiple(tmp_path, upload_file: AppHarness, driver): driver: WebDriver instance. """ assert upload_file.app_instance is not None - token_input = driver.find_element(By.ID, "token") - assert token_input - # wait for the backend connection to send the token - token = upload_file.poll_for_value(token_input) - assert token is not None + token = poll_for_token(driver, upload_file) full_state_name = upload_file.get_full_state_name(["_upload_state"]) state_name = upload_file.get_state_name("_upload_state") substate_token = f"{token}_{full_state_name}" @@ -301,11 +355,7 @@ def test_clear_files( secondary: whether to use the secondary upload form. """ assert upload_file.app_instance is not None - token_input = driver.find_element(By.ID, "token") - assert token_input - # wait for the backend connection to send the token - token = upload_file.poll_for_value(token_input) - assert token is not None + poll_for_token(driver, upload_file) suffix = "_secondary" if secondary else "" @@ -357,11 +407,7 @@ async def test_cancel_upload(tmp_path, upload_file: AppHarness, driver: WebDrive driver: WebDriver instance. """ assert upload_file.app_instance is not None - token_input = driver.find_element(By.ID, "token") - assert token_input - # wait for the backend connection to send the token - token = upload_file.poll_for_value(token_input) - assert token is not None + token = poll_for_token(driver, upload_file) state_name = upload_file.get_state_name("_upload_state") state_full_name = upload_file.get_full_state_name(["_upload_state"]) substate_token = f"{token}_{state_full_name}" @@ -403,3 +449,55 @@ async def test_cancel_upload(tmp_path, upload_file: AppHarness, driver: WebDrive assert Path(exp_name).name not in normalized_file_data target_file.unlink() + + +@pytest.mark.asyncio +async def test_upload_download_file( + tmp_path, + upload_file: AppHarness, + driver: WebDriver, +): + """Submit a file upload and then fetch it with rx.download. + + This checks the special case `getBackendURL` logic in the _download event + handler in state.js. + + Args: + tmp_path: pytest tmp_path fixture + upload_file: harness for UploadFile app. + driver: WebDriver instance. + """ + assert upload_file.app_instance is not None + poll_for_token(driver, upload_file) + + upload_box = driver.find_elements(By.XPATH, "//input[@type='file']")[2] + assert upload_box + upload_button = driver.find_element(By.ID, "upload_button_tertiary") + assert upload_button + + exp_name = "test.txt" + exp_contents = "test file contents!" + target_file = tmp_path / exp_name + target_file.write_text(exp_contents) + + upload_box.send_keys(str(target_file)) + upload_button.click() + + # Download via event embedded in frontend code. + download_frontend = driver.find_element(By.ID, "download-frontend") + with poll_for_navigation(driver): + download_frontend.click() + assert urlsplit(driver.current_url).path == f"/{Endpoint.UPLOAD.value}/test.txt" + assert driver.find_element(by=By.TAG_NAME, value="body").text == exp_contents + + # Go back and wait for the app to reload. + with poll_for_navigation(driver): + driver.back() + poll_for_token(driver, upload_file) + + # Download via backend event handler. + download_backend = driver.find_element(By.ID, "download-backend") + with poll_for_navigation(driver): + download_backend.click() + assert urlsplit(driver.current_url).path == f"/{Endpoint.UPLOAD.value}/test.txt" + assert driver.find_element(by=By.TAG_NAME, value="body").text == exp_contents