-
Notifications
You must be signed in to change notification settings - Fork 8
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
Improve type validation for PageIterator #267
Comments
@baywet 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 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 |
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 -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? |
@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 |
Thanks for the additional information. I'm not sure you can actually since the create from discriminator function is static? |
@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. |
Thanks for sharing this. 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. |
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 |
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) |
Only concern with that is there is no compile time validation with the discriminator method. |
can you expand on what you mean by that? We don't have defined type for the discriminator method? |
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
We need to provide possible solutions for the type validation of the parameters in the constructor function
The text was updated successfully, but these errors were encountered: