Skip to content

PageIterator - fix mutual recursion, add to service library #523

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.

Sign up for GitHub

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

Jump to bottom
Merged
MIchaelMainer merged 8 commits into from
Aug 15, 2019
Merged

PageIterator - fix mutual recursion, add to service library #523

MIchaelMainer merged 8 commits into from
Aug 15, 2019

Conversation

@MIchaelMainer
Copy link
Collaborator

@MIchaelMainer MIchaelMainer commented Aug 2, 2019 •
edited
Loading

Changes proposed in this pull request

  • Move PageIterator from Core library to the service library. When we moved from a single repo to to, the PageIterator was left in the core and the tests moved to the service library. This completes the refactor.
  • Remove the mutual recursion that caused stackoverflow exception and replace it with a loop based on nextLink. Fixes PageIterator StackoverflowException on large dataset msgraph-sdk-dotnet-core#16
  • Fix issue with adding items to the processing queue.

Merge gates

  • Remove PageIterator from Microsoft.Graph.Core and publish Microsoft.Graph.Core to NuGet. We need this so we can make the following changes before building.
  • Change the PageIterator namespace to Microsoft.Graph from Microsoft.Graph.Tasks. We had to do this during development as the Core library has the PageIterator in the published NuGet and we'd have a name collision.
  • Remove the Microsoft.Graph.Tasks alias from PageIteratorTests.cs. This was done to disambiguate the PageIterator (see previous point).
  • Increment version.

Post publication

  • Integrate these verified changes to the beta client library.

@MIchaelMainer
Add pageiterator
4c699f7
@MIchaelMainer
Temp: making reference to Core before PageIterator existed in it.
c28c598
@MIchaelMainer
Updated PageIterator to use a loop to iterate
63709fc 
On branch mm/movePageIterator
Changes to be committed:
 modified:   src/Microsoft.Graph/Microsoft.Graph.csproj

 Set project reference to use will card.

 modified:   src/Microsoft.Graph/Tasks/PageIterator.cs

 * Moved iterator state to IntrapageIterate.
 * Fixed queue bug.
 * Added nextLink detection.
 * NOTE> Requires change to namespace before merge.
 * NOTE> Requires PageIterator.cs removal from M.G.C. before merge.

 modified:   tests/Microsoft.Graph.DotnetCore.Test/Mocks/MockUserEventsCollectionPage.cs

 * Fix mock deltaLink and nextLink.

 modified:   tests/Microsoft.Graph.DotnetCore.Test/Tasks/PageIteratorTests.cs

 * Added loop detection test.
 * NOTE> Will need to change namespace alias for PageIterator before merge.
@MIchaelMainer
Minor fixes.
0a5be2e
@MIchaelMainer MIchaelMainer requested a review from peombwa August 2, 2019 00:41
@MIchaelMainer MIchaelMainer mentioned this pull request Aug 2, 2019
Merged
peombwa
peombwa requested changes Aug 5, 2019
View reviewed changes
peombwa
peombwa previously approved these changes Aug 13, 2019
View reviewed changes
@MIchaelMainer MIchaelMainer merged commit 4186d84 into dev Aug 15, 2019
@MIchaelMainer MIchaelMainer deleted the mm/movePageIterator branch August 15, 2019 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peombwa peombwa peombwa approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PageIterator StackoverflowException on large dataset

3 participants

@MIchaelMainer @peombwa