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

EA-3705 adapt interfaces #47

Merged
merged 11 commits into from
Feb 20, 2024
Merged

EA-3705 adapt interfaces #47

merged 11 commits into from
Feb 20, 2024

Conversation

SrishtiSingh-eu
Copy link
Contributor

No description provided.

if (e instanceof ExternalServiceException) {
// throw gateway timeout (504) for External service call exceptions.
// This will also include the Resource Exhausted Exception
throw new LanguageDetectionException(e.getMessage(), HttpStatus.GATEWAY_TIMEOUT.value(), e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually we should have ExternalServiceException for any 5XX errors. The status code is available in the response, better reuse that value

if (LOG.isDebugEnabled()) {
LOG.debug(e.getMessage());
}
throw new LanguageDetectionException(e.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR.value(), e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

4XX errors should not be converted to 500, as they need to be fixed/handled by the client

if (e instanceof ExternalServiceException) {
// throw gateway timeout (504) for External service call exceptions.
// This will also include the Resource Exhausted Exception
throw new TranslationException(e.getMessage(), HttpStatus.GATEWAY_TIMEOUT.value(), e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually we should have ExternalServiceException for any 5XX errors. The status code is available in the response, better reuse that value

if (LOG.isDebugEnabled()) {
LOG.debug(e.getMessage());
}
throw new TranslationException(e.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR.value(), e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

4XX errors should not be converted to 500, as they need to be fixed/handled by the client. Better use exception handling by status code than by using the instanceOf

* @param languageDetectionObjs
* @return
*/
public static LangDetectRequest createLangDetectRequest(List<LanguageDetectionObj> languageDetectionObjs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks more like a factory method which can be implemented directly in the class that fires the request. There is no reason to have this as a static method

* @return
*/
public static TranslationRequest createTranslationRequest(List<TranslationObj> translationStrings) {
TranslationRequest translationRequest = new TranslationRequest();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks more like a factory method which can be implemented directly in the class that fires the request. There is no reason to have this as a static method

@gsergiu gsergiu merged commit c794c04 into develop Feb 20, 2024
1 check passed
@SrishtiSingh-eu SrishtiSingh-eu deleted the EA-3705_adaptInterfaces branch February 20, 2024 11:55
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.

2 participants