From 05108760f68dc97079ebf1e6747defee326feb8b Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Mon, 27 Jan 2020 21:37:25 -0800 Subject: [PATCH] narrow: Add support for passing oldest/newest for anchor. A wart that has long been present inin Zulip's get_messages API is how to request "the latest messages" in the API. Previously, the recommendation was basically to pass anchor=10000000000000000 (for an appropriately huge number). An accident of the server's implementation meant that specific number of 0s was actually important to avoid a buggy (or at least wasteful) value of found_newest=False if the query had specified num_after=0 (since we didn't check). This was the cause of the mobile issue https://github.com/zulip/zulip-mobile/issues/3654. The solution is to allow passing a special value of anchor='newest', basically a special string-type value that the server can interpret as meaning the user precisely just wants the most recent messages. We also add an analogous anchor='oldest' or similar to avoid folks needing to write a somewhat ugly anchor=0 for fetching the very first messages. We may want to also replace the use_first_unread_anchor argument to be a "first_unread" value for the anchor parameter. While it's not always ideal to make a value have a variable type like this, in this case it seems like a really clean way to express the idea of what the user is asking for in the API. --- zerver/lib/test_classes.py | 5 +++-- zerver/openapi/zulip.yaml | 4 +++- zerver/tests/test_messages.py | 8 ++++++++ zerver/tests/test_narrow.py | 34 ++++++++++++++++++++++++++++++++++ zerver/views/messages.py | 24 +++++++++++++++++++++++- 5 files changed, 71 insertions(+), 4 deletions(-) diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index e9b3e0b262..01a8afc6f3 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -489,7 +489,8 @@ class ZulipTestCase(TestCase): realm=recipient_realm, ) - def get_messages_response(self, anchor: int=1, num_before: int=100, num_after: int=100, + def get_messages_response(self, anchor: Union[int, str]=1, + num_before: int=100, num_after: int=100, use_first_unread_anchor: bool=False) -> Dict[str, List[Dict[str, Any]]]: post_params = {"anchor": anchor, "num_before": num_before, "num_after": num_after, @@ -498,7 +499,7 @@ class ZulipTestCase(TestCase): data = result.json() return data - def get_messages(self, anchor: int=1, num_before: int=100, num_after: int=100, + def get_messages(self, anchor: Union[str, int]=1, num_before: int=100, num_after: int=100, use_first_unread_anchor: bool=False) -> List[Dict[str, Any]]: data = self.get_messages_response(anchor, num_before, num_after, use_first_unread_anchor) return data['messages'] diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 5b63569cfd..f5b6a5a452 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -291,7 +291,9 @@ paths: description: The message ID to fetch messages near. Required unless `use_first_unread_anchor` is set to true. schema: - type: integer + oneOf: + - type: string + - type: integer example: 42 - name: use_first_unread_anchor in: query diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index d8cd347e2f..30e4864b2e 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -1373,6 +1373,14 @@ class MessageDictTest(ZulipTestCase): self.assert_json_error( result, "Missing 'anchor' argument (or set 'use_first_unread_anchor'=True).") + def test_invalid_anchor(self) -> None: + self.login(self.example_email("hamlet")) + result = self.client_get( + '/json/messages?use_first_unread_anchor=false&num_before=1&num_after=1&anchor=chocolate') + + self.assert_json_error( + result, "Invalid anchor") + class SewMessageAndReactionTest(ZulipTestCase): def test_sew_messages_and_reaction(self) -> None: sender = self.example_user('othello') diff --git a/zerver/tests/test_narrow.py b/zerver/tests/test_narrow.py index c1a190b8d7..0ed28e2ba2 100644 --- a/zerver/tests/test_narrow.py +++ b/zerver/tests/test_narrow.py @@ -2076,6 +2076,29 @@ class GetOldMessagesTest(ZulipTestCase): self.assertEqual(data['found_newest'], False) self.assertEqual(data['history_limited'], False) + # Verify that with anchor=-1 we always get found_oldest=True + # anchor=-1 is arguably invalid input, but it used to be supported + with first_visible_id_as(0): + data = self.get_messages_response(anchor=-1, num_before=0, num_after=5) + + messages = data['messages'] + messages_matches_ids(messages, message_ids[0:5]) + self.assertEqual(data['found_anchor'], False) + self.assertEqual(data['found_oldest'], True) + self.assertEqual(data['found_newest'], False) + self.assertEqual(data['history_limited'], False) + + # And anchor='first' does the same thing. + with first_visible_id_as(0): + data = self.get_messages_response(anchor='oldest', num_before=0, num_after=5) + + messages = data['messages'] + messages_matches_ids(messages, message_ids[0:5]) + self.assertEqual(data['found_anchor'], False) + self.assertEqual(data['found_oldest'], True) + self.assertEqual(data['found_newest'], False) + self.assertEqual(data['history_limited'], False) + data = self.get_messages_response(anchor=message_ids[5], num_before=5, num_after=4) messages = data['messages'] @@ -2163,6 +2186,17 @@ class GetOldMessagesTest(ZulipTestCase): self.assertEqual(data['found_newest'], True) self.assertEqual(data['history_limited'], False) + # The anchor value of 'last' behaves just like LARGER_THAN_MAX_MESSAGE_ID. + with first_visible_id_as(0): + data = self.get_messages_response(anchor='newest', num_before=5, num_after=0) + + messages = data['messages'] + self.assert_length(messages, 5) + self.assertEqual(data['found_anchor'], False) + self.assertEqual(data['found_oldest'], False) + self.assertEqual(data['found_newest'], True) + self.assertEqual(data['history_limited'], False) + with first_visible_id_as(0): data = self.get_messages_response(anchor=LARGER_THAN_MAX_MESSAGE_ID + 1, num_before=5, num_after=0) diff --git a/zerver/views/messages.py b/zerver/views/messages.py index c1e88b018a..09d340e82e 100644 --- a/zerver/views/messages.py +++ b/zerver/views/messages.py @@ -764,15 +764,37 @@ def zcommand_backend(request: HttpRequest, user_profile: UserProfile, command: str=REQ('command')) -> HttpResponse: return json_success(process_zcommands(command, user_profile)) +def parse_anchor_value(anchor_val: Optional[str]) -> Optional[int]: + if anchor_val is None: + return None + if anchor_val == "oldest": + return 0 + if anchor_val == "newest": + return LARGER_THAN_MAX_MESSAGE_ID + try: + # We don't use `.isnumeric()` to support negative numbers for + # anchor. We don't recommend it in the API (if you want the + # very first message, use 0 or 1), but it used to be supported + # and was used by the webapp, so we need to continue + # supporting it for backwards-compatibility + anchor = int(anchor_val) + if anchor < 0: + return 0 + return anchor + except ValueError: + raise JsonableError(_("Invalid anchor")) + @has_request_variables def get_messages_backend(request: HttpRequest, user_profile: UserProfile, - anchor: Optional[int]=REQ(converter=int, default=None), + anchor_val: Optional[str]=REQ( + 'anchor', str_validator=check_string, default=None), num_before: int=REQ(converter=to_non_negative_int), num_after: int=REQ(converter=to_non_negative_int), narrow: OptionalNarrowListT=REQ('narrow', converter=narrow_parameter, default=None), use_first_unread_anchor: bool=REQ(validator=check_bool, default=False), client_gravatar: bool=REQ(validator=check_bool, default=False), apply_markdown: bool=REQ(validator=check_bool, default=True)) -> HttpResponse: + anchor = parse_anchor_value(anchor_val) if anchor is None and not use_first_unread_anchor: return json_error(_("Missing 'anchor' argument (or set 'use_first_unread_anchor'=True).")) if num_before + num_after > MAX_MESSAGES_PER_FETCH: