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(administration service): change error handling #1192

Conversation

AnuragNagpure
Copy link
Contributor

@AnuragNagpure AnuragNagpure commented Dec 2, 2024

Description

Implement new error handling in below mentioned Administrative Services Controller and respective Business Logic
CompanyDataController
DocumentsController
IdentityProviderController
NetworkController
PartnerNetworkController
RegistrationController
RegistrationStatusController
ServiceAccountController
SubscriptionConfigurationController

All supported exceptions need to get transferred to the new error response method.

Why

Implement for all controller and its business logic the new error handling - see details regarding the structure below.

{
"type": "string",
"title": "string",
"status": 0,
"errors": {
"additionalProp1": [
"string"
],
"additionalProp2": [
"string"
],
"additionalProp3": [
"string"
]
},
"errorId": "string",
"details": [
{
"errorCode": "string",
"type": "string",
"message": "string",
"parameters": [
{
"name": "string",
"value": "string"
}
]
}
]
}

Issue

#1183

Checklist

Please delete options that are not relevant.

  • I have followed the contributing guidelines
  • I have performed a self-review of my own code
  • I have successfully tested my changes locally
  • I have added tests that prove my changes work
  • I have checked that new and existing tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas

@AnuragNagpure AnuragNagpure added this to the Release 25.03 milestone Dec 3, 2024
Phil91
Phil91 previously requested changes Dec 3, 2024
Copy link
Member

@Phil91 Phil91 left a comment

Choose a reason for hiding this comment

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

In my opinion it would make sense to add a DefaultErrorMessageContainer to the Framework.ErrorHandling -> ErrorHandling.

This could include a ENTITY_NOT_EXIST with the following error message: {entityType} {entityId} does not exist
It could include NULL_NOT_ALLOWED with the following error message: {propertyName} must not be null
It could include VALUE_EXPECTED with the following error message: value for {headerProviderAlias} expected
It could include ENTITY_ALREADY_EXISTS with the following error message: {entityType} {entityId} already exist

maybe there are a few more which can be aligned

@Phil91 Phil91 self-assigned this Dec 3, 2024
@evegufy evegufy changed the title feat(error): error handling changes added in administrative services feat(administration service): change error handling Dec 13, 2024
@ntruchsess
Copy link
Contributor

In my opinion it would make sense to add a DefaultErrorMessageContainer to the Framework.ErrorHandling -> ErrorHandling.

This could include a ENTITY_NOT_EXIST with the following error message: {entityType} {entityId} does not exist It could include NULL_NOT_ALLOWED with the following error message: {propertyName} must not be null It could include VALUE_EXPECTED with the following error message: value for {headerProviderAlias} expected It could include ENTITY_ALREADY_EXISTS with the following error message: {entityType} {entityId} already exist

maybe there are a few more which can be aligned

@Phil91 : Certainly having generic error-types would reduce the amount of code in the backend. But the idea is that the frontend can (optionally) do more than just pop-up an internationalized error-message. If appropriate it could precisely link to error-specific information that might help the user to resolve the issue. It's also the plan to implement a unified error-handling that any fe-code getting a standard-error from the backend just would delegate to. If error-codes were generic the fe would need to pass additional context-information to the error-handler. As a result the reduced amount of code in the backend would result in higher complexity in the frontend.

@ntruchsess ntruchsess force-pushed the feature/1183-error-handling-administrative-services branch 3 times, most recently from cefc21f to 5524c15 Compare December 17, 2024 17:26
@ntruchsess ntruchsess force-pushed the feature/1183-error-handling-administrative-services branch from 5524c15 to b525446 Compare December 17, 2024 17:32
Copy link

@Phil91
Copy link
Member

Phil91 commented Dec 18, 2024

In my opinion it would make sense to add a DefaultErrorMessageContainer to the Framework.ErrorHandling -> ErrorHandling.
This could include a ENTITY_NOT_EXIST with the following error message: {entityType} {entityId} does not exist It could include NULL_NOT_ALLOWED with the following error message: {propertyName} must not be null It could include VALUE_EXPECTED with the following error message: value for {headerProviderAlias} expected It could include ENTITY_ALREADY_EXISTS with the following error message: {entityType} {entityId} already exist
maybe there are a few more which can be aligned

@Phil91 : Certainly having generic error-types would reduce the amount of code in the backend. But the idea is that the frontend can (optionally) do more than just pop-up an internationalized error-message. If appropriate it could precisely link to error-specific information that might help the user to resolve the issue. It's also the plan to implement a unified error-handling that any fe-code getting a standard-error from the backend just would delegate to. If error-codes were generic the fe would need to pass additional context-information to the error-handler. As a result the reduced amount of code in the backend would result in higher complexity in the frontend.

@ntruchsess okay got the point, I will do a rereview. I think we can merge the pr after that.

@ntruchsess ntruchsess merged commit 75db806 into eclipse-tractusx:main Dec 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: USER READY
Development

Successfully merging this pull request may close these issues.

3 participants