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

Make request body readable and changeable in interceptFunc and beforeFunc #81

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

groovili
Copy link
Contributor

@groovili groovili commented May 3, 2020

Hi!

This PR gives users the ability to access and change request.Body in functions registered with RegisterInterceptFunc and RegisterBeforeFunc. I've already described it in issue #80.
Also, I've added couple of simple tests to check ability to change request data in these functions.

Changes:

  1. In rpc/v2 close request.Body after the execution of beforeFunc's and interceptFunc's. Update codec request info after calls to functions above.
  2. Read request body bytes, decode it to codec format and provide bytes.Buffer and request.Body for underlying functions in v2/json, v2/json2 and v2/protorpc codecs.

Of course, exists a better way to do that, but it will require changes in the signature of RegisterInterceptFunc and RegisterBeforeFunc and it would be breaking changes. If both of these methods will have an original *http.Request as input parameter, they could be executed before the creation of codec. In this case, users can access and alter request data, and only after that it would be read by the codec and marshaled to service request params. But since these changes are breaking, it's not an option at the moment, probably it can fit the next version or release.

Would be nice to know your opinion, thanks!

jaitaiwan
jaitaiwan previously approved these changes Feb 10, 2024
Copy link
Member

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

LGTM @AlexVulaj @apoorvajagtap take a review and if happy lets merge.

@AlexVulaj
Copy link
Member

LGTM as well @jaitaiwan . @groovili thanks for the contribution! Do you mind resolving your branch conflicts so we can go ahead and merge this? Thanks!

@groovili
Copy link
Contributor Author

groovili commented Feb 14, 2024

@AlexVulaj @jaitaiwan thanks for the review! Merged with latest changes and updated the CL to not use deprecated ioutil. Please take another look to proceed with merging.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 64.15094% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 71.70%. Comparing base (39123e3) to head (422b3f1).

Files Patch % Lines
v2/json2/server.go 47.05% 8 Missing and 1 partial ⚠️
v2/server.go 63.63% 3 Missing and 1 partial ⚠️
v2/json/server.go 78.57% 2 Missing and 1 partial ⚠️
v2/protorpc/server.go 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   71.44%   71.70%   +0.25%     
==========================================
  Files          15       15              
  Lines         844      887      +43     
==========================================
+ Hits          603      636      +33     
- Misses        181      189       +8     
- Partials       60       62       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AlexVulaj AlexVulaj merged commit 4342b77 into gorilla:main Mar 6, 2024
7 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants