Skip to content

LG-10062 Accept compressed Service Provider#9074

Merged
Sgtpluck merged 6 commits intomainfrom
dmm/allow-compressed-requests
Aug 25, 2023
Merged

LG-10062 Accept compressed Service Provider#9074
Sgtpluck merged 6 commits intomainfrom
dmm/allow-compressed-requests

Conversation

@Sgtpluck
Copy link
Contributor

🎫 Ticket

https://cm-jira.usa.gov/browse/LG-10062

🛠 Summary of changes

This ticket surfaced an issue where, when JSON requests to sync the service providers are over 20k, they are automatically rejected by nginx. We raised the nginx threshold, but also would like to compress requests to make sure the requests stay below the threshold.

It should be merged before 18F/identity-dashboard#667

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

@Sgtpluck Sgtpluck force-pushed the dmm/allow-compressed-requests branch from 14449f7 to be00975 Compare August 24, 2023 17:48
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one idea, but not a blocker

Comment on lines +39 to +45
return {} unless request.headers['Content-Type'] == 'gzip/json'

body = request.body.read

return {} unless body.present?

JSON.parse(Zlib.gunzip(body))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in my mind there are 3 cases we want to handle, in the spirit of "be liberal in what you accept, and strict in what you output":

  • GZIP'ed JSON params
  • regular JSON params
  • form encoded params

So seeing this it seems like we just have empty params for anything that's not gzipped?

Is it possible to allow the other formats in addition to the GZIP? Like something like:

Suggested change
return {} unless request.headers['Content-Type'] == 'gzip/json'
body = request.body.read
return {} unless body.present?
JSON.parse(Zlib.gunzip(body))
if request.headers['Content-Type'] == 'gzip/json'
body = request.body.read
if body.present?
JSON.parse(Zlib.gunzip(body))
else
{}
end
elsif params.key?(:service_provider) # fall back to rails processing of params?
params.permit(service_provider: {})
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i like that! thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in ec73ab3 (#9074)

@Sgtpluck Sgtpluck merged commit 92248db into main Aug 25, 2023
@Sgtpluck Sgtpluck deleted the dmm/allow-compressed-requests branch August 25, 2023 13:51
@mitchellhenke mitchellhenke changed the title LG-10062 register gzip Mime::Type LG-10062 Accept compressed Service Provider Aug 25, 2023
@mdiarra3 mdiarra3 mentioned this pull request Aug 29, 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.

3 participants