diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 32ccaf0895..2944eb7e96 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,17 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 211** + +* [`POST /streams/{stream_id}/delete_topic`](/api/delete-topic), + [`POST /mark_all_as_read`](/api/mark-all-as-read): + Added a `complete` boolean field in the success response to indicate + whether all or only some of the targeted messages were processed. + This replaces the use of `"result": "partially_completed"` (introduced + in feature levels 154 and 153), so that these endpoints now send a + `result` string of either `"success"` or `"error"`, like the rest of + the Zulip API. + **Feature level 210** * [`POST /register`](/api/register-queue), [`PATCH /settings`](/api/update-settings), @@ -538,17 +549,20 @@ No changes; feature level used for Zulip 6.0 release. **Feature level 154** * [`POST /streams/{stream_id}/delete_topic`](/api/delete-topic): - When the process of deleting messages times out, a success response - with "partially_completed" result will now be returned by the server, - analogically to the `/mark_all_as_read` endpoint. + When the process of deleting messages times out, but successfully + deletes some messages in the topic (see feature level 147 for when + this endpoint started deleting messages in batches), a success + response with `"result": "partially_completed"` will now be returned + by the server, analogically to the `POST /mark_all_as_read` endpoint + (see feature level 153 entry below). **Feature level 153** * [`POST /mark_all_as_read`](/api/mark-all-as-read): Messages are now marked as read in batches, so that progress will be made even if the request times out because of an extremely large number of unread - messages to process. Upon timeout, a success response with a - "partially_completed" result will be returned by the server. + messages to process. Upon timeout, a success response with + `"result": "partially_completed"` will be returned by the server. **Feature level 152** diff --git a/version.py b/version.py index bd8d1c1878..916688ec3a 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 210 +API_FEATURE_LEVEL = 211 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/web/src/message_edit.js b/web/src/message_edit.js index d3e6f434e3..9e280248ee 100644 --- a/web/src/message_edit.js +++ b/web/src/message_edit.js @@ -1156,7 +1156,7 @@ export function delete_topic(stream_id, topic_name, failures = 0) { topic_name, }, success(data) { - if (data.result === "partially_completed") { + if (data.complete === false) { if (failures >= 9) { // Don't keep retrying indefinitely to avoid DoSing the server. return; diff --git a/zerver/lib/response.py b/zerver/lib/response.py index 451d177037..83b313249f 100644 --- a/zerver/lib/response.py +++ b/zerver/lib/response.py @@ -105,10 +105,6 @@ def json_success(request: HttpRequest, data: Mapping[str, Any] = {}) -> MutableJ return json_response(data=data) -def json_partial_success(request: HttpRequest, data: Mapping[str, Any] = {}) -> MutableJsonResponse: - return json_response(res_type="partially_completed", data=data, status=200) - - def json_response_from_error(exception: JsonableError) -> MutableJsonResponse: """ This should only be needed in middleware; in app code, just raise. diff --git a/zerver/openapi/openapi.py b/zerver/openapi/openapi.py index 14aba7c347..d123b27edb 100644 --- a/zerver/openapi/openapi.py +++ b/zerver/openapi/openapi.py @@ -332,10 +332,11 @@ def get_openapi_parameters( def get_openapi_return_values(endpoint: str, method: str) -> Dict[str, Any]: operation = openapi_spec.openapi()["paths"][endpoint][method.lower()] schema = operation["responses"]["200"]["content"]["application/json"]["schema"] - # In cases where we have used oneOf, the schemas only differ in examples - # So we can choose any. - if "oneOf" in schema: - schema = schema["oneOf"][0] + # We do not currently have documented endpoints that have multiple schemas + # ("oneOf", "anyOf", "allOf") for success ("200") responses. If this changes, + # then the assertion below will need to be removed, and this function updated + # so that endpoint responses will be rendered as expected. + assert "properties" in schema return schema["properties"] @@ -390,7 +391,7 @@ def validate_against_openapi_schema( return True # Check if the response matches its code if status_code.startswith("2") and ( - content.get("result", "success").lower() not in ["success", "partially_completed"] + content.get("result", "success").lower() != "success" ): # nocoverage raise SchemaError("Response is not 200 but is validating against 200 schema") # Code is not declared but appears in various 400 responses. If diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index c0932a6777..5c11cea689 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -4829,38 +4829,50 @@ paths: description: | Marks all of the current user's unread messages as read. - **Changes**: Before Zulip 6.0 (feature level 153), this - request did a single atomic operation, which could time out - with 10,000s of unread messages to mark as read. + Because this endpoint marks messages as read in batches, it is possible + for the request to time out after only marking some messages as read. + When this happens, the `complete` boolean field in the success response + will be `false`. Clients should repeat the request when handling such a + response. If all messages were marked as read, then the success response + will return `"complete": true`. - It now marks messages as read in batches, starting with the - newest messages, so that progress will be made even if the - request times out. + **Changes**: Before Zulip 8.0 (feature level 211), if the server's + processing was interrupted by a timeout, but some messages were marked + as read, then it would return `"result": "partially_completed"`, along + with a `code` field for an error string, in the success response to + indicate that there was a timeout and that the client should repeat the + request. - If the server's processing is interrupted by a timeout, it - will return an HTTP 200 success response with result - "partially_completed". A correct client should repeat the - request when handling such a response. + Before Zulip 6.0 (feature level 153), this request did a single atomic + operation, which could time out with 10,000s of unread messages to mark + as read. As of this feature level, messages are marked as read in + batches, starting with the newest messages, so that progress is made + even if the request times out. And, instead of returning an error when + the request times out and some messages have been marked as read, a + success response with `"result": "partially_completed"` is returned. responses: "200": - description: Success or partial success. + description: Success. content: application/json: schema: - oneOf: - - $ref: "#/components/schemas/JsonSuccess" - - allOf: - - $ref: "#/components/schemas/PartiallyCompleted" - - example: - { - "code": "REQUEST_TIMEOUT", - "msg": "", - "result": "partially_completed", - } + allOf: + - $ref: "#/components/schemas/JsonSuccessBase" + - additionalProperties: false + required: + - complete + properties: + result: {} + msg: {} + ignored_parameters_unsupported: {} + complete: + type: boolean description: | - If the request exceeds its processing time limit after having - successfully marked some messages as read, response code 200 - with result "partially_completed" and code "REQUEST_TIMEOUT" will be returned like this: + Whether all unread messages were marked as read. + + Will be `false` if the request successfully marked + some, but not all, messages as read. + example: {"msg": "", "result": "success", "complete": true} /mark_stream_as_read: post: operationId: mark-stream-as-read @@ -16247,19 +16259,34 @@ paths: description: | Delete all messages in a topic. - Topics are a field on messages (not an independent - data structure), so deleting all the messages in the topic - deletes the topic from Zulip. + Topics are a field on messages (not an independent data structure), so + deleting all the messages in the topic deletes the topic from Zulip. - **Changes**: As of Zulip 6.0 (feature level 154), in case - of timeout, a success response with "partially_completed" - result will now be returned. + Because this endpoint deletes messages in batches, it is possible for + the request to time out after only deleting some messages in the topic. + When this happens, the `complete` boolean field in the success response + will be `false`. Clients should repeat the request when handling such a + response. If all messages in the topic were deleted, then the success + response will return `"complete": true`. - Before Zulip 6.0 (feature level 147), this request did a - single atomic operation, which could time out for very large - topics. It now deletes messages in batches, starting with - the newest messages, so that progress will be made even if - the request times out. + **Changes**: Before Zulip 8.0 (feature level 211), if the server's + processing was interrupted by a timeout, but some messages in the topic + were deleted, then it would return `"result": "partially_completed"`, + along with a `code` field for an error string, in the success response + to indicate that there was a timeout and that the client should repeat + the request. + + As of Zulip 6.0 (feature level 154), instead of returning an error + response when a request times out after successfully deleting some of + the messages in the topic, a success response is returned with + `"result": "partially_completed"` to indicate that some messages were + deleted. + + Before Zulip 6.0 (feature level 147), this request did a single atomic + operation, which could time out for very large topics. As of this + feature level, messages are deleted in batches, starting with the newest + messages, so that progress is made even if the request times out and + returns an error. x-requires-administrator: true parameters: - $ref: "#/components/parameters/StreamIdInPath" @@ -16273,24 +16300,27 @@ paths: required: true responses: "200": - description: Success or partial success. + description: Success. content: application/json: schema: - oneOf: - - $ref: "#/components/schemas/JsonSuccess" - - allOf: - - $ref: "#/components/schemas/PartiallyCompleted" - - example: - { - "code": "REQUEST_TIMEOUT", - "msg": "", - "result": "partially_completed", - } + allOf: + - $ref: "#/components/schemas/JsonSuccessBase" + - additionalProperties: false + required: + - complete + properties: + result: {} + msg: {} + ignored_parameters_unsupported: {} + complete: + type: boolean description: | - If the request exceeds its processing time limit after having - successfully marked some messages as read, response code 200 - with result "partially_completed" and code "REQUEST_TIMEOUT" will be returned like this: + Whether all unread messages were marked as read. + + Will be `false` if the request successfully marked + some, but not all, messages as read. + example: {"msg": "", "result": "success", "complete": true} "400": description: Bad request. content: diff --git a/zerver/tests/test_message_flags.py b/zerver/tests/test_message_flags.py index 0124b051b4..d6651b4796 100644 --- a/zerver/tests/test_message_flags.py +++ b/zerver/tests/test_message_flags.py @@ -66,7 +66,8 @@ class FirstUnreadAnchorTests(ZulipTestCase): # Mark all existing messages as read with timeout_mock("zerver.views.message_flags"): result = self.client_post("/json/mark_all_as_read") - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) # Send a new message (this will be unread) new_message_id = self.send_stream_message(self.example_user("othello"), "Verona", "test") @@ -126,7 +127,8 @@ class FirstUnreadAnchorTests(ZulipTestCase): with timeout_mock("zerver.views.message_flags"): result = self.client_post("/json/mark_all_as_read") - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) new_message_id = self.send_stream_message(self.example_user("othello"), "Verona", "test") @@ -674,7 +676,8 @@ class MarkAllAsReadEndpointTest(ZulipTestCase): self.assertNotEqual(unread_count, 0) with timeout_mock("zerver.views.message_flags"): result = self.client_post("/json/mark_all_as_read", {}) - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) new_unread_count = ( UserMessage.objects.filter(user_profile=hamlet) @@ -687,12 +690,8 @@ class MarkAllAsReadEndpointTest(ZulipTestCase): self.login("hamlet") with mock.patch("zerver.views.message_flags.timeout", side_effect=TimeoutExpiredError): result = self.client_post("/json/mark_all_as_read", {}) - self.assertEqual(result.status_code, 200) - - result_dict = orjson.loads(result.content) - self.assertEqual( - result_dict, {"result": "partially_completed", "msg": "", "code": "REQUEST_TIMEOUT"} - ) + result_dict = self.assert_json_success(result) + self.assertFalse(result_dict["complete"]) class GetUnreadMsgsTest(ZulipTestCase): diff --git a/zerver/tests/test_message_topics.py b/zerver/tests/test_message_topics.py index 45ec739b72..ec65b21d28 100644 --- a/zerver/tests/test_message_topics.py +++ b/zerver/tests/test_message_topics.py @@ -1,6 +1,5 @@ from unittest import mock -import orjson from django.utils.timezone import now as timezone_now from zerver.actions.streams import do_change_stream_permission @@ -264,7 +263,8 @@ class TopicDeleteTest(ZulipTestCase): "topic_name": topic_name, }, ) - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) self.assertTrue(Message.objects.filter(id=last_msg_id).exists()) # Try to delete all messages in the topic again. There are no messages accessible @@ -275,7 +275,8 @@ class TopicDeleteTest(ZulipTestCase): "topic_name": topic_name, }, ) - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) self.assertTrue(Message.objects.filter(id=last_msg_id).exists()) # Make the stream's history public to subscribers @@ -294,7 +295,8 @@ class TopicDeleteTest(ZulipTestCase): "topic_name": topic_name, }, ) - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) self.assertFalse(Message.objects.filter(id=last_msg_id).exists()) self.assertTrue(Message.objects.filter(id=initial_last_msg_id).exists()) @@ -306,7 +308,8 @@ class TopicDeleteTest(ZulipTestCase): "topic_name": topic_name, }, ) - self.assert_json_success(result) + result_dict = self.assert_json_success(result) + self.assertTrue(result_dict["complete"]) self.assertFalse(Message.objects.filter(id=last_msg_id).exists()) self.assertTrue(Message.objects.filter(id=initial_last_msg_id).exists()) @@ -329,9 +332,5 @@ class TopicDeleteTest(ZulipTestCase): "topic_name": topic_name, }, ) - self.assertEqual(result.status_code, 200) - - result_dict = orjson.loads(result.content) - self.assertEqual( - result_dict, {"result": "partially_completed", "msg": "", "code": "REQUEST_TIMEOUT"} - ) + result_dict = self.assert_json_success(result) + self.assertFalse(result_dict["complete"]) diff --git a/zerver/views/message_flags.py b/zerver/views/message_flags.py index d366fa899d..808f05b8f0 100644 --- a/zerver/views/message_flags.py +++ b/zerver/views/message_flags.py @@ -8,7 +8,7 @@ from zerver.actions.message_flags import ( do_mark_stream_messages_as_read, do_update_message_flags, ) -from zerver.lib.exceptions import ErrorCode, JsonableError +from zerver.lib.exceptions import JsonableError from zerver.lib.narrow import ( OptionalNarrowListT, fetch_messages, @@ -16,7 +16,7 @@ from zerver.lib.narrow import ( parse_anchor_value, ) from zerver.lib.request import REQ, RequestNotes, has_request_variables -from zerver.lib.response import json_partial_success, json_success +from zerver.lib.response import json_success from zerver.lib.streams import access_stream_by_id from zerver.lib.timeout import TimeoutExpiredError, timeout from zerver.lib.topic import user_message_exists_for_topic @@ -123,13 +123,13 @@ def mark_all_as_read(request: HttpRequest, user_profile: UserProfile) -> HttpRes try: count = timeout(50, lambda: do_mark_all_as_read(user_profile)) except TimeoutExpiredError: - return json_partial_success(request, data={"code": ErrorCode.REQUEST_TIMEOUT.name}) + return json_success(request, data={"complete": False}) log_data_str = f"[{count} updated]" assert request_notes.log_data is not None request_notes.log_data["extra"] = log_data_str - return json_success(request) + return json_success(request, data={"complete": True}) @has_request_variables diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 5c0cf8dc08..b43895a80f 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -48,14 +48,13 @@ from zerver.decorator import ( ) from zerver.lib.default_streams import get_default_stream_ids_for_realm from zerver.lib.exceptions import ( - ErrorCode, JsonableError, OrganizationOwnerRequiredError, ResourceNotFoundError, ) from zerver.lib.mention import MentionBackend, silent_mention_syntax_for_user from zerver.lib.request import REQ, has_request_variables -from zerver.lib.response import json_partial_success, json_success +from zerver.lib.response import json_success from zerver.lib.retention import STREAM_MESSAGE_BATCH_SIZE as RETENTION_STREAM_MESSAGE_BATCH_SIZE from zerver.lib.retention import parse_message_retention_days from zerver.lib.stream_traffic import get_streams_traffic @@ -965,9 +964,9 @@ def delete_in_topic( try: timeout(50, delete_in_batches) except TimeoutExpiredError: - return json_partial_success(request, data={"code": ErrorCode.REQUEST_TIMEOUT.name}) + return json_success(request, data={"complete": False}) - return json_success(request) + return json_success(request, data={"complete": True}) @require_post