feat(connect): ATL-6599 Use ZIO Failures and Defects effectively + RFC-9457 in connect

[bvoiturier]

Description:

ATL-6599 Implement the following error-handling ADRs for Connect:

• Use ZIO Failures and Defects Effectively
• Use RFC 9457 for REST API Error Reporting

Checklist:

• My PR follows the contribution guidelines of this project
• My PR is free of third-party dependencies that don't comply with the Allowlist
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have checked the PR title to follow the conventional commit specification

[github-actions]

Unit Test Results

 90 files  ±0   90 suites  ±0   20m 32s ⏱️ + 1m 14s
776 tests ±0  768 ✅ ±0  8 💤 ±0  0 ❌ ±0 
783 runs  ±0  775 ✅ ±0  8 💤 ±0  0 ❌ ±0 

Results for commit f8e56e0. ± Comparison against base commit 9a38cc9.

♻️ This comment has been updated with latest results.

[shotexa]

While I was working on Revocation PR, I've added 2 endpoints, and I wanted to hide 500 error messages just like you did it in this PR, and log them as well, here and here. Now since you've made this functionality work for every URL, please remove this .logError and .mapError calls from this endpoints 🙏 @bvoiturier

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[bvoiturier]

While I was working on Revocation PR, I've added 2 endpoints, and I wanted to hide 500 error messages just like you did it in this PR, and log them as well, here and here. Now since you've made this functionality work for every URL, please remove this .logError and .mapError calls from this endpoints 🙏 @bvoiturier

This won't actually work, @shotexa. The logic in this PR will only filter out unmanaged errors (ZIO defects) propagating through the call stack as exceptions and not the managed errors (ZIO failures) propagating via the effect's error channel.

The rule is not to type unexpected errors and let them propagate through the call stack as exceptions until they are caught "at the end of the world" and logged/obfuscated appropriately. Any managed error bubbling up to the top level via the error channel will be mapped to an ErrorResponse and reported "as is" to the user.

In your case, the code should be refactored to properly segregate managed failures and unmanaged defects. This should be done when the error-handling ADRs are applied to this part of the codebase...

[shotexa]

While I was working on Revocation PR, I've added 2 endpoints, and I wanted to hide 500 error messages just like you did it in this PR, and log them as well, here and here. Now since you've made this functionality work for every URL, please remove this .logError and .mapError calls from this endpoints 🙏 @bvoiturier

This won't actually work, @shotexa. The logic in this PR will only filter out unmanaged errors (ZIO defects) propagating through the call stack as exceptions and not the managed errors (ZIO failures) propagating via the effect's error channel.

The rule is not to type unexpected errors and let them propagate through the call stack as exceptions until they are caught "at the end of the world" and logged/obfuscated appropriately. Any managed error bubbling up to the top level via the error channel will be mapped to an ErrorResponse and reported "as is" to the user.

In your case, the code should be refactored to properly segregate managed failures and unmanaged defects. This should be done when the error-handling ADRs are applied to this part of the codebase...

So, just so I can understand it correctly. Currently if in any endpoint, error is thrown in repository layer user will receive the message similar to "could not find column bla bla bla", but we plan to hide this in one of the upcoming PR related to error handling?

[bvoiturier]

So, just so I can understand it correctly. Currently if in any endpoint, error is thrown in repository layer user will receive the message similar to "could not find column bla bla bla", but we plan to hide this in one of the upcoming PR related to error handling?

No. Actually, the present PR ensures that any thrown exception that is not caught, and therefore propagated through the call stack up to the "end of the world" (i.e. application entry point), will be reported to the user as a 500 - Internal Server Error without further details (no sensitive data leaked).

At the same time, the error response will contain a unique instance-id visible by the user and will be logged in the server console with the related stack trace, if any.

The thing is, in your code, the 500 errors are not exceptions but rather ZIO failures appearing in the effect's error channel. So I propose to fix this as part of the "error-handling in status list" refactoring effort.

[shotexa]

So, just so I can understand it correctly. Currently if in any endpoint, error is thrown in repository layer user will receive the message similar to "could not find column bla bla bla", but we plan to hide this in one of the upcoming PR related to error handling?

No. Actually, the present PR ensures that any thrown exception that is not caught, and therefore propagated through the call stack up to the "end of the world" (i.e. application entry point), will be reported to the user as a 500 - Internal Server Error without further details (no sensitive data leaked).

At the same time, the error response will contain a unique instance-id visible by the user and will be logged in the server console with the related stack trace, if any.

The thing is, in your code, the 500 errors are not exceptions but rather ZIO failures appearing in the effect's error channel. So I propose to fix this as part of the "error-handling in status list" refactoring effort.

What refactoring would be needed other than removing these extra .logError and .mapError calls? I mean since we are not catching errors in the repository layer now, any error (defect) happening there will already be returned as 500 error to the user, right? so this kind of errors won't land in the effects error channel anyway. I've added these lines just so I can filter errors coming from the repository layer, but since they are handled already, this extra lines serve no purpose. I might not be seeing something here tho.

[bvoiturier]

What refactoring would be needed other than removing these extra .logError and .mapError calls? I mean since we are not catching errors in the repository layer now, any error (defect) happening there will already be returned as 500 error to the user, right? so this kind of errors won't land in the effects error channel anyway. I've added these lines just so I can filter errors coming from the repository layer, but since they are handled already, this extra lines serve no purpose. I might not be seeing something here tho.

The refactoring is about making the status list code part (actually, step-by-step, all parts) compliant with the error-handling ADRs referenced in the description above. The present PR is about applying this to the 'connect' part/slice of the code base. And looking at the number of files changed count (35), it is not trivial. So my proposition is to include the changes you are requesting in this broader refactoring effort and not mix it with this PR.

I've added these lines just so I can filter errors coming from the repository layer, but since they are handled already, this extra lines serve no purpose.

They won't actually be handled by this PR. They are failures defined in the error channel and will reported "as is" to the user. This PR will only "obfuscate" defects/exceptions. So I think your extra lines are purposeful.