Skip to content
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

Fixes more Paginations loops with value not being reset, and wrong value cloud be returned #1493

Closed
wants to merge 5 commits into from

Conversation

auyer
Copy link
Contributor

@auyer auyer commented Feb 3, 2024

Description

All this endpoints are susceptible to returning wrong data due to the Struct not getting reset for every iteration of the loop.
I fixed it by moving the variable declaration to inside the loop.

This is related to my other PR #1482 that fixed this for a single api endpoint.
I also found this PR that accomplished the same result in a different way #1016 . If you prefer this method, let me know. I can change them all to be in the same way.
And there is also this PR #1447 that is still missing the change-log. I cloud remove the 3 cases that PR fixes from mine, in case you want to merge that one too.

Has your change been tested?

Yes, go test ./... --race, and also some API calls.
But as I mentioned in #1482, I was not able to create tests that fail without this change, but it happens in runtime.
Most of the tests in this repo do not cover pagination.

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented in cloudflare/api-schemas
    and relies on stable APIs.

Copy link
Contributor

github-actions bot commented Feb 3, 2024

changelog detected ✅

@auyer auyer marked this pull request as ready for review February 3, 2024 21:14
@auyer auyer requested a review from jacobbednarz as a code owner February 3, 2024 21:14
@auyer
Copy link
Contributor Author

auyer commented Feb 29, 2024

Hi @jacobbednarz , could you review this please?
Its the same change repeated in all paginations I found that needed it.

Thanks!

@jacobbednarz
Copy link
Member

i haven't managed to set aside anytime for this lately but my concern is that there is no test coverage for this condition so it's hard to identify if this is actually addressing a problem. this hasn't been observed anywhere that we are using (in hundred of paginations) so i'm hesitant to blindly accept these ones. before we merge this, we need to either 1) find cases where this is a problem to confirm the fix or 2) identify a way to isolate this within a test case.

@jacobbednarz
Copy link
Member

v2 provides the option to handle the pagination automatically so this is no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants