suggestion: simplify pager/result handling

[mblaschke]

here some thoughts how the code could be improved:

1. why do i have to set the adapter for the pager? can this not be passed via result or can the Get() func not return the pager at all?
2. the get function should know the model and set is automatically to the pager. is there a reason why the developer has to set it?
3. why not directly return a pager instead of the result?

current code:

result, err := client.Applications().Get(m.Context(), &opts)
if err != nil {
  panic(err)
}

pageIterator, err := msgraphcore.NewPageIterator(result, adapter, models.CreateApplicationCollectionResponseFromDiscriminatorValue)
err = pageIterator.Iterate(m.Context(), func(pageItem interface{}) bool {
  // ...
})

suggestion 1 (directly create pager):

pager, err := client.Applications().Get(m.Context(), adapter, &opts)
if err != nil {
  panic(err)
}

err = pager.Iterate(m.Context(), func(pageItem interface{}) bool {
  // ...
})

suggestion 2 (adapter automatically passed by client and pager with for loop):

pager, err := client.Applications().Get(m.Context(),&opts)
if err != nil {
  panic(err)
}

for pager.More() {
  result, err := pager.NextPage(m.Context())
  for _, row := result.Value {
    // ...
  }
}

this would make the library much easier to use and more similar like the azure-sdk-for-go or other libraries.

[rkodev]

Hi @mblaschke , Thanks for reaching out, The suggestions presented are certainly interesting and worth exploring but the overall architecture of the SDK is defined by design guidelines.

Another reason why paged responses are returned by the SDK is because the code is generated using kiota , a tool that directly translates an OpenAPI spec to an sdk, and OpenAPI spec does not have a native interpretation of Pagination. cc @darrelmiller @baywet

[baywet]

Hi @mblaschke Thanks for all the great suggestions here.

As @rkodev mentioned, we won't be changing the generation pattern to return a pager directly. That's because the paging construct is not part of standard OpenAPI specification, the way paging is done is specific to Microsoft Graph, and Kiota the generator we're using, is a general purpose client generator.

At some point we considered making paging a first class concept in Kiota but pushed back on it for reasons explained above.

As for the more pattern, which is idiomatic to Go, this is also something we debated internally before shipping the feature. The outcome of the discussion was that we have a spec SDKs follow from which we don't want to diverge too much.

We might be able to implement some of your suggestions using reflection + generics however. Of course we'd need @maisarissi (our PM) to greenlight those changes.

-result, err := client.Applications().Get(m.Context(), &opts)
-if err != nil {
-  panic(err)
-}

-pageIterator, err := msgraphcore.NewPageIterator(result, adapter, models.CreateApplicationCollectionResponseFromDiscriminatorValue)
+pageIterator, err := msgraphcore.NewPageIterator(client.Applications())
-err = pageIterator.Iterate(m.Context(), func(pageItem interface{}) bool {
+err = pageIterator.Iterate<Applicationable>(m.Context(), func(pageItem Applicationable) bool {
  // ...
})

What do you think of those potential improvements? would that improve the situation in your opinion?

Effectively we can get the discriminator by reflection and conventions, and we don't need to have you query the initial request (although that's part of the specification and we'll need greenlight from @darrelmiller), and we might even be able to get the request adapter by reflection + conventions as well. The reflection costs should not be that bad since it's only on initialization of the iterator, not for each iteration.

[dadams39]

Not having to use the CreateFromDiscriminatorValue would be a great help as this does take up a lot of space in the code base.

[darrelmiller]

As this is Graph specific work that is not impacting the Kiota generator output, I am perfectly fine with doing enhancements to the PageIterator Task to make it more usable.

[maisarissi]

We might be able to implement some of your suggestions using reflection + generics however. Of course we'd need @maisarissi (our PM) to greenlight those changes.

-result, err := client.Applications().Get(m.Context(), &opts)
-if err != nil {
-  panic(err)
-}

-pageIterator, err := msgraphcore.NewPageIterator(result, adapter, models.CreateApplicationCollectionResponseFromDiscriminatorValue)
+pageIterator, err := msgraphcore.NewPageIterator(client.Applications())
-err = pageIterator.Iterate(m.Context(), func(pageItem interface{}) bool {
+err = pageIterator.Iterate<Applicationable>(m.Context(), func(pageItem Applicationable) bool {
  // ...
})

I'm ok using reflection and generics to remove the need of users passing the discriminator and request adapter. @rkodev can you evaluate whether we can have generics for this case and that we are not going to have much impact on build times?

[rkodev]

The proposed change should simplify creating an iterator using the function

    i, err := client.NewPageIterator(result)
    i.Iterate(context.Background(), func(pageItem interface{}) bool {
        return true
    })

[baywet]

@rkodev can't we use generic types to save callers from having to do the type assertion themselves in the callback? otherwise they'll have a random object and have to figure out how to assert it.

[rkodev]

@baywet Yes we can, I considered that we might have breaking change but it does look worthwhile. This is definitely achievable

i, err := client.NewPageIterator[Groupable](result)
    i.Iterate(context.Background(), func(pageItem Groupable) bool {
        return true
    })

[rkodev]

An update on the progress of this issue.

1. The page iterator now take a generic type argument that enforces the type case in the iterator consumer function.
2. As of now, there is still not solution to passing the Discriminator func to the costructor of the argument. We will still need to pass the argument in the PageIterator constructor. (We will still attempt to work on refinig this further in future)

Do to some limitation in Go, it's not possible to improve the syntax further and have even better syntax for the iterator using Generics. Further simplification would force developers to pass the necessary type argument which would not provide the best experiece.

As of now, this is the current state of the pade iterator.

pageIterator, _ := NewPageIterator[models.User](graphResponse, reqAdapter, CreateUserFromDiscriminator)
err := pageIterator.Iterate(context.Background(), func(item models.User) bool {

cc @maisarissi @baywet

[baywet]

Thanks for investigating this and providing the results here. I think the fact the callback can now be more specific, saving people from having to type assert as the first thing, represents an improvement. We could additionally introduce a "facilitator method" in the service lib clients so people can do client.PageIterator[models.User](graphResponse, models.CreateUserFromDiscriminator).Iterate(context.Background(), func(...))

If that's possible (generic types limitations) and desirable (@maisarissi ) But besides that, I agree that's a good checkpoint and we can always review that in a V2 when/if Go generic improve. CC @ddyett

[rkodev]

This issue was resolved by https://github.com/microsoftgraph/msgraph-sdk-go-core/pull/177