SCIM routes pagination

[exageraldo]

I'm facing a issue when I try to paginate resources from the SCIM provisioned identities list (client.SCIM.ListSCIMProvisionedIdentities) and when I try to set the number of items per page.

When I added the httpdebug module, I noticed that the query strings are being sent with the first letter capitalized.

2023/02/25 20:37:23 curl -X GET \
  https://api.github.com/scim/v2/organizations/<ORG>/Users?Count=100&Filter=&StartIndex=1 \
  -H 'Accept: application/vnd.github.v3+json' \
  -H 'Authorization: <REDACTED>' \
  -H 'User-Agent: go-github/v50.0.0' \
  -H 'X-Github-Api-Version: 2022-11-28'

In the documentation the parameters start with lower case letters.

Query parameters
--
startIndex integer
count integer
filter string

And if the parameters are not sent exactly as in the documentation, the value is ignored.

This makes it impossible to paginate this resource because even if the StartIndex value is changed and sent, the value is not identified by the Github API.

[exageraldo]

When trying to do the request in a "manual" way (client.NewRequest + client.Do), the pagination worked perfectly.

func (gc *GithubClient) ManualListScimProviderUsers(organizationName string) (allSamlUsers []*github.SCIMUserAttributes, err error) {
	startIndex := 1

	req, err := gc.client.NewRequest(
		"GET",
		fmt.Sprintf("scim/v2/organizations/%s/Users", organizationName),
		nil,
	)

	q := req.URL.Query()
	q.Add("count", fmt.Sprint(gc.perPage))
	q.Add("startIndex", fmt.Sprint(startIndex))

	for {
		req.URL.RawQuery = q.Encode()

		scimList := &github.SCIMProvisionedIdentities{}
		_, err = gc.client.Do(
			context.Background(),
			req,
			scimList,
		)

		if err != nil {
			fmt.Printf("error: %v", err)
			return nil, err
		}

		allSamlUsers = append(allSamlUsers, scimList.Resources...)

		if len(scimList.Resources) < gc.perPage {
			break
		}
		startIndex += gc.perPage
		q.Set("startIndex", fmt.Sprint(startIndex))
	}

	return allSamlUsers, nil
}

[exageraldo]

I'm also having problems trying to use the caching module (github.com/gregjones/httpcache) with SCIM routes. It's not creating caches at all.

I don't know if these problems are related or not.

[gmlewis]

Hmmm... That's very odd. I'm on my phone right now, but did you search the code to see if we are capitalizing those parameters?

Also, there is another thread already discussing httpcache if you want to search for it... Hopefully you will find it useful, but I know others have had challenges using it too.

[gmlewis]

Also, I'm OOO for a week starting tonight but hopefully others will be able to help you out.

[exageraldo]

I notice that the "Filter" field is not omitted as in the other requests.
Within the ListSCIMProvisionedIdentitiesOptions struct the tags start with "json" and not "url" (check more here github.com/google/go-querystring/query) as in the other options structures (like RepositoryListOptions or CommitsListOptions).

I would suggest this solution:

type ListSCIMProvisionedIdentitiesOptions struct {
	StartIndex *int `url:"startIndex,omitempty"` // Used for pagination: the index of the first result to return. (Optional.)
	Count      *int `url:"count,omitempty"`      // Used for pagination: the number of results to return. (Optional.)
	// ...
	Filter *string `url:"filter,omitempty"`
}

I would like to ask if I can contribute to this solution.

[gmlewis]

Hmmm... I'm not seeing a reason for your query parameters to have been capitalized, and in the repo on line 79 of scim.go, I'm seeing that "Filter" already has "omitempty" on it. Are you sure you are using the latest version of this repo?

https://github.com/google/go-github/blob/master/github/scim.go#L68-L80

[exageraldo]

yeah, I'm using the latest version.

My suggestion is only to change the prefix of the fields in the ListSCIMProvisionedIdentitiesOptions structure from "json" to "url".

type ListSCIMProvisionedIdentitiesOptions struct {
	StartIndex *int `json:"startIndex,omitempty"` // Used for pagination: the index of the first result to return. (Optional.)
	Count      *int `json:"count,omitempty"`      // Used for pagination: the number of results to return. (Optional.)
	// ...
	Filter *string `json:"filter,omitempty"`
}

type ListSCIMProvisionedIdentitiesOptions struct {
	StartIndex *int `url:"startIndex,omitempty"` // Used for pagination: the index of the first result to return. (Optional.)
	Count      *int `url:"count,omitempty"`      // Used for pagination: the number of results to return. (Optional.)
	// ...
	Filter *string `url:"filter,omitempty"`
}

Into the function addOptions we use the query.Values to "mount" the query params from the structs.

A sample from the module docs:

type Options struct {
  Query   string `url:"q"`
  ShowAll bool   `url:"all"`
  Page    int    `url:"page"`
}

opt := Options{ "foo", true, 2 }
v, _ := query.Values(opt)
fmt.Print(v.Encode()) // will output: "q=foo&all=true&page=2"

Tags used in other optional parameter structures start with "url". That way the query is assembled correctly and then added to the end of the url.

RepositoryListOptions

type RepositoryListOptions struct {
	Visibility string `url:"visibility,omitempty"`
	Affiliation string `url:"affiliation,omitempty"`
	Type string `url:"type,omitempty"`
	Sort string `url:"sort,omitempty"`
	Direction string `url:"direction,omitempty"`
	ListOptions
}

CommitsListOptions

type CommitsListOptions struct {
	SHA string `url:"sha,omitempty"`
	Path string `url:"path,omitempty"`
	Author string `url:"author,omitempty"`
	Since time.Time `url:"since,omitempty"`
	Until time.Time `url:"until,omitempty"`
	ListOptions
}

[exageraldo]

The test that exists called TestListSCIMProvisionedIdentitiesOptions_Marshal is using the testJSONMarshal to validate. The function Test whether the marshaling of v produces JSON that corresponds to the want string..
Since the tag used refers to json, then the test is passing as expected.

[gmlewis]

Ah, I apologize, I misunderstood. Yes, it sounds like what you recommend is what is needed.
The issue is yours. Thanks, @exageraldo !

[exageraldo]

Thanks a lot! it's a bit late here now, but by tomorrow I'll send something for review.

[exageraldo]

I applied the change and created a test for it and I opened a draft pull request here #2680
I would like feedback on the nomenclatures and structure of the test.

An "interesting" point is that the old test (TestListSCIMProvisionedIdentitiesOptions_Marshal) did not break, even though it doesn't have the "json" tags.

[exageraldo]

About the test not breaking, I think I found something about that.

Inside the function testJSONMarshal we use the parameter want only once, together with the function json.Unmarshal. But when we do this,Unmarshal is not differentiating between uppercase and lowercase letters (it's case-insensitive).

From json.Unmarshal docs:

To unmarshal JSON into a struct, Unmarshal matches incoming object keys to the keys used by Marshal (either the struct field name or its tag), preferring an exact match but also accepting a case-insensitive match.

Here are the cases I tested (in go playground):

package main

import (
	"encoding/json"
	"fmt"
)

func main() {
	u := struct {
		StartIndex int    `url:"startIndex,omitempty"`
		Count      int    `url:"count,omitempty"`
		Filter     string `url:"filter,omitempty"`
	}{
		StartIndex: 1,
		Count:      10,
		Filter:     "test",
	}

	caseOne := `{
		"startIndex": 1,
		"count": 10,
	 	"filter": "test1"
	}`

	if err := json.Unmarshal([]byte(caseOne), &u); err != nil {
		fmt.Printf("Case one: Unable to unmarshal JSON for %v: %v", caseOne, err)
	} else {
		fmt.Println("Case one: All good!")
		fmt.Printf("Case one: %+v\n", u)
	}

	caseTwo := `{
		"StartIndex": 2,
		"Count": 20,
	 	"Filter": "test2"
	}`

	if err := json.Unmarshal([]byte(caseTwo), &u); err != nil {
		fmt.Printf("Case two: Unable to unmarshal JSON for %v: %v", caseTwo, err)
	} else {
		fmt.Println("Case two: All good!")
		fmt.Printf("Case two: %+v\n", u)
	}

	caseThree := `{
		"startindex": 3,
		"count": 30,
	 	"filter": "test3"
	}`

	if err := json.Unmarshal([]byte(caseThree), &u); err != nil {
		fmt.Printf("Case three: Unable to unmarshal JSON for %v: %v", caseThree, err)
	} else {
		fmt.Println("Case three: All good!")
		fmt.Printf("Case three: %+v\n", u)
	}
}

Output:

Case one: All good!
Case one: {StartIndex:1 Count:10 Filter:test1}
Case two: All good!
Case two: {StartIndex:2 Count:20 Filter:test2}
Case three: All good!
Case three: {StartIndex:3 Count:30 Filter:test3}

Program exited.

So for example, if we take the TestUpdateAttributeForSCIMUserOptions_Marshal test and change the want variable so that the fields start with uppercase letters, the test will still pass successfully.

func TestUpdateAttributeForSCIMUserOptions_Marshal(t *testing.T) {
	testJSONMarshal(t, &UpdateAttributeForSCIMUserOptions{}, `{}`)

	u := &UpdateAttributeForSCIMUserOptions{
		Schemas: []string{"test", "schema"},
		Operations: UpdateAttributeForSCIMUserOperations{
			Op:   "TestOp",
			Path: String("path"),
		},
	}

	want := `{
		"Schemas": ["test", "schema"],
		"Operations": {
			"Op": "TestOp",
			"Path": "path"
		}
	}`

	testJSONMarshal(t, u, want)
}

Output:

Running tool: /usr/local/bin/go test -timeout 30s -run ^TestUpdateAttributeForSCIMUserOptions_Marshal$ github.com/google/go-github/v50/github

ok  	github.com/google/go-github/v50/github	0.807s

That seems to be an old and long conversation: golang/go#14750