Skip to content

Conversation

@bryanapellanes-okta
Copy link
Contributor

@bryanapellanes-okta bryanapellanes-okta commented Dec 7, 2023

This change adds a parameter to api_response.next() which causes the response to be returned as the third tuple value during paging operations; this provides access to rate limit headers.

Copy link
Contributor

@justinabrokwah-okta justinabrokwah-okta left a comment

Choose a reason for hiding this comment

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

LGTM! Great work

@bryanapellanes-okta bryanapellanes-okta marked this pull request as ready for review December 8, 2023 21:00
Copy link
Contributor

@justinabrokwah-okta justinabrokwah-okta left a comment

Choose a reason for hiding this comment

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

🚀

@bryanapellanes-okta bryanapellanes-okta force-pushed the OKTA-672843-include-response-for-next branch from 94208cc to 759735e Compare December 11, 2023 15:38
@bryanapellanes-okta bryanapellanes-okta force-pushed the OKTA-672843-include-response-for-next branch from 759735e to 4d1cbb3 Compare December 11, 2023 15:42
@bryanapellanes-okta bryanapellanes-okta merged commit 700c5f1 into master Dec 13, 2023
@bryanapellanes-okta bryanapellanes-okta deleted the OKTA-672843-include-response-for-next branch December 13, 2023 13:33
@gabrielsroka
Copy link
Contributor

gabrielsroka commented Dec 13, 2023

so, when u first call list_users, it's users, resp, err and the 2nd time it's users, err, resp ???

also, in the example below (from the readme), should it be resp, not response ?

from okta.client import Client as OktaClient
import asyncio

async def main():
    client = OktaClient()
    users, resp, err = await client.list_users()
    while True:
        for user in users:
            print(user.profile.login) 
        if resp.has_next():
            users, err, response = await resp.next(True) # Specify True to receive the response object as the third part of the tuple for further analysis
        else:
            break

@justinabrokwah-okta
Copy link
Contributor

@gabrielsroka Yes, at least we don't break existing scripts for users but now we have the functionality for those who requested

@gabrielsroka
Copy link
Contributor

gabrielsroka commented Dec 15, 2023

There are no existing scripts that pass in True. Plus you need to change the tuple and also actually use the response headers which means this code is going to get looked at.

Or you could just create a new method.

also, in the example (from the readme), should it be resp or response ?

@gabrielsroka
Copy link
Contributor

also, since you're adding a new feature, maybe u could take another look at the 2-line PR i submitted last year, which @justinabrokwah-okta approved, but then got rolled back

#328

@gabrielsroka
Copy link
Contributor

why is it includeResponse and not include_response?

@gabrielsroka
Copy link
Contributor

gabrielsroka commented Dec 15, 2023

in the example (from the readme), should it be resp or response ?

to answer my own q, it looks like u need 2 different variables: resp and resp2. so you can't reuse the get_headers(), either.

if i change resp2 to resp, i get a weird error, it looks like dicts aren't being converted to objects -- idk if this is a bug: AttributeError: 'dict' object has no attribute 'profile'

import okta.client
# import okta.api_response
import asyncio

async def main():
    # async with reuses the session, so it's much faster.
    async with okta.client.Client() as client:
        # Paginate users
        # resp: okta.api_response.OktaAPIResponse
        params = {'filter': 'profile.lastName eq "Doe"', 'limit': 1}
        users, resp, err = await client.list_users(params)
        print(resp.get_headers().get('x-rate-limit-remaining'))
        while True:
            for user in users:
                print(user.profile.login)
            if resp.has_next(): 
                users, err, resp2 = await resp.next(True)
                print(resp2.get_headers().get('x-rate-limit-remaining'))
            else: break

asyncio.run(main())

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.

5 participants