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

firewall_rules/filter: reset fResponse on each loop to avoid residual data #1156

Merged
merged 4 commits into from
Dec 29, 2022

Conversation

jafowler
Copy link
Contributor

@jafowler jafowler commented Dec 23, 2022

Description

We found an issue were data from the previous page could populate into the current page because we were not reseting the struct on each loop.

Has your change been tested?

This is something i'd love to have a conversation about. We need / should look into how to properly test our paginated APIs

Types of changes

bugfix
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)

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Merging #1156 (50908bb) into master (6153c1e) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1156      +/-   ##
==========================================
- Coverage   49.40%   49.33%   -0.08%     
==========================================
  Files         127      128       +1     
  Lines       12290    12406     +116     
==========================================
+ Hits         6072     6120      +48     
- Misses       4840     4885      +45     
- Partials     1378     1401      +23     
Impacted Files Coverage Δ
email_routing_destination.go 66.66% <100.00%> (+0.41%) ⬆️
email_routing_rules.go 65.64% <100.00%> (+0.26%) ⬆️
filter.go 43.38% <100.00%> (+0.41%) ⬆️
firewall_rules.go 54.38% <100.00%> (+0.40%) ⬆️
lockdown.go 58.22% <100.00%> (+0.53%) ⬆️
queue.go 72.02% <100.00%> (+0.29%) ⬆️
teams_list.go 53.01% <100.00%> (+0.28%) ⬆️
workers.go 57.14% <100.00%> (ø)
workers_kv.go 39.68% <100.00%> (+0.32%) ⬆️
mtls_certificates.go 26.59% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

changelog detected ✅

@jacobbednarz
Copy link
Member

jacobbednarz commented Dec 23, 2022

have you checked the other list methods? they all use basically the same layout and may be impacted in a similar way.

in an ideal world, this should be a helper method or utilise Go generics from 1.19.

I'll give this change a run through a couple of projects I have using the pagination conditions as this needs to account for automatic and manual pagination.

@jafowler
Copy link
Contributor Author

have you checked the other list methods? they all use basically the same layout and may be impacted in a similar way.

in an ideal world, this should be a helper method or utilise Go generics from 1.19.

I'll give this change a run through a couple of projects I have using the pagination conditions as this needs to account for automatic and manual pagination.

There are a couple more places where this should be implemented. I have them stashed locally but i'll look into how to do this with generics to make things more ideal.

@jacobbednarz jacobbednarz merged commit 7800dcf into master Dec 29, 2022
@jacobbednarz jacobbednarz deleted the reset-struct-on-each-loop branch December 29, 2022 21:44
@github-actions github-actions bot added this to the v0.58.0 milestone Dec 29, 2022
@jacobbednarz
Copy link
Member

we'll address the move to a generics based helper once Go 1.20 lands and we drop support for 1.17.

github-actions bot pushed a commit that referenced this pull request Dec 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

This functionality has been released in v0.58.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

ivan-section-io pushed a commit to section/cloudflare-go that referenced this pull request Jan 12, 2023
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.

4 participants