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

feature: preserve body after parse #63

Merged
merged 23 commits into from Mar 31, 2022

Conversation

suikabreaker
Copy link
Contributor

@suikabreaker suikabreaker commented Jan 12, 2022

Fix #41 and #42. Inspired from this PR: #43.

My goal is to leave the request body unchanged after "regeneration" and decouple the implementation of #61 from this, so I implement this in another way and try to make fewer modifications to the original logic.

@suikabreaker
Copy link
Contributor Author

@zhuizhuhaomeng May you review this PR?

lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
t/sanity.t Outdated Show resolved Hide resolved
t/sanity.t Outdated Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
Copy link
Contributor

@xiaocang xiaocang left a comment

Choose a reason for hiding this comment

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

The document of the preserve option is also needed.

Note that it should be stated that if the preserve option is turned on, the memory usage is doubled.

@xiaocang
Copy link
Contributor

And there is a small doubt on my own: what is the scenario for the use of the function of preserving the request body here, and is it common enough?

lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Show resolved Hide resolved
t/sanity.t Outdated Show resolved Hide resolved
Co-authored-by: Johnny Wang <wangjiahao@openresty.com>
@suikabreaker
Copy link
Contributor Author

And there is a small doubt on my own: what is the scenario for the use of the function of preserving the request body here, and is it common enough?

There is. I have been working on a WAF project, which acts as a proxy filtering request body. It needs to pass the body(and if we can, do not modify the body) if there's no attempt of attack found.

lib/resty/upload.lua Outdated Show resolved Hide resolved
@suikabreaker
Copy link
Contributor Author

The document of the preserve option is also needed.

Note that it should be stated that if the preserve option is turned on, the memory usage is doubled.

Seems other two options are also not documented. Added description.

@xiaocang
Copy link
Contributor

@suikabreaker there's still typo: warpped -> wrapped.

lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Outdated Show resolved Hide resolved
@suikabreaker
Copy link
Contributor Author

@suikabreaker there's still typo: warpped -> wrapped.

Fixed.

lib/resty/upload.lua Outdated Show resolved Hide resolved
@xiaocang
Copy link
Contributor

The other part looks good to me.

lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Outdated Show resolved Hide resolved
lib/resty/upload.lua Outdated Show resolved Hide resolved
@suikabreaker
Copy link
Contributor Author

The other part looks good to me.

I have re-requested review.

@xiaocang
Copy link
Contributor

could you help check this PR?
ping @zhuizhuhaomeng @doujiang24

README.markdown Outdated Show resolved Hide resolved
README.markdown Outdated Show resolved Hide resolved
Co-authored-by: lijunlong <lijunlong@openresty.com>
@zhuizhuhaomeng zhuizhuhaomeng merged commit 73c8984 into openresty:master Mar 31, 2022
@xiaocang xiaocang mentioned this pull request Apr 7, 2022
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.

How to reconstruct body after upload
3 participants