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

Fixes issue #255: Can't access HTTP request body in java11-vert-x template. #256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

p-fortier
Copy link

@p-fortier p-fortier commented Apr 3, 2021

Signed-off-by: Patrik Fortier [email protected]

Description

Added a io.vertx.ext.web.handler.BodyHandler instance to the handlers to populate routingContext with the HTTP request body.

Motivation and Context

  • I have raised an issue to propose this change (required)

Which issue(s) this PR fixes

Fixes #255

How Has This Been Tested?

I deployed a function reading the http request body using the modified

Types of changes

  • 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)
  • Version change (see: Impact to existing users)

Impact to existing users

Adds one handler to the router. According to vertx documentation, the overhead is negligible.

Checklist:

  • My code follows the code style of this project.
    • Maybe a separation is required between my modification and the static handler bloc.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
    • I did not run the verify.sh script

@derek derek bot added the new-contributor label Apr 3, 2021
@alexellis
Copy link
Member

How Has This Been Tested?
I deployed a function reading the http request body using the modified

Could you share testing instructions please? I'd like to try it.

Before:

faas-cli template pull ...
faas-cli new --...
faas-cli up ...
faas-cli invoke

After:

faas-cli template pull ...
faas-cli new --...
faas-cli up ...
faas-cli invoke

@akshatdev0
Copy link

Tried this change. And it works.

@alexellis
Copy link
Member

Sorry I wasn't clear.

How Has This Been Tested? <- I'd like the output from the above.

Thanks

@p-fortier
Copy link
Author

p-fortier commented May 2, 2021

Hi, sorry I didn't follow this right away. I took time to make a small example.
Source code is available at https://github.com/p-fortier/try-fix but the idea is described below:

Before:

faas-cli template pull 
faas-cli new --lang java11-vert-x tryout

The handler for the project should look something like this. This simply returns the request body sent:

  public void handle(RoutingContext routingContext) {
    routingContext.response()
      .putHeader("content-type", "application/json;charset=UTF-8")
      .end(
              routingContext.getBodyAsString()
      );
  }

Calling the function

faas-cli up -f tryout.yml -g $OPENFAAS_URL
echo "hello" | faas-cli invoke tryout -g $OPENFAAS_URL
->  Server returned unexpected status code: 500 - Internal Server Error

Logs show something like this:

tryout-5f76f98558-5hkhn:2021/05/02 14:51:53 stderr: SEVERE: Unexpected exception in route                                                                  
tryout-5f76f98558-5hkhn:2021/05/02 14:51:53 stderr: java.lang.NullPointerException
...
tryout-5f76f98558-5hkhn:2021/05/02 14:51:53 POST / - 500 Internal Server Error - ContentLength: 21

Applying fix

rm -r template/java11-vert-x
faas template pull https://github.com/p-fortier/templates#f255 #This should take the fixed template 
faas-cli up -f tryout.yml -g $OPENFAAS_URL
echo "hello" | faas-cli invoke tryout -g $OPENFAAS_URL
--> hello

@alexellis
Copy link
Member

Hi @p-fortier this dropped off my radar.

It seems important to be able to access the request body and I wasn't aware that this was not possible with the template!

@pmlopes pinged me about this, so I think we should consider revisiting this.

Thanks for adding context.

@alexellis
Copy link
Member

Please see also #282 - let me know your thoughts on that one.

@alexellis
Copy link
Member

@pmlopes can this be merged in your opinion?

@pmlopes
Copy link

pmlopes commented Feb 18, 2022

Yes, this will always parse the request body. For security or latency reasons not everyone might want/need it, but is a quick solution. In the refered issue about upgrading the template we shall address this in a user defined way.

@alexellis
Copy link
Member

What do you mean "parse"?

Is there a way to get the RAW body without this change?

@pmlopes
Copy link

pmlopes commented Feb 18, 2022

Yes you can get the raw body, by replicating what the BodyHandler is doing, on the request object, add a handler that will receive the chunks of the HTTP body and either assemble a full buffer or process it as a stream.

The body handler does a bit more, it handles, http body, form uploads, ensures that files are discarded, limits the size to avoid DoS, etc...

@pmlopes pmlopes mentioned this pull request Mar 2, 2022
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't access HTTP request body in java11-vert-x template.
4 participants