ArcanePlugins / Treasury

🏦 A powerful multi-platform library for next-level plugin integrations.
https://hangar.papermc.io/ArcanePlugins/Treasury
Other
56 stars 13 forks source link

`Response`: add method to fail with a given exception #251

Closed lokka30 closed 1 year ago

lokka30 commented 1 year ago

Often times, I am writing code to create a failed response caused by a general exception being caught when attempting to do something like retrieving a balance. It would be nice if Response had a static method which creates a failed response with a given exception object.

This could be better implemented as a static method in FailureReason: FailureReason#of(Exception)

I encourage you to read this comment for a better explanation of why this may be useful.

I have labelled this issue as a good first issue, since it should be quite an easy job for a keen first-time contributor.

Please leave a comment describing your opinion on this matter below.

Approvals:

uraurora commented 1 year ago

Hi, I found this issue though the 'https://goodfirstissues.com/'. just some experience, we rarely use the entity with exception object in our daily coding work, since it's not necessary. We usually use the following two methods:

  1. status code or error enum, in this case you have to manage it manually, after all, you need to consider all the situations.
  2. custom exception and exception handler, use 'event bus' or interceptor to catch the fatal error and handler it. As a backstop, maybe a global handler is needed.

Perhaps the only sure thing is to consider all error cases , of course a static method is quite helpful Any other methods please let me know :)

lokka30 commented 1 year ago

Hi, welcome! Glad to hear that the web embraces the good first issue label. 😄

Treasury does not deal with any exceptions itself - it uses a response object which can either 'succeed' or 'fail', with a result or failure reason being provided for those respective outcomes. These Response objects are (at the moment, always) wrapped within a CompletableFuture, which in combination, provides a way for a plugin to make a query to a service provider, which 'will be responded to at some point in the future, and the response may or may not be successful'.

This issue proposes a static method which instantiates a failed Response through supplying a given Exception object.

Thanks

MrIvanPlays commented 1 year ago

exceptions should be handled via futures as there proper code and proper methods exist. if someone makes it so that his code swallows exceptions, not our problem.

lokka30 commented 1 year ago

exceptions should be handled via futures as there proper code and proper methods exist. if someone makes it so that his code swallows exceptions, not our problem.

So the design will be that unexpected exceptions -> handled by CFs, forced failures by the economy provider -> handled by responses? I'm all good with this, in fact, it may make it easier to implement. Though, I am concerned about having to handle errors in two separate places, guess it's not really a biggie.

MrIvanPlays commented 1 year ago

exceptions such as sqlexception (when handling with a sql type database) shall be handled by the economy implementation and sent through as failure response, whilst unexpected exceptions (such as NPEs) shall be handled via futures.

lokka30 commented 1 year ago

Thanks, reshaped this issue into #256 so that the failure behaviour is clarified. 🙂