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

Multiple access application pages result in bad data returned #1447

Closed
wants to merge 2 commits into from

Conversation

theFong
Copy link

@theFong theFong commented Nov 23, 2023

Description

When there are multiple pages of access applications, I observed incorrect create times, update times, and other objects being assigned for each access application object in the list returned. I didn't look into the root cause of why this was occurring because it seems like the code should work (and does for values), but the following fix addresses the problem by not reassigning the entire object on each page.

Has your change been tested?

I have tested the fix in our own code base. I did not add a new test to cover this because it wasn't trivial to add a test for multiple pages. Happy to add if someone points me in the right direction.

Screenshots (if appropriate):

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

Oops! It looks like no changelog entry is attached to this PR. Please include a release note as described in https://github.com/cloudflare/cloudflare-go/blob/master/docs/changelog-process.md.

Example:

```release-note:TYPE
Release note
```

If you do not require a release note to be included, please add the workflow/skip-changelog-entry label.

@jacobbednarz
Copy link
Member

can you please provide a Go reproduction case and the HTTP interactions being made? the result info here is intentionally meant to move with the paginated result.

@theFong
Copy link
Author

theFong commented Nov 23, 2023

Hi @jacobbednarz! I can give adding a test/repro case a shot, are there examples of pagination being tested in the repo currently?

Also, as far as I can tell the result info still moves with the paginated result.

@theFong
Copy link
Author

theFong commented Nov 27, 2023

Hi @jacobbednarz, was fixing some of the other endpoints I use and found that a PR had been made for ListDNSRecords which arrived at the same exact solution! #1222

I'll use his test cases :)

@auyer
Copy link
Contributor

auyer commented Feb 3, 2024

Hi @theFong !
Since I found a similar issue in my #1481 PR, I decided to fix it in all files I cloud find.
If you still want to merge this PR, take a look at the docs/changelog-process.md to complete it, and I can remove these 3 files from my #1493 PR.
Thanks!

@theFong
Copy link
Author

theFong commented Feb 10, 2024

Hi @theFong ! Since I found a similar issue in my #1481 PR, I decided to fix it in all files I cloud find. If you still want to merge this PR, take a look at the docs/changelog-process.md to complete it, and I can remove these 3 files from my #1493 PR. Thanks!

Hey @auyer!

Awesome!! Glad I was a little helpful. Forgot to get around and write the test. If you've fixed it everywhere then no need for my PR. I'll close this.

@theFong theFong closed this Feb 10, 2024
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.

3 participants