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

add StringWriter interface to ResponseWriter #921

Conversation

romainmenke
Copy link
Contributor

@romainmenke romainmenke commented Nov 20, 2023

Thank you for contributing to Coraza WAF, your effort is greatly appreciated
Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Make sure that you've checked the boxes below before you submit PR:


This change adds the io.StringWriter interface to the http.ResponseWriter implementation in interceptor.go.

This is a fairly small addition because string values can be trivially converted to bytes.
It does not rely on the underlying http.ResponseWriter to also be a io.StringWriter.

Implementing WriteString is fairly common for http response writers.

@romainmenke romainmenke requested a review from a team as a code owner November 20, 2023 07:02
@romainmenke romainmenke changed the title add StringWriter interface to ResponseWriter add StringWriter interface to ResponseWriter Nov 20, 2023
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (48cf923) 81.81% compared to head (3ef48cb) 81.82%.

❗ Current head 3ef48cb differs from pull request most recent head 74ca0a5. Consider uploading reports for the commit 74ca0a5 to get more accurate results

Files Patch % Lines
http/interceptor.go 26.31% 14 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #921   +/-   ##
=======================================
  Coverage   81.81%   81.82%           
=======================================
  Files         160      160           
  Lines        9064     9066    +2     
=======================================
+ Hits         7416     7418    +2     
  Misses       1399     1399           
  Partials      249      249           
Flag Coverage Δ
default 76.92% <26.31%> (+<0.01%) ⬆️
examples 26.36% <5.26%> (-0.01%) ⬇️
ftw 46.63% <5.26%> (-0.02%) ⬇️
ftw-multiphase 48.81% <5.26%> (-0.02%) ⬇️
tinygo 75.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

http/interceptor.go Outdated Show resolved Hide resolved
var _ http.ResponseWriter = (*rwInterceptor)(nil)
type stringResponseWriter interface {
http.ResponseWriter
io.StringWriter
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind pointing at an example of another similar code that implements io.StringWriter? This doesn't seem so bad, but I'm wondering when a caller wouldn't just call Write([]byte(s)) themselves without inspecting the interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to a thing but now I only find old references.
Like these :

I suspect that WriteString on ResponseWriter is no longer common and that Write([]bytes(s)) is the idiomatic way.


We still have a bunch of tests to make sure that all middleware we add implements the needed interfaces. But it has been a while (±6 years) since I revisited those tests.

e.g. CloseNotifier is also unimplemented here but that is documented as deprecated in http.

So, yeah, this is ok to close for me :)
It can always be revisited if there is an actual need for this and not because of some legacy requirement.


Thank you for the super speedy review 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for the investigation @romainmenke! Yeah let's go ahead and close for now then, if we find a strong use case for it we can reintroduce it.

@anuraaga anuraaga closed this Nov 20, 2023
@romainmenke romainmenke deleted the add-stringwriter-interface-to-response-writer--philosophical-asian-giant-hornet-90e534f95d branch November 20, 2023 16:04
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