okta / okta-sdk-python

Apache License 2.0
234 stars 143 forks source link

pagination should be easier #328

Closed gabrielsroka closed 1 year ago

gabrielsroka commented 1 year ago

currently, pagination requires many lines of code:

async with Client() as client:
    users, resp, _ = await client.list_users()
    while users:
        for user in users:
            print(user.profile.login)
        users, _ = await resp.next() if resp.has_next() else (None, None)

my 2 line PR would reduce some of the boilerplate code (the last line above ^^^) to:

        users, _ = await resp.next()

it'd be even nicer to have an async generator to either return each page, or each object in the page (eg, Users). basically take the 5 lines above and move them into the SDK itself. then use async for to get the users:

# in the SDK `UserClient` class:
async def page_users(self):
    users, resp, _ = await self.list_users()
    while users:
        for user in users:
            yield user
        users, _ = await resp.next()

# call the SDK code:
async for user in client.page_users():
    print(user.profile.login)

this is more Pythonic, and similar to how other Okta SDKs work, eg, Node.js (https://github.com/okta/okta-sdk-nodejs#serial-or-parallel-synchronous-work), and I think Java, too, but I can't tell from the example.

Node.js example https://github.com/okta/okta-sdk-nodejs#list-all-org-users

// Node.js example

// You can also use async iterators.
for await (let user of client.listUsers()) {
    console.log(user);
}

my code doesn't handle errors, but it could. error handling should use try--it's more Pythonic.

see also https://github.com/okta/okta-sdk-python/issues/326#issuecomment-1314496367

gabrielsroka commented 1 year ago

it looks like this was rolled back due to breaking tests. is there a plan to fix the tests?

https://github.com/okta/okta-sdk-python/blob/0e71c4d6e78af872ee95ac2372e1d630858277b0/okta/api_response.py#L134-L135

bretterer commented 1 year ago

Hi there! @gabrielsroka, thank you for your initial PR. Unfortunately, at this time, it is not on our roadmap to re-enable after finding it broke our tests. However, I have created an internal ticket to track this as a feature to re-enable in the future. We appreciate your feedback and will keep it in mind