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 identity-backfill pass on zuora errors as 400 #224

Merged
merged 2 commits into from Nov 28, 2018

Conversation

QuarpT
Copy link
Contributor

@QuarpT QuarpT commented Nov 28, 2018

When running scripts it's hard to tell what's happened on an internal server error.

I can't see any secrets returned on 500s. Is there a reason for hiding the message? Endusers don't hit any of this project's endpoints directly do they?

Edit: I've changed this to return 400s with error reasons on api client errors in Identity backfill.

@coveralls
Copy link

coveralls commented Nov 28, 2018

Pull Request Test Coverage Report for Build 718

  • 1 of 4 (25.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.02%) to 71.354%

Changes Missing Coverage Covered Lines Changed/Added Lines %
handlers/identity-backfill/src/main/scala/com/gu/identityBackfill/TypeConvert.scala 1 4 25.0%
Files with Coverage Reduction New Missed Lines %
lib/restHttp/src/main/scala/com/gu/util/resthttp/RestRequestMaker.scala 1 72.13%
lib/handler/src/main/scala/com/gu/util/apigateway/ApiGatewayHandler.scala 1 80.95%
handlers/batch-email-sender/src/main/scala/com/gu/batchemailsender/api/batchemail/Handler.scala 10 0.0%
Totals Coverage Status
Change from base Build 698: 0.02%
Covered Lines: 1918
Relevant Lines: 2688

💛 - Coveralls

@pvighi
Copy link
Contributor

pvighi commented Nov 28, 2018

Some endpoints are hit directly by end users, digital-subscription-expiry comes to mind but there might be others as well.
Whether there are secrets or not in the response is hard to tell since the error message depends on exceptions that could be generated in many different places.

I don't think this is the commonly expected behaviour and since this project is the default location for our future lambda based apis I would prefer not to do that.
But that's just my opinion,
@johnduffell @paulbrown1982 @twrichards @jacobwinch : what do you think?

def toApiGatewayOp(action: String): ApiGatewayOp[A] = clientOp.toDisjunction.toApiGatewayOp { error =>
logger.error(s"Failed to $action: $error")
badRequest(s"Failed to execute lambda - unable to $action")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are changing it only for identity backfill then I would also be ok with you returning the error description in a 500 if you think that suits it better.
I just didn't think we should do it globally for all apis

@johnduffell
Copy link
Member

agree with patricio, if it's going to be a protected endpoint indefinitely then returning directly is OK 👍 otherwise it's risky.
If we had better logging e.g. ELK it would be less of an issue though - that's a future TODO though.

Copy link
Contributor

@pvighi pvighi left a comment

Choose a reason for hiding this comment

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

👍 I think error 500 would be better if it is not an actual bad request but I'll leave it up to you

@QuarpT QuarpT merged commit e97ca18 into master Nov 28, 2018
@QuarpT QuarpT deleted the error-message-on-500 branch November 28, 2018 16:38
@johnduffell johnduffell changed the title Return error message on 500s make identity-backfill pass on zuora errors as 400 Nov 28, 2018
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.

None yet

4 participants