这是indexloc提供的服务，不要输入任何密码
Skip to content

log when request body parsing fails (fix #2555) #2556

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Aug 1, 2019
Merged

log when request body parsing fails (fix #2555) #2556

merged 10 commits into from
Aug 1, 2019

Conversation

@ecthiender
Copy link
Contributor

@ecthiender ecthiender commented Jul 16, 2019 •
edited
Loading

Description

Regression: since beta.3 whenever request body parsing failed, the error was not logged. This PR fixes the bug.

Affected components

  • Server

Related Issues

#2555

Solution and Design

Simple fix. The parseBody function was previously part of the handler functions. Since beta.3 it has been moved to mkSpockAction, but the result of it was handled by qErrToResp instead of logAndThrow function.

Edit: After review from @rakeshkky , refactored mkSpockAction to have separate functions for [logging error and returning the response] and [logging success and returning the response]. Removed specific functions which only returned response (which was the source of the bug).

Notes to reviewer

  • Simple one line fix in Hasura.Server.App module

Steps to test and verify

  1. Run graphql-engine
  2. Make a request to /v1/graphql or /v1/query with wrong JSON structure.
  3. Observe: The error is logged

Limitations, known bugs & workarounds

@ecthiender ecthiender requested a review from 0x777 as a code owner July 16, 2019 10:41
@hasura-bot
Copy link
Contributor

hasura-bot commented Jul 16, 2019

Beep boop! 🤖

Hey @ecthiender, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible. 🕐

Stay awesome! 😎

@ecthiender ecthiender added c/server Related to server k/bug Something isn't working s/ok-to-merge Status: This pull request can be merged to master labels Jul 16, 2019
@netlify
Copy link

netlify bot commented Jul 16, 2019 •
edited
Loading

Deploy preview for hasura-docs ready!

Built with commit 63bd7f8

https://deploy-preview-2556--hasura-docs.netlify.com

@hasura-bot
Copy link
Contributor

hasura-bot commented Jul 16, 2019

Review app for commit 0ec99cf deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-0ec99cf7

@ecthiender ecthiender requested a review from rakeshkky July 16, 2019 11:06
@ecthiender
refactor logging code
967a141 
  - fix a bug in websocket-log
@ecthiender ecthiender added s/do-not-merge Do not merge this pull request to master and removed s/ok-to-merge Status: This pull request can be merged to master labels Jul 16, 2019
@ecthiender
Copy link
Contributor Author

ecthiender commented Jul 16, 2019

As per @rakeshkky , refactored some logging code at the server module. Need to verify if it breaks any existing logging.

@hasura-bot
Copy link
Contributor

hasura-bot commented Jul 16, 2019

Review app for commit 967a141 deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-967a1410

@ecthiender
fix review comments
1ccfdca 
  - tidy logging in jwk-refresh-log
  - fix a minor bug in refreshing JWK:
    the JWK refreshed 10 seconds before the actual expiry (assuming the
    expiry time will always be larger than 10 seconds). Fix that.
@hasura-bot
Copy link
Contributor

hasura-bot commented Jul 17, 2019

Review app for commit 1ccfdca deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-1ccfdca9

rakeshkky
rakeshkky previously approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@rakeshkky rakeshkky left a comment

Choose a reason for hiding this comment

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

server LGTM

@hasura-bot
Copy link
Contributor

hasura-bot commented Jul 17, 2019

Review app for commit b9812a1 deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-b9812a14

@ecthiender ecthiender added s/ok-to-merge Status: This pull request can be merged to master and removed s/do-not-merge Do not merge this pull request to master labels Jul 21, 2019
@0x777 0x777 requested a review from lexi-lambda as a code owner July 30, 2019 08:08
@hasura-bot
Copy link
Contributor

hasura-bot commented Jul 30, 2019

Review app for commit 05d9ed0 deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-05d9ed09

lexi-lambda
lexi-lambda previously approved these changes Jul 31, 2019
View reviewed changes
Copy link
Contributor

@lexi-lambda lexi-lambda left a comment

Choose a reason for hiding this comment

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

These changes seem reasonable to me (though I’m not very familiar with the code and functionality in question, so it probably isn’t a super thorough review), but I do wonder: is there any way to add a test for this functionality that isn’t too difficult? It seems like logging is worth having access to in the test harness, but I don’t know if it’s set up for that right now.

@hasura-bot
Copy link
Contributor

hasura-bot commented Aug 1, 2019

Review app for commit 657c025 deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-657c0257

@ecthiender
Copy link
Contributor Author

ecthiender commented Aug 1, 2019

These changes seem reasonable to me (though I’m not very familiar with the code and functionality in question, so it probably isn’t a super thorough review), but I do wonder: is there any way to add a test for this functionality that isn’t too difficult? It seems like logging is worth having access to in the test harness, but I don’t know if it’s set up for that right now.

@lexi-lambda there are tests for logging. Let me see if I can add tests for this quickly.

@hasura-bot
Copy link
Contributor

hasura-bot commented Aug 1, 2019

Review app for commit ab555de deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-ab555de3

@ecthiender ecthiender dismissed stale reviews from lexi-lambda and rakeshkky via 63bd7f8 August 1, 2019 09:28
@hasura-bot
Copy link
Contributor

hasura-bot commented Aug 1, 2019

Review app for commit 63bd7f8 deployed to Heroku: https://hge-ci-pull-2556.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2556-63bd7f8b

rakeshkky
rakeshkky reviewed Aug 1, 2019
View reviewed changes
Copy link
Member

@rakeshkky rakeshkky left a comment

Choose a reason for hiding this comment

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

server test LGTM

@0x777 0x777 merged commit b4a0a03 into hasura:master Aug 1, 2019
@hasura-bot
Copy link
Contributor

hasura-bot commented Aug 1, 2019

Review app https://hge-ci-pull-2556.herokuapp.com is deleted

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
@ecthiender @polRk
log when request body parsing fails (fix hasura#2555) (hasura#2556)
87cde04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rakeshkky rakeshkky rakeshkky left review comments

@0x777 0x777 Awaiting requested review from 0x777

+1 more reviewer

@lexi-lambda lexi-lambda lexi-lambda left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c/server Related to server k/bug Something isn't working s/ok-to-merge Status: This pull request can be merged to master

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ecthiender @hasura-bot @lexi-lambda @rakeshkky @0x777 @shahidhk