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

Feature: Add support for paging #70

Merged

Conversation

replicaJunction
Copy link
Contributor

This is a cleaner re-implementation of #64. Also addresses #19.

  • Add support for the sysparm_offet URL parameter, which was not previously supported. This is exposed via PowerShell's Skip parameter.
  • Add support for PowerShell's -First parameter. This duplicates the existing functionality of the Limit parameter.
  • Add a warning message that the Limit parameter is deprecated and may be removed.
  • Remove default values from Limit, so that the above warning is only generated when the user explicitly passes the parameter.

Default behavior has not been modified - if the user does not explicitly pass either First, Limit, or Skip, the first 10 records are returned.

If the user passes both First and Limit, the value of the Limit parameter is used, and the value of the First parameter is discarded. This is to preserve compatibility with scripts that currently set the Limit parameter.

Here are a couple of usage examples:

# Assume that $splat is a variable containing filtering information - MatchContains, MatchExact, etc.

# Get the first 10 records (identical to current behavior)
PS> Get-ServiceNowChangeRequest @splat

# Get the next 10 records after the above - new functionality
PS> Get-ServiceNowChangeRequest @splat -Skip 10

# Get the first 20 records
PS> Get-ServiceNowChangeRequest @splat -First 20

# Get the first 20 records using the Limit param - this will generate a warning message, but will work as expected
PS> Get-ServiceNowChangeRequest @splat -Limit 20

# Simple loop to get all change requests using paging
# Note - this is a very simple example, and there are a lot of ways it could be improved (like with Write-Progress)
$stillGoing = $true
$first = 10
$skip = 0
while ($stillGoing) {
    $currentResults = Get-ServiceNowChangeRequest @splat -First $first -Skip $skip
    # We've returned $first records, so add that to our $skip offset for the next run
    $skip += $first

    # If we got back fewer results than we expected, assume we're done
    # n.b. I'm sure there are better ways to detect whether we're done than this, but again, this is a simple example
    if ($currentResults.Count -lt $first) {
        Write-Verbose "Only $($currentResults.Count) results were returned when $first were expected; assuming we've hit the end"
        $stillGoing = $false
    }
    Write-Output $currentResults
}

Feel free to let me know if you've got any questions or would like to see any changes.

@Rick-2CA Rick-2CA self-assigned this Feb 7, 2019
@Rick-2CA
Copy link
Member

Rick-2CA commented Feb 7, 2019

It appears that the IncludeTotalCount parameter and the NewTotalCount method internally don't do anything for us, right? Does that feature make you lean towards moving to Invoke-WebRequest and I should be researching the pros/cons of such a decision?

Do we want to offer a paging solution of some sort in a future PR or is that something typically left up to others?

@replicaJunction
Copy link
Contributor Author

Yup, right now passing IncludeTotalCount wouldn't really accomplish anything for the user right now. Because the NewTotalCount method includes a double for "certainty," though, at least the user isn't misled into thinking that they're getting useful information out of it. :)

In my personal opinion, I've found it to be useful to develop a private function that wraps Invoke-WebRequest for REST APIs like this. That makes it a lot easier to make changes like switching between Invoke-RestMethod, Invoke-WebRequest, or using the .NET classes directly without having to touch everything in the module. It also allows you to handle some common API errors gracefully - for example, Invoke-RestMethod writes an error message on any status code 400+, but if we get a 404, maybe it's better just to write a verbose message that no data was returned. (Just an example, not an actual suggestion.)

In the long run, that would be the direction I'd suggest, and yes, I'd probably have it use Invoke-WebRequest under the hood. I don't think that's a decision that needs to be made immediately, though.

I could go either way on offering an all-in-one paging solution. I don't think it's necessary - this module provides the tools, but it's up to individual devs to write the controllers - but it could be a convenient feature to provide a switch that returns all data instead of just the first page.

@Rick-2CA
Copy link
Member

Rick-2CA commented Feb 8, 2019

Good deal. There's a small chance I can get this merged, tested, and published this afternoon. Otherwise i'll have to try early next week.

Thanks for your work.

@Rick-2CA Rick-2CA merged commit c24ea5b into Snow-Shell:development Feb 8, 2019
@replicaJunction replicaJunction deleted the feature-paging-rewrite branch March 21, 2019 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants