Skip to content

fix(rtdb): do not re-sort order_by_key() results at the client side - #982

Open
Sanjays2402 wants to merge 1 commit into
firebase:mainfrom
Sanjays2402:fix-rtdb-order-by-key
Open

fix(rtdb): do not re-sort order_by_key() results at the client side#982
Sanjays2402 wants to merge 1 commit into
firebase:mainfrom
Sanjays2402:fix-rtdb-order-by-key

Conversation

@Sanjays2402

Copy link
Copy Markdown

Fixes #677

The RTDB server returns order_by_key() results in Firebase key order (integer-like keys first, in ascending numeric order), but Query.get() re-sorted them lexicographically at the client side via _Sorter. For example, the server order ['123', '100001', '100002', '100003'] was returned to the caller as ['100001', '100002', '100003', '123'], which breaks key-based pagination: calling start_at() with the last key of the returned page yields the same page over and over.

This change skips the client-side sort for $key queries (like we already do for $priority) so the server-side ordering is preserved. This also matches the behavior of the Node Admin SDK, which performs no client-side re-sorting.

Testing: reproduced the bug with a mocked RTDB response before the fix (keys came back lexicographically sorted), and confirmed the fix preserves the server order. Added a regression test (test_order_by_key_does_not_resort_results) in tests/test_db.py; the full test_db.py suite passes (475 passed; 6 pre-existing environment-related setup errors in TestListenerRegistration also occur on the unmodified tree) and pylint rates both changed files 10.00/10.

Context Sources Used:

  • id: firebase-admin-python

The RTDB server returns order_by_key() results in Firebase key order
(integer-like keys first, in ascending numeric order). The client-side
_Sorter re-sorted them lexicographically, which mangled the order and
broke key-based pagination (e.g. with start_at()). Skip the client-side
sort for $key queries, like we already do for $priority, and preserve
the server-side ordering.

Fixes firebase#677

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request prevents client-side re-sorting of database query results when ordering by $key, ensuring the server-side Firebase key order is preserved, and adds a corresponding regression test. The reviewer pointed out that bypassing the sorter for $key and $priority queries causes Query.get() to return a standard dict instead of the documented OrderedDict in production. They suggested converting standard dictionaries to OrderedDict before returning, and updating the regression test to mock a standard dict to properly simulate production behavior.

Comment thread firebase_admin/db.py
Comment on lines +625 to 627
if isinstance(result, (dict, list)) and self._order_by not in ('$priority', '$key'):
return _Sorter(result, self._order_by).get()
return result

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In production, self._client.body() returns a standard dict (parsed from JSON by the underlying HTTP client). Because $key and $priority queries bypass _Sorter, Query.get() currently returns the result as-is, which means it returns a standard dict in production.\n\nHowever, the docstring of Query and Query.get() states that dictionary results are returned as an OrderedDict. To maintain consistency with the documented return type and ensure backward compatibility for any client code that strictly checks isinstance(..., OrderedDict), we should convert standard dict results to collections.OrderedDict before returning them.

        if isinstance(result, (dict, list)) and self._order_by not in ('$priority', '$key'):\n            return _Sorter(result, self._order_by).get()\n        if isinstance(result, dict) and not isinstance(result, collections.OrderedDict):\n            return collections.OrderedDict(result)\n        return result

Comment thread tests/test_db.py
Comment on lines +961 to +967
query._client = mock.Mock()
query._client.body.return_value = collections.OrderedDict([
('123', {'myValue': True}),
('100001', {'myValue': True}),
('100002', {'myValue': True}),
('100003', {'myValue': True}),
])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test currently mocks _client.body to return a collections.OrderedDict and then asserts assert isinstance(result, collections.OrderedDict). This is a "testing the mock" pattern, as the test only passes because the mock itself is an OrderedDict, whereas in production the real client returns a standard dict.\n\nBy changing the mock to return a standard dict (which preserves insertion order in Python 3.7+), we can realistically simulate the production environment and verify that Query.get() correctly converts it to an OrderedDict as asserted on line 970.

        query._client = mock.Mock()\n        query._client.body.return_value = {\n            '123': {'myValue': True},\n            '100001': {'myValue': True},\n            '100002': {'myValue': True},\n            '100003': {'myValue': True},\n        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Order diff between backend and client with order_by_key()

1 participant