Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/pagination next key #333

Merged
merged 3 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

All notable changes to this project will be documented in this file.

## [1.5.3] - 2024-06-12
### Changed
- Changed parameter `key` from the PaginationOption class.
- Fixed an error when using the next page key in PaginationOption, causing incorrect pagination responses.

## [1.5.2] - 2024-05-10
### Changed
- Updated `protobuf` dependency version to ">=5.26.1"
Expand Down
41 changes: 41 additions & 0 deletions examples/chain_client/9_PaginatedRequestExample.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import asyncio

from pyinjective.async_client import AsyncClient
from pyinjective.client.model.pagination import PaginationOption
from pyinjective.core.network import Network


async def main() -> None:
network = Network.testnet()
client = AsyncClient(network)
pagination = PaginationOption(
limit=10,
count_total=True,
)
first_result = await client.fetch_total_supply(
pagination=pagination,
)
print(f"First result:\n{first_result}")

next_page_key = first_result["pagination"]["nextKey"]

for i in range(5):
if next_page_key == "":
break

next_request_pagination = PaginationOption(
limit=pagination.limit,
count_total=pagination.count_total,
encoded_page_key=next_page_key,
)

next_result = await client.fetch_total_supply(
pagination=next_request_pagination,
)
print(f"Page {i+2} result:\n{next_result}")

next_page_key = next_result["pagination"]["nextKey"]
Comment on lines +22 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize the pagination loop to handle potential exceptions and ensure robustness.

- for i in range(5):
-     if next_page_key == "":
-         break
+ for i in range(5):
+     try:
+         if next_page_key == "":
+             break
+     except Exception as e:
+         print(f"Error during pagination: {e}")
+         break

Committable suggestion was skipped due to low confidence.



if __name__ == "__main__":
asyncio.get_event_loop().run_until_complete(main())
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using asyncio.run(main()) for a more modern approach to running asyncio programs.

- asyncio.get_event_loop().run_until_complete(main())
+ asyncio.run(main())
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if __name__ == "__main__":
asyncio.get_event_loop().run_until_complete(main())
if __name__ == "__main__":
asyncio.run(main())

6 changes: 2 additions & 4 deletions pyinjective/async_client.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import asyncio
import base64
import time
from copy import deepcopy
from decimal import Decimal
Expand Down Expand Up @@ -3064,11 +3063,10 @@ async def initialize_tokens_from_chain_denoms(self):
next_key = query_result.get("pagination", {}).get("nextKey", "")

while next_key != "":
query_result = await self.fetch_denoms_metadata(pagination=PaginationOption(key=next_key))
query_result = await self.fetch_denoms_metadata(pagination=PaginationOption(encoded_page_key=next_key))

all_denoms_metadata.extend(query_result.get("metadatas", []))
result_next_key = query_result.get("pagination", {}).get("nextKey", "")
next_key = base64.b64decode(result_next_key).decode()
next_key = query_result.get("pagination", {}).get("nextKey", "")

for token_metadata in all_denoms_metadata:
symbol = token_metadata["symbol"]
Expand Down
10 changes: 6 additions & 4 deletions pyinjective/client/model/pagination.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import base64
from typing import Optional

from pyinjective.proto.cosmos.base.query.v1beta1 import pagination_pb2 as pagination_pb
Expand All @@ -6,7 +7,7 @@
class PaginationOption:
def __init__(
self,
key: Optional[str] = None,
encoded_page_key: Optional[str] = None,
skip: Optional[int] = None,
limit: Optional[int] = None,
start_time: Optional[int] = None,
Expand All @@ -17,7 +18,7 @@ def __init__(
to_number: Optional[int] = None,
):
super().__init__()
self.key = key
self.encoded_page_key = encoded_page_key
self.skip = skip
self.limit = limit
self.start_time = start_time
Expand All @@ -30,8 +31,9 @@ def __init__(
def create_pagination_request(self) -> pagination_pb.PageRequest:
page_request = pagination_pb.PageRequest()

if self.key is not None:
page_request.key = self.key.encode()
if self.encoded_page_key is not None and self.encoded_page_key != "":
page_key = base64.b64decode(self.encoded_page_key)
page_request.key = page_key
Comment on lines +34 to +36
Copy link
Contributor

@coderabbitai coderabbitai bot Jun 11, 2024

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The create_pagination_request method was found in pyinjective/client/model/pagination.py, but there is no evidence of exception handling around the base64 decoding within this method.

  • File: pyinjective/client/model/pagination.py
  • Lines: 34-36

Please ensure to handle potential exceptions from incorrect base64 strings to avoid runtime errors.

Analysis chain

Proper handling of base64 decoding for the page key. Ensure to handle potential exceptions from incorrect base64 strings.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify exception handling for base64 decoding in `create_pagination_request`.

# Test: Search for exception handling patterns around base64 decoding.
ast-grep --lang python --pattern $'def create_pagination_request($_) {
  try {
    $$$
    base64.b64decode($_)
    $$$
  } catch ($_){
    $$$
  }
}'

Length of output: 496



Script:

#!/bin/bash
# Description: Locate the `create_pagination_request` method and check for exception handling around base64 decoding.

# Step 1: Locate the `create_pagination_request` method.
ast-grep --lang python --pattern 'def create_pagination_request($_) {
  $$$
}' > create_pagination_request_location.txt

# Step 2: Check for try-except blocks within the `create_pagination_request` method.
if [ -s create_pagination_request_location.txt ]; then
  # Extract the file path from the location result
  file_path=$(head -n 1 create_pagination_request_location.txt | cut -d: -f1)
  
  # Search for try-except blocks within the located method
  ast-grep --lang python --pattern 'def create_pagination_request($_) {
    $$$
    try:
      $$$
    except $_:
      $$$
  }' $file_path
else
  echo "Method create_pagination_request not found."
fi

Length of output: 695



Script:

#!/bin/bash
# Description: Locate the `create_pagination_request` method and check for exception handling around base64 decoding.

# Step 1: Locate the `create_pagination_request` method.
ast-grep --lang python --pattern 'def create_pagination_request($_):' > create_pagination_request_location.txt

# Step 2: Check for try-except blocks within the `create_pagination_request` method.
if [ -s create_pagination_request_location.txt ]; then
  # Extract the file path from the location result
  file_path=$(head -n 1 create_pagination_request_location.txt | cut -d: -f1)
  
  # Search for try-except blocks within the located method
  ast-grep --lang python --pattern 'try:
  $$$
except $_:
  $$$' $file_path
else
  echo "Method create_pagination_request not found."
fi

Length of output: 220



Script:

#!/bin/bash
# Description: Search for the `create_pagination_request` method and inspect for exception handling around base64 decoding.

# Step 1: Search for the `create_pagination_request` method definition.
rg 'def create_pagination_request' > create_pagination_request_location.txt

# Step 2: Check for try-except blocks within the located method.
if [ -s create_pagination_request_location.txt ]; then
  # Extract the file path from the location result
  file_path=$(head -n 1 create_pagination_request_location.txt | cut -d: -f1)
  
  # Search for try-except blocks within the located method
  rg -A 10 'def create_pagination_request' $file_path | rg 'try:|except'
else
  echo "Method create_pagination_request not found."
fi

Length of output: 307

Choose a reason for hiding this comment

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

This is a fair point, but wonder how could this happen. Think it's better to crash if some of these exceptions are risen

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolasbaum, I understand your perspective on preferring the application to crash in cases where exceptions like incorrect base64 strings occur, as it makes such issues more visible and likely to be fixed promptly.

However, it's also worth considering the user experience, especially in a production environment where such crashes could lead to disruptions in service. An alternative approach could be to log these errors comprehensively while still allowing the application to continue running. This way, the issues are documented and can be addressed without immediately impacting end users.

Would you like to explore setting up detailed logging for these exceptions, or would you prefer to proceed with the current approach where the application will crash under these circumstances? This decision can be aligned with how critical the stability of this feature is and the expected stability of the input data.

Let me know how you'd like to proceed or if there's any other aspect of the implementation you'd like to discuss!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this is a fair point. If there was an error in the base64 encoded string, the error would have happened before, when the response was converted into its JSON representation. Since we reached this point, we are sure the encoded pagination key can be decoded without issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

if self.skip is not None:
page_request.offset = self.skip
if self.limit is not None:
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "injective-py"
version = "1.5.2"
version = "1.5.3"
description = "Injective Python SDK, with Exchange API Client"
authors = ["Injective Labs <[email protected]>"]
license = "Apache-2.0"
Expand Down
2 changes: 1 addition & 1 deletion tests/client/chain/grpc/test_chain_grpc_auth_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ async def test_fetch_accounts(
api._stub = auth_servicer

pagination_option = PaginationOption(
key="011ab4075a94245dff7338e3042db5b7cc3f42e1",
encoded_page_key="011ab4075a94245dff7338e3042db5b7cc3f42e1",
skip=10,
limit=30,
reverse=False,
Expand Down
Empty file added tests/client/model/__init__.py
Empty file.
35 changes: 35 additions & 0 deletions tests/client/model/test_pagination.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import base64

from pyinjective.client.model.pagination import PaginationOption


class TestPaginationOption:
def test_create_pagination_request(self):
next_page_key = b"next page key"
encoded_next_page_key = base64.b64encode(next_page_key).decode()
pagination_option = PaginationOption(
encoded_page_key=encoded_next_page_key,
skip=5,
limit=10,
start_time=3,
end_time=4,
reverse=False,
count_total=True,
from_number=105,
to_number=135,
)

page_request = pagination_option.create_pagination_request()
assert page_request.key == next_page_key
assert page_request.offset == pagination_option.skip
assert page_request.limit == pagination_option.limit
assert not page_request.reverse
assert page_request.count_total

def test_next_key_is_none_for_empty_encoded_page_key(self):
pagination_option = PaginationOption(
encoded_page_key="",
)
page_request = pagination_option.create_pagination_request()

assert page_request.key == b""
Loading