mirror of
				https://github.com/zulip/zulip.git
				synced 2025-11-04 05:53:43 +00:00 
			
		
		
		
	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.
This commit is contained in:
		@@ -489,7 +489,8 @@ class ZulipTestCase(TestCase):
 | 
				
			|||||||
            realm=recipient_realm,
 | 
					            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]]]:
 | 
					                              use_first_unread_anchor: bool=False) -> Dict[str, List[Dict[str, Any]]]:
 | 
				
			||||||
        post_params = {"anchor": anchor, "num_before": num_before,
 | 
					        post_params = {"anchor": anchor, "num_before": num_before,
 | 
				
			||||||
                       "num_after": num_after,
 | 
					                       "num_after": num_after,
 | 
				
			||||||
@@ -498,7 +499,7 @@ class ZulipTestCase(TestCase):
 | 
				
			|||||||
        data = result.json()
 | 
					        data = result.json()
 | 
				
			||||||
        return data
 | 
					        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]]:
 | 
					                     use_first_unread_anchor: bool=False) -> List[Dict[str, Any]]:
 | 
				
			||||||
        data = self.get_messages_response(anchor, num_before, num_after, use_first_unread_anchor)
 | 
					        data = self.get_messages_response(anchor, num_before, num_after, use_first_unread_anchor)
 | 
				
			||||||
        return data['messages']
 | 
					        return data['messages']
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -291,7 +291,9 @@ paths:
 | 
				
			|||||||
        description: The message ID to fetch messages near.
 | 
					        description: The message ID to fetch messages near.
 | 
				
			||||||
          Required unless `use_first_unread_anchor` is set to true.
 | 
					          Required unless `use_first_unread_anchor` is set to true.
 | 
				
			||||||
        schema:
 | 
					        schema:
 | 
				
			||||||
          type: integer
 | 
					          oneOf:
 | 
				
			||||||
 | 
					            - type: string
 | 
				
			||||||
 | 
					            - type: integer
 | 
				
			||||||
        example: 42
 | 
					        example: 42
 | 
				
			||||||
      - name: use_first_unread_anchor
 | 
					      - name: use_first_unread_anchor
 | 
				
			||||||
        in: query
 | 
					        in: query
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -1373,6 +1373,14 @@ class MessageDictTest(ZulipTestCase):
 | 
				
			|||||||
        self.assert_json_error(
 | 
					        self.assert_json_error(
 | 
				
			||||||
            result, "Missing 'anchor' argument (or set 'use_first_unread_anchor'=True).")
 | 
					            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):
 | 
					class SewMessageAndReactionTest(ZulipTestCase):
 | 
				
			||||||
    def test_sew_messages_and_reaction(self) -> None:
 | 
					    def test_sew_messages_and_reaction(self) -> None:
 | 
				
			||||||
        sender = self.example_user('othello')
 | 
					        sender = self.example_user('othello')
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -2076,6 +2076,29 @@ class GetOldMessagesTest(ZulipTestCase):
 | 
				
			|||||||
        self.assertEqual(data['found_newest'], False)
 | 
					        self.assertEqual(data['found_newest'], False)
 | 
				
			||||||
        self.assertEqual(data['history_limited'], 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)
 | 
					        data = self.get_messages_response(anchor=message_ids[5], num_before=5, num_after=4)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        messages = data['messages']
 | 
					        messages = data['messages']
 | 
				
			||||||
@@ -2163,6 +2186,17 @@ class GetOldMessagesTest(ZulipTestCase):
 | 
				
			|||||||
        self.assertEqual(data['found_newest'], True)
 | 
					        self.assertEqual(data['found_newest'], True)
 | 
				
			||||||
        self.assertEqual(data['history_limited'], False)
 | 
					        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):
 | 
					        with first_visible_id_as(0):
 | 
				
			||||||
            data = self.get_messages_response(anchor=LARGER_THAN_MAX_MESSAGE_ID + 1,
 | 
					            data = self.get_messages_response(anchor=LARGER_THAN_MAX_MESSAGE_ID + 1,
 | 
				
			||||||
                                              num_before=5, num_after=0)
 | 
					                                              num_before=5, num_after=0)
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -764,15 +764,37 @@ def zcommand_backend(request: HttpRequest, user_profile: UserProfile,
 | 
				
			|||||||
                     command: str=REQ('command')) -> HttpResponse:
 | 
					                     command: str=REQ('command')) -> HttpResponse:
 | 
				
			||||||
    return json_success(process_zcommands(command, user_profile))
 | 
					    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
 | 
					@has_request_variables
 | 
				
			||||||
def get_messages_backend(request: HttpRequest, user_profile: UserProfile,
 | 
					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_before: int=REQ(converter=to_non_negative_int),
 | 
				
			||||||
                         num_after: 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),
 | 
					                         narrow: OptionalNarrowListT=REQ('narrow', converter=narrow_parameter, default=None),
 | 
				
			||||||
                         use_first_unread_anchor: bool=REQ(validator=check_bool, default=False),
 | 
					                         use_first_unread_anchor: bool=REQ(validator=check_bool, default=False),
 | 
				
			||||||
                         client_gravatar: 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:
 | 
					                         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:
 | 
					    if anchor is None and not use_first_unread_anchor:
 | 
				
			||||||
        return json_error(_("Missing 'anchor' argument (or set 'use_first_unread_anchor'=True)."))
 | 
					        return json_error(_("Missing 'anchor' argument (or set 'use_first_unread_anchor'=True)."))
 | 
				
			||||||
    if num_before + num_after > MAX_MESSAGES_PER_FETCH:
 | 
					    if num_before + num_after > MAX_MESSAGES_PER_FETCH:
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user