From d21ee6fa23ed26fef7793ab7a7c5a8d364aff47a Mon Sep 17 00:00:00 2001 From: Vector73 Date: Mon, 15 Jul 2024 10:36:38 +0530 Subject: [PATCH] api: Deprecate uri and add url parameter in "/user_uploads" endpoint. --- api_docs/changelog.md | 6 +++ version.py | 2 +- web/src/upload.ts | 2 +- web/tests/upload.test.js | 4 +- zerver/openapi/python_examples.py | 2 +- zerver/openapi/zulip.yaml | 16 +++++- .../test_delete_unclaimed_attachments.py | 2 +- zerver/tests/test_events.py | 4 +- zerver/tests/test_scheduled_messages.py | 4 +- zerver/tests/test_subs.py | 2 +- zerver/tests/test_thumbnail.py | 7 ++- zerver/tests/test_upload.py | 51 +++++++++++-------- zerver/tests/test_upload_local.py | 3 +- zerver/tests/test_upload_s3.py | 4 +- zerver/views/upload.py | 7 ++- 15 files changed, 78 insertions(+), 38 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 4ae4fa6c64..5606c48f67 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 9.0 +**Feature level 272** + +* [`POST /user_uploads`](/api/upload-file): `uri` was renamed + to `url`, but remains available as a deprecated alias for + backwards-compatibility. + **Feature level 271** * [`GET /messages`](/api/get-messages), diff --git a/version.py b/version.py index 6256378b44..34849354b4 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 271 # Last bumped for `with` operator. +API_FEATURE_LEVEL = 272 # Last bumped for "POST /user_uploads" # Bump the minor PROVISION_VERSION to indicate that folks should provision diff --git a/web/src/upload.ts b/web/src/upload.ts index cbc9802046..d716e2623c 100644 --- a/web/src/upload.ts +++ b/web/src/upload.ts @@ -369,7 +369,7 @@ export function setup_upload(config: Config): Uppy { uppy.on("upload-success", (file, response) => { assert(file !== undefined); - const {uri: url} = z.object({uri: z.string().optional()}).parse(response.body); + const {url} = z.object({url: z.string().optional()}).parse(response.body); if (url === undefined) { return; } diff --git a/web/tests/upload.test.js b/web/tests/upload.test.js index cd15479fd1..01fb417968 100644 --- a/web/tests/upload.test.js +++ b/web/tests/upload.test.js @@ -500,7 +500,7 @@ test("uppy_events", ({override_rewire, mock_template}) => { }; let response = { body: { - uri: "/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png", + url: "/user_uploads/4/cb/rue1c-MlMUjDAUdkRrEM4BTJ/copenhagen.png", }, }; @@ -525,7 +525,7 @@ test("uppy_events", ({override_rewire, mock_template}) => { response = { body: { - uri: undefined, + url: undefined, }, }; compose_ui_replace_syntax_called = false; diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index ce5dc489fc..50cf1f8aa6 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -1416,7 +1416,7 @@ def upload_file(client: Client) -> None: "type": "stream", "to": "Denmark", "topic": "Castle", - "content": "Check out [this picture]({}) of my castle!".format(result["uri"]), + "content": "Check out [this picture]({}) of my castle!".format(result["url"]), } ) # {code_example|end} diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 87415a5f0a..db6ed61f99 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -8093,7 +8093,7 @@ paths: summary: Upload a file tags: ["messages"] description: | - Upload a single file and get the corresponding URI. + Upload a single file and get the corresponding URL. Initially, only you will be able to access the link. To share the uploaded file, you'll need to [send a message][send-message] @@ -8140,13 +8140,25 @@ paths: ignored_parameters_unsupported: {} uri: type: string + deprecated: true description: | - The URI of the uploaded file. + The URL of the uploaded file. Alias of `url`. + + **Changes**: Deprecated in Zulip 9.0 (feature level 272). The term + "URI" is deprecated in [web standards](https://url.spec.whatwg.org/#goals). + url: + type: string + description: | + The URL of the uploaded file. + + **Changes**: New in Zulip 9.0 (feature level 272). Previously, + this property was only available under the legacy `uri` name. example: { "msg": "", "result": "success", "uri": "/user_uploads/1/4e/m2A3MSqFnWRLUf9SaPzQ0Up_/zulip.txt", + "url": "/user_uploads/1/4e/m2A3MSqFnWRLUf9SaPzQ0Up_/zulip.txt", } /user_uploads/{realm_id_str}/{filename}: get: diff --git a/zerver/tests/test_delete_unclaimed_attachments.py b/zerver/tests/test_delete_unclaimed_attachments.py index 61cb0d2b28..4c0db52b43 100644 --- a/zerver/tests/test_delete_unclaimed_attachments.py +++ b/zerver/tests/test_delete_unclaimed_attachments.py @@ -32,7 +32,7 @@ class UnclaimedAttachmentTest(UploadSerializeMixin, ZulipTestCase): response = self.assert_json_success( self.client_post("/json/user_uploads", {"file": file_obj}) ) - path_id = re.sub(r"/user_uploads/", "", response["uri"]) + path_id = re.sub(r"/user_uploads/", "", response["url"]) return Attachment.objects.get(path_id=path_id) def assert_exists( diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 5e7306890e..eb06698de5 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3231,7 +3231,9 @@ class NormalActionsTest(BaseAction): response_dict = self.assert_json_success(result) self.assertIn("uri", response_dict) - url = response_dict["uri"] + self.assertIn("url", response_dict) + url = response_dict["url"] + self.assertEqual(response_dict["uri"], url) base = "/user_uploads/" self.assertEqual(base, url[: len(base)]) diff --git a/zerver/tests/test_scheduled_messages.py b/zerver/tests/test_scheduled_messages.py index ab71ab171b..d5a5001633 100644 --- a/zerver/tests/test_scheduled_messages.py +++ b/zerver/tests/test_scheduled_messages.py @@ -656,13 +656,13 @@ class ScheduledMessageTest(ZulipTestCase): attachment_file1 = StringIO("zulip!") attachment_file1.name = "dummy_1.txt" result = self.client_post("/json/user_uploads", {"file": attachment_file1}) - path_id1 = re.sub(r"/user_uploads/", "", result.json()["uri"]) + path_id1 = re.sub(r"/user_uploads/", "", result.json()["url"]) attachment_object1 = Attachment.objects.get(path_id=path_id1) attachment_file2 = StringIO("zulip!") attachment_file2.name = "dummy_1.txt" result = self.client_post("/json/user_uploads", {"file": attachment_file2}) - path_id2 = re.sub(r"/user_uploads/", "", result.json()["uri"]) + path_id2 = re.sub(r"/user_uploads/", "", result.json()["url"]) attachment_object2 = Attachment.objects.get(path_id=path_id2) content = f"Test [zulip.txt](http://{hamlet.realm.host}/user_uploads/{path_id1})" diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index faaed28a1a..d8cd2fa63a 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -1195,7 +1195,7 @@ class StreamAdminTest(ZulipTestCase): fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] owner = self.example_user("desdemona") realm = owner.realm diff --git a/zerver/tests/test_thumbnail.py b/zerver/tests/test_thumbnail.py index 7e80af056f..25e7ec3b3f 100644 --- a/zerver/tests/test_thumbnail.py +++ b/zerver/tests/test_thumbnail.py @@ -20,7 +20,9 @@ class ThumbnailTest(ZulipTestCase): self.assert_json_success(result) json = orjson.loads(result.content) self.assertIn("uri", json) - url = json["uri"] + self.assertIn("url", json) + url = json["url"] + self.assertEqual(json["uri"], url) base = "/user_uploads/" self.assertEqual(base, url[: len(base)]) @@ -60,7 +62,8 @@ class ThumbnailTest(ZulipTestCase): result = self.client_post("/json/user_uploads", {"file": fp}) self.assert_json_success(result) json = orjson.loads(result.content) - url = json["uri"] + url = json["url"] + self.assertEqual(json["uri"], url) with ratelimit_rule(86400, 1000, domain="spectator_attachment_access_by_file"): # Deny file access for non-web-public stream diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 01202c55f5..ed6556bad8 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -56,7 +56,9 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): result = self.api_post(self.example_user("hamlet"), "/api/v1/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) self.assertIn("uri", response_dict) - url = response_dict["uri"] + self.assertIn("url", response_dict) + url = response_dict["url"] + self.assertEqual(response_dict["uri"], url) base = "/user_uploads/" self.assertEqual(base, url[: len(base)]) @@ -83,7 +85,9 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): result = self.api_post(self.example_user("hamlet"), "/api/v1/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) self.assertIn("uri", response_dict) - url = response_dict["uri"] + self.assertIn("url", response_dict) + url = response_dict["url"] + self.assertEqual(response_dict["uri"], url) base = "/user_uploads/" self.assertEqual(base, url[: len(base)]) @@ -171,7 +175,9 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) self.assertIn("uri", response_dict) - url = response_dict["uri"] + self.assertIn("url", response_dict) + url = response_dict["url"] + self.assertEqual(response_dict["uri"], url) base = "/user_uploads/" self.assertEqual(base, url[: len(base)]) @@ -218,7 +224,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp.name = "zulip_web_public.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] with ratelimit_rule(86400, 1000, domain="spectator_attachment_access_by_file"): # Deny file access for non-web-public stream @@ -275,7 +281,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) - url = "/json" + response_dict["uri"] + url = "/json" + response_dict["url"] result = self.client_get(url) data = self.assert_json_success(result) @@ -292,7 +298,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) - url = "/json" + response_dict["uri"] + url = "/json" + response_dict["url"] start_time = time.time() with mock.patch("django.core.signing.time.time", return_value=start_time): @@ -314,7 +320,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) - url = response_dict["uri"] + url = response_dict["url"] self.logout() response = self.client_get(url) @@ -330,7 +336,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) - url = response_dict["uri"] + url = response_dict["url"] self.logout() response = self.client_get( @@ -356,6 +362,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): response = self.client_get(response_dict["uri"]) self.assertEqual(response.status_code, 404) + response = self.client_get(response_dict["url"]) + self.assertEqual(response.status_code, 404) def test_non_existing_file_download(self) -> None: """ @@ -421,7 +429,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): d1.name = "dummy_1.txt" result = self.client_post("/json/user_uploads", {"file": d1}) response_dict = self.assert_json_success(result) - d1_path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + d1_path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) self.subscribe(self.example_user("hamlet"), "Denmark") host = self.example_user("hamlet").realm.host @@ -440,7 +448,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): d1.name = "dummy_1.txt" result = self.client_post("/json/user_uploads", {"file": d1}) response_dict = self.assert_json_success(result) - d1_path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + d1_path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) host = self.example_user("hamlet").realm.host self.make_stream("private_stream", invite_only=True) @@ -500,11 +508,11 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): self.login_user(hamlet) result = self.client_post("/json/user_uploads", {"file": f1}) response_dict = self.assert_json_success(result) - f1_path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + f1_path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) result = self.client_post("/json/user_uploads", {"file": f2}) response_dict = self.assert_json_success(result) - f2_path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + f2_path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) self.subscribe(hamlet, "test") body = ( @@ -515,7 +523,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): result = self.client_post("/json/user_uploads", {"file": f3}) response_dict = self.assert_json_success(result) - f3_path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + f3_path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) new_body = ( f"[f3.txt](http://{host}/user_uploads/" + f3_path_id + ") " @@ -568,6 +576,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): result = self.client_post("/json/user_uploads", {"f1": fp}) response_dict = self.assert_json_success(result) assert sanitize_name(expected) in response_dict["uri"] + assert sanitize_name(expected) in response_dict["url"] def test_sanitize_file_name(self) -> None: self.login("hamlet") @@ -587,6 +596,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): response_dict = self.assert_json_success(result) self.assertNotIn(response_dict["uri"], uploaded_filename) self.assertTrue(response_dict["uri"].endswith("/" + expected)) + self.assertNotIn(response_dict["url"], uploaded_filename) + self.assertTrue(response_dict["url"].endswith("/" + expected)) def test_realm_quota(self) -> None: """ @@ -598,7 +609,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): d1.name = "dummy_1.txt" result = self.client_post("/json/user_uploads", {"file": d1}) response_dict = self.assert_json_success(result) - d1_path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + d1_path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) d1_attachment = Attachment.objects.get(path_id=d1_path_id) realm = get_realm("zulip") @@ -657,7 +668,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp = StringIO("zulip!") fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] fp_path_id = re.sub(r"/user_uploads/", "", url) body = f"First message ...[zulip.txt](http://{host}/user_uploads/" + fp_path_id + ")" with self.settings(CROSS_REALM_BOT_EMAILS={user_2.email, user_3.email}): @@ -703,7 +714,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp = StringIO("zulip!") fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] fp_path_id = re.sub(r"/user_uploads/", "", url) body = f"First message ...[zulip.txt](http://{realm.host}/user_uploads/" + fp_path_id + ")" self.send_stream_message(hamlet, stream_name, body, "test") @@ -755,7 +766,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp = StringIO("zulip!") fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] fp_path_id = re.sub(r"/user_uploads/", "", url) body = ( f"First message ...[zulip.txt](http://{user.realm.host}/user_uploads/" @@ -824,7 +835,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp = StringIO("zulip!") fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] fp_path_id = re.sub(r"/user_uploads/", "", url) for i in range(20): body = ( @@ -869,7 +880,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp = StringIO("zulip!") fp.name = "zulip.txt" result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] fp_path_id = re.sub(r"/user_uploads/", "", url) body = f"First message ...[zulip.txt](http://{realm.host}/user_uploads/" + fp_path_id + ")" self.send_stream_message(self.example_user("hamlet"), "test-subscribe", body, "test") @@ -893,7 +904,7 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase): fp = StringIO("zulip!") fp.name = name result = self.client_post("/json/user_uploads", {"file": fp}) - url = self.assert_json_success(result)["uri"] + url = self.assert_json_success(result)["url"] fp_path_id = re.sub(r"/user_uploads/", "", url) fp_path = os.path.split(fp_path_id)[0] if download: diff --git a/zerver/tests/test_upload_local.py b/zerver/tests/test_upload_local.py index ccf2451cc8..d076baffc0 100644 --- a/zerver/tests/test_upload_local.py +++ b/zerver/tests/test_upload_local.py @@ -76,7 +76,8 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) - path_id = re.sub(r"/user_uploads/", "", response_dict["uri"]) + self.assertEqual(response_dict["uri"], response_dict["url"]) + path_id = re.sub(r"/user_uploads/", "", response_dict["url"]) assert settings.LOCAL_FILES_DIR is not None file_path = os.path.join(settings.LOCAL_FILES_DIR, path_id) diff --git a/zerver/tests/test_upload_s3.py b/zerver/tests/test_upload_s3.py index 2c2baca5fc..d8aa9b3e99 100644 --- a/zerver/tests/test_upload_s3.py +++ b/zerver/tests/test_upload_s3.py @@ -164,8 +164,10 @@ class S3Test(ZulipTestCase): result = self.client_post("/json/user_uploads", {"file": fp}) response_dict = self.assert_json_success(result) self.assertIn("uri", response_dict) + self.assertIn("url", response_dict) base = "/user_uploads/" - url = response_dict["uri"] + url = response_dict["url"] + self.assertEqual(response_dict["uri"], url) self.assertEqual(base, url[: len(base)]) # In development, this is just a redirect diff --git a/zerver/views/upload.py b/zerver/views/upload.py index 31ea8419c4..faf5f85df7 100644 --- a/zerver/views/upload.py +++ b/zerver/views/upload.py @@ -319,5 +319,8 @@ def upload_file_backend(request: HttpRequest, user_profile: UserProfile) -> Http ) check_upload_within_quota(user_profile.realm, file_size) - uri = upload_message_attachment_from_request(user_file, user_profile) - return json_success(request, data={"uri": uri}) + url = upload_message_attachment_from_request(user_file, user_profile) + + # TODO/compatibility: uri is a deprecated alias for url that can + # be removed once there are no longer clients relying on it. + return json_success(request, data={"uri": url, "url": url})