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

feat(error-handling): return status 504 instead of 500 for triplestore timeout exception (DEV-749) #2046

Merged

Conversation

BalduinLandolt
Copy link
Collaborator

@BalduinLandolt BalduinLandolt commented Apr 21, 2022

Resolves DEV-749

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

When a triplestore request times out, a generic 500 Internal Server Error is returned.
The app can therefor not apply any specific behaviour for this case.

Issue Number: DEV-749

What is the new behavior?

A specific 504 Gateway Timeout is returned to enable the App to handle this case specifically.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@BalduinLandolt BalduinLandolt marked this pull request as draft Apr 21, 2022
@BalduinLandolt BalduinLandolt self-assigned this Apr 21, 2022
@BalduinLandolt BalduinLandolt marked this pull request as ready for review Apr 21, 2022
@BalduinLandolt BalduinLandolt requested a review from kilchenmann Apr 21, 2022
Copy link
Collaborator

@kilchenmann kilchenmann left a comment

This is not a code review.
I tested it with the tweaked config that @BalduinLandolt mentioned in DEV-751 and I was able to produce a 504 timeout-error with a simple fulltext-search. So, it seems to work. Thanks

Hm, but a gravsearch query returns a 500 server error.

@sonarcloud
Copy link

@sonarcloud sonarcloud bot commented Apr 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@irinaschubert irinaschubert left a comment

LGTM

} else {
log.error(
e,
s"Couldn't parse response from triplestore:$logDelimiter$turtleStr${logDelimiter}in response to SPARQL query:$logDelimiter$sparql"
Copy link
Collaborator

@irinaschubert irinaschubert Apr 26, 2022

Choose a reason for hiding this comment

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

Why does logDelimiter once is wrapped into ${...} and once not? Can it be unified? I think both $... and ${...} are the same.

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 26, 2022

Choose a reason for hiding this comment

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

you're right, that's just how it is in the place where I copy-pasted it from... ;) I'll unify it

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 26, 2022

Choose a reason for hiding this comment

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

ah, no... this can't be changed because right after the variable there is a word, so it would consider this part of the variable name, which then could not be found... I'll have to leave it as it is, even though it looks strange

@BalduinLandolt BalduinLandolt merged commit a47096e into main Apr 26, 2022
11 checks passed
@BalduinLandolt BalduinLandolt deleted the wip/DEV-749-return-specific-statuscode-for-timeout branch Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants