Skip to content

Commit c6d17eb

Browse files
ashjordaashjorda
authored
fixed docs error, max parameter in messages api, and pagination logic (#261)
See PAGINATION_FIX_README.md in this branch repo.
2 parents 903e74a + efe90bb commit c6d17eb

File tree

4 files changed

+417
-21
lines changed

4 files changed

+417
-21
lines changed

‎PAGINATION_FIX_README.md

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
# Pagination Fix for Webex Python SDK
2+
3+
## Overview
4+
5+
This fix addresses an issue with the `max` parameter in the `list_messages()` function and other list methods where the parameter wasn't being properly preserved across pagination requests.
6+
7+
## Problem Description
8+
9+
The original implementation had a flaw in the `_fix_next_url` function in `src/webexpythonsdk/restsession.py`. When handling pagination:
10+
11+
1. **Webex API behavior**: Webex returns "next" URLs in Link headers that may not include all original query parameters
12+
2. **Parameter loss**: Critical parameters like `max`, `roomId`, `parentId`, etc. could be lost or modified during pagination
13+
3. **Inconsistent results**: This led to unpredictable pagination behavior and inconsistent page sizes
14+
15+
## Solution Implemented
16+
17+
The fix improves the `_fix_next_url` function to:
18+
19+
1. **Always preserve critical parameters**: Parameters like `max`, `roomId`, `parentId`, `mentionedPeople`, `before`, and `beforeMessage` are always preserved with their original values
20+
2. **Remove problematic parameters**: The `max=null` parameter (a known Webex API issue) is properly removed
21+
3. **Smart parameter handling**: Non-critical parameters are preserved from the next URL if they exist, or added if they don't
22+
4. **Consistent pagination**: Ensures the `max` parameter maintains consistent page sizes across all pagination requests
23+
24+
## Files Modified
25+
26+
- `src/webexpythonsdk/restsession.py` - Updated `_fix_next_url` function
27+
28+
## Testing
29+
30+
### Option 1: Run the Simple Test Runner
31+
32+
```bash
33+
python test_pagination_fix_runner.py
34+
```
35+
36+
This script tests the fix without requiring pytest and provides clear output about what's working.
37+
38+
### Option 2: Run with Pytest
39+
40+
```bash
41+
# Install pytest if you don't have it
42+
pip install pytest
43+
44+
# Run the comprehensive test suite
45+
pytest tests/test_pagination_fix.py -v
46+
```
47+
48+
### Option 3: Test the Fix Manually
49+
50+
You can test the fix manually by examining how the `_fix_next_url` function behaves:
51+
52+
```python
53+
from webexpythonsdk.restsession import _fix_next_url
54+
55+
# Test case 1: Remove max=null and preserve original max
56+
next_url = "https://webexapis.com/v1/messages?max=null&roomId=123"
57+
params = {"max": 10, "roomId": "123"}
58+
fixed_url = _fix_next_url(next_url, params)
59+
print(f"Fixed URL: {fixed_url}")
60+
61+
# Test case 2: Preserve critical parameters
62+
next_url = "https://webexapis.com/v1/messages?max=5&roomId=456"
63+
params = {"max": 10, "roomId": "123", "parentId": "parent123"}
64+
fixed_url = _fix_next_url(next_url, params)
65+
print(f"Fixed URL: {fixed_url}")
66+
```
67+
68+
## What the Fix Ensures
69+
70+
1. **Consistent Page Sizes**: The `max` parameter will always be applied consistently across all pagination requests
71+
2. **Parameter Preservation**: Critical parameters are never lost during pagination
72+
3. **Backward Compatibility**: Non-critical parameters are handled the same way as before
73+
4. **Robust Pagination**: The pagination behavior is now predictable and reliable
74+
75+
## Impact on Existing Code
76+
77+
This fix is **backward compatible** and doesn't change the public API. It only improves the internal pagination logic to ensure that:
78+
79+
- `list_messages(roomId="123", max=10)` will consistently return pages of 10 messages
80+
- `list_rooms(max=5)` will consistently return pages of 5 rooms
81+
- All other list methods will maintain consistent page sizes
82+
83+
## Verification
84+
85+
After applying the fix, you should see:
86+
87+
1. **Consistent page sizes**: Each page returns the expected number of items (up to the max limit)
88+
2. **Proper parameter preservation**: All specified parameters are maintained across pagination
89+
3. **No more max=null issues**: The problematic `max=null` parameter is properly handled
90+
4. **Predictable behavior**: Pagination works the same way every time
91+
92+
## Example Before/After
93+
94+
### Before (Problematic):
95+
```
96+
Page 1: 10 messages (max=10)
97+
Page 2: 50 messages (max=null - default behavior)
98+
Page 3: 50 messages (max=null - default behavior)
99+
```
100+
101+
### After (Fixed):
102+
```
103+
Page 1: 10 messages (max=10)
104+
Page 2: 10 messages (max=10)
105+
Page 3: 10 messages (max=10)
106+
```
107+
108+
## Support
109+
110+
If you encounter any issues with this fix or have questions about the implementation, please:
111+
112+
1. Run the test suite to verify the fix is working
113+
2. Check that your pagination calls are now returning consistent results
114+
3. Ensure that the `max` parameter is being respected across all pages
115+
116+
The fix addresses the root cause of the pagination issue and should resolve the problem where the `max` parameter wasn't being implemented correctly in the `list_messages()` function.

‎docs/contributing.rst

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -46,41 +46,41 @@ Contributing Code
4646

4747
.. code-block:: bash
4848
49-
python -m venv .venv
50-
source .venv/bin/activate
49+
python3 -m venv venv
50+
source venv/bin/activate
5151
52-
4. Use the ``setup`` target to install the project dependencies and setup your environment for development.
52+
4. Install poetry.
5353

5454
.. code-block:: bash
5555
56-
make setup
56+
pip install poetry
5757
58-
5. Install the SDK in Editable Mode.
58+
5. Use the ``setup`` target to install the project dependencies and setup your environment for development.
5959

6060
.. code-block:: bash
6161
62-
pip install -e
62+
make setup
6363
64-
5. Add your code to your forked repository.
64+
6. Add your code to your forked repository.
6565

6666
If you are creating some new feature or functionality (excellent!), please also write tests to verify that your code works as expected.
6767

68-
6. Please format your code and make sure your code passes the linter.
68+
7. Please format your code and make sure your code passes the linter.
6969

7070
.. code-block:: bash
7171
7272
make format
7373
make lint
7474
75-
7. If you running the test suite locally, ensure your code passes all of the default tests. Use the ``test`` target and ensure all tests execute successfully.
75+
8. If you running the test suite locally, ensure your code passes all of the default tests. Use the ``test`` target and ensure all tests execute successfully.
7676

7777
.. code-block:: bash
7878
7979
make tests
8080
81-
8. Commit your changes.
81+
9. Commit your changes.
8282

83-
9. Submit a `pull request`_.
83+
10. Submit a `pull request`_.
8484

8585

8686
Running the Test Suite Locally
@@ -90,8 +90,7 @@ To run the test suite locally, you must configure the following environment vari
9090

9191
* ``WEBEX_ACCESS_TOKEN`` - Your test account's Webex access token.
9292

93-
* ``WEBEX_TEST_DOMAIN`` - The test suite creates some users as part of the testing process. The test suite uses this domain name as the e-mail suffix of for the user's e-mail addresses.
94-
To ensure that the developer passes all tests, the developer should use the domain name of the sandbox organization that they have created.
93+
* ``WEBEX_TEST_DOMAIN`` - The test suite creates some users as part of the testing process. The test suite uses this domain name as the e-mail suffix of for the user's e-mail addresses. To ensure that the developer passes all tests, the developer should use the domain name of the sandbox organization that they have created.
9594

9695
* ``WEBEX_TEST_ID_START`` - The test suite uses this integer as the starting number for creating test user accounts (example: "[email protected]").
9796

@@ -103,7 +102,7 @@ To ensure that the developer passes all tests, the developer should use the doma
103102
104103
#!/usr/bin/env bash
105104
export WEBEX_ACCESS_TOKEN="<test account's access token>"
106-
export WEBEX_TEST_DOMAIN="domain.com"
105+
export WEBEX_TEST_DOMAIN="<your sandbox organization domain>"
107106
export WEBEX_TEST_ID_START=42
108107
export WEBEX_TEST_FILE_URL="https://www.webex.com/content/dam/wbx/us/images/navigation/CiscoWebex-Logo_white.png"
109108

‎src/webexpythonsdk/restsession.py

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,19 @@
4949

5050
# Helper Functions
5151
def _fix_next_url(next_url, params):
52-
"""Remove max=null parameter from URL.
52+
"""Remove max=null parameter from URL and ensure critical parameters are preserved.
5353
5454
Patch for Webex Defect: "next" URL returned in the Link headers of
55-
the responses contain an errant "max=null" parameter, which causes the
55+
the responses contain an errant "max=null" parameter, which causes the
5656
next request (to this URL) to fail if the URL is requested as-is.
5757
58-
This patch parses the next_url to remove the max=null parameter.
58+
This patch parses the next_url to remove the max=null parameter and
59+
ensures that critical parameters like 'max' are properly preserved
60+
across pagination requests.
5961
6062
Args:
6163
next_url(str): The "next" URL to be parsed and cleaned.
64+
params(dict): The original request parameters to ensure are preserved.
6265
6366
Returns:
6467
str: The clean URL to be used for the "next" request.
@@ -80,20 +83,46 @@ def _fix_next_url(next_url, params):
8083

8184
if parsed_url.query:
8285
query_list = parsed_url.query.split("&")
86+
87+
# Remove the problematic max=null parameter
8388
if "max=null" in query_list:
8489
query_list.remove("max=null")
8590
warnings.warn(
86-
"`max=null` still present in next-URL returned " "from Webex",
91+
"`max=null` still present in next-URL returned from Webex",
8792
RuntimeWarning,
8893
stacklevel=1,
8994
)
95+
96+
# Parse existing query parameters into a dict for easier manipulation
97+
existing_params = {}
98+
for param in query_list:
99+
if "=" in param:
100+
key, value = param.split("=", 1)
101+
existing_params[key] = value
102+
103+
# Ensure critical parameters from the original request are preserved
90104
if params:
91105
for k, v in params.items():
92-
if not any(p.startswith("{}=".format(k)) for p in query_list):
93-
query_list.append("{}={}".format(k, v))
94-
new_query = "&".join(query_list)
106+
# Always preserve critical parameters like 'max' to maintain consistent pagination
107+
if k in ['max', 'roomId', 'parentId', 'mentionedPeople', 'before', 'beforeMessage']:
108+
existing_params[k] = str(v)
109+
# For other parameters, only add if they don't exist
110+
elif k not in existing_params:
111+
existing_params[k] = str(v)
112+
113+
# Rebuild the query string
114+
new_query_list = [f"{k}={v}" for k, v in existing_params.items()]
115+
new_query = "&".join(new_query_list)
116+
95117
parsed_url = list(parsed_url)
96118
parsed_url[4] = new_query
119+
else:
120+
# No query parameters in next_url, add all params
121+
if params:
122+
new_query_list = [f"{k}={v}" for k, v in params.items()]
123+
new_query = "&".join(new_query_list)
124+
parsed_url = list(parsed_url)
125+
parsed_url[4] = new_query
97126

98127
return urllib.parse.urlunparse(parsed_url)
99128

0 commit comments

Comments
 (0)