Improve type validation for PageIterator

[rkodev]

PageIterator does not provide type validation on the constructor function and this is resulting to runtime errors that arenot easy to debug for users. e.g

1. Retrieving site pages panics with "interface {} is *models.ListItem, not *models.SitePage" msgraph-beta-sdk-go#380
2. panic while iterating over Page Item msgraph-sdk-go#558

We need to provide possible solutions for the type validation of the parameters in the constructor function

[rkodev]

Vincent Biret (@baywet)
We should be able to support type inference by making the following changes to the Page Iterator and the sdk generator

this would be the result

msgraphcore.NewPageIterator(pages, client.GetAdapter(), models.NewSitePage) // 

The benefit of this function would be it no longer requires users to know the ParsableFactory but instead only requires the constructor of the return type that they should be aware of

changes to the page iterator would include modification to the constructor function so as to take a generic argument for "parseable collection" and a contructor for the similar value

i.e

// ParseCollections represents a contract with the GetValue() method
type ParseCollections[T serialization.Parsable] interface {
	serialization.Parsable
	GetValue() []T
	SetValue(value []T)
}

type ParseFactory[T serialization.Parsable] func() T

// NewPageIterator creates an iterator instance
//
// It has three parameters. res is the graph response from the initial request and represents the first page.
// reqAdapter is used for getting the next page and constructorFunc is used for serializing next page's response to the specified type.
func NewPageIterator[T serialization.Parsable](res ParseCollections[T], reqAdapter abstractions.RequestAdapter, constructorFunc ParseFactory[T]) (*PageIterator[T], error) {

This change would however require a change in the constructor syntax of objects in Kiota as type assertion in Go not yet intelligent enough to infer current reference to a struct. Constructor functions will need to return an interface i.e

func NewSitePage() *SitePage // curent contructor

func NewSitePage() SitePageable // proposed syntax

will be changed to

[baywet]

Thanks for all this information, it helps a lot.

There are a couple of things I'd like to clarify to understand the impact better.

First off

-func NewSitePage() *SitePage
+func NewSitePage() SitePageable

I can't think of this change being source breaking since *SitePage and SitePageable offer the same API surface, is that your understanding as well?

Then if we take the example of group members GET /groups/{id}/members here is the change we're talking about I believe (please confirm)

-msgraphcore.NewPageIterator(pages, client.GetAdapter(), models.CreateDirectoryObjectCollectionResponseFromDiscriminatorValue)
+msgraphcore.NewPageIterator(pages, client.GetAdapter(), models.NewDirectoryObject)

This would effectively be binary breaking as far as I can tell, correct? We could probably get around to that by deprecating the existing method and introducing an overload with tons of documentation.

This also poses another challenge: for each result, we're now not calling the CreateDirectoryObjectFromDiscriminatorValue anymore, which means we won't downcast and deserialize to users, service principals, etc... as expected. Which is a pretty major change in behaviour in my opinion. Any thoughts?

[rkodev]

Vincent Biret (@baywet) I do agree with the change for constructor behavior

-func NewSitePage() *SitePage
+func NewSitePage() SitePageable

This will not be source breaking, but it will help with automatic type assertion while using the generic function.

As for

-msgraphcore.NewPageIterator(pages, client.GetAdapter(), models.CreateDirectoryObjectCollectionResponseFromDiscriminatorValue)
+msgraphcore.NewPageIterator(pages, client.GetAdapter(), models.NewDirectoryObject)

I think we can provide an overload that will not be binary breaking, and deprecated the older version. I'd be happy for overload name suggestion

On the change in behavior or down casting, I got around it by using the models.NewDirectoryObject and wrapping with a generic ParseableConstructor. We will still retain the types when using the Iterate function

[baywet]

Thanks for the additional information. I'm not sure you can actually since the create from discriminator function is static?

[rkodev]

Vincent Biret (@baywet) Here is some sample code that works in a branch .

Only change that was required for type hinting is the constructor is

-func NewSitePage() *SitePage
+func NewSitePage() SitePageable

and initializer is

- pageIterator, err := msgraphcore.NewPageIterator[models.Userable](result, adapter, models.CreateUserCollectionResponseFromDiscriminatorValue)
+ pageIterator, err := msgraphcore.NewPageIteratorV2(result, adapter, models.NewUser)


	if err != nil {
		fmt.Printf("Error creating iterator: %v\n", err)
		panic(err)
	}

	err = pageIterator.Iterate(context.Background(), func(pageItem models.Userable) bool {
		fmt.Printf(*pageItem.GetDisplayName())
		userList = append(userList, pageItem)
		return true
	})

The benefit for this method is that you don't need to know the factor method for the collection response and instead only the constructor of the iterate object , which seems to be what the are passing in the examples above.

[baywet]

Thanks for sharing this.
Again, because of this line and from the example you've provided, I don't believe the discriminator will work.

If you use that approach to query /groups/id/members, and pass models.newDirectoryObject (because other types of directory objects can be members of a group), you'll get a homogenous collection of directory objects. Not a heterogenous collection of users, service principals, etc... Which would be a regression.

[rkodev]

In that case the only other way possible way I can think of is to generate a registry on the client that returns a map of [package.objectName]factoryMethod on the client object and maintain it. That way it should possibly simplify the constructor to msgraphcore.NewPageIterator(client, result) since reflection cannot be used to identify the factory method.

[baywet]

I'd like to avoid this option as well: It'd be tweaking kiota for a graph specific scenario. Also that'd be a one off for the Go language specifically. And it'd probably take significant memory given the number of models we have ( ~2000), including for people who don't use the task. Any objection with people passing the discriminator method instead? (for the item, not the collection)

[rkodev]

Only concern with that is there is no compile time validation with the discriminator method.

[baywet]

can you expand on what you mean by that? We don't have defined type for the discriminator method?

[andrewbcohere]

I came across this and it confused me while trying to use the sdk, not only is there no validation that the constructor function passed is correct until runtime, but it will actually succeed regardless of the constructor method passed in (even nil), as long as there are not enough results to trigger paging. So this can lead to very insidious bugs where your code works until all of a sudden your data grows to have more items and need paging, at which point the code will start panicking with zero source changes leading to a no doubt confusing debugging situation.

For example I ran this:

pageIterator, err := msgraphcore.NewPageIterator[*models.Drive](drives, c.client.GetAdapter(), nil)

and the code runs without error, since I don't have enough drives to trigger a page