Fixes the missing exception throw action when an api request is executed, like BucketExistsAsync, etc.

[ebozduman]

Fixes #1118
and probably #1114, #1121, #1131 and #1132, ....
There may be some more issues filed for this very same issue.
If I find more, I'll add them here.
#1120 (The PR that tried to fix the issue)

[ramondeklein]

I can't really sign off on this. Code can be simplified, but even if you do, this core method changes semantics significantly and may have serious impact on API usage. Instead of returning an error response, this throws an exception instead.

[rhegner]

@ebozduman even with your suggested changes, that entire error handling does not look very trustworthy to me.

@ramondeklein already mentioned some issues.

I didn't dig very deep in the code base, but it seems to me that the root cause of all the evil is that there is a mix of throwing exceptions and returning (unthrown) exception objects within the ResponseResult. For example, with your latest code change, in most cases exception are (re-)thrown. But if the responseResult variable is null in line 147, the exception will not be thrown, but instead returned. The fact that these two approaches are mixed, makes understanding (and maintaining) the entire error handling logic pretty difficult and fragile. [The fact that error handling is broken in various ways across multiple versions of the library is the best proof for that.]

My suggestion would be this (again, I don't really know the code base, so I might miss something):
Remove the Exception field in ResponseResult completely and transport exceptions only by throwing them. I think this would streamline the entire exception handling logic (and I think the required changes are limited to only a small part of the codebase).

---

Additional thoughts:

I could imagine that the original rationale for transporting exceptions inside of ResponseResult instead of throwing was to facilitate the idea of invoking "error handlers".
BUT: That mechanism seems to be broken anyways, see HandleIfErrorResponse method. If there is an exception, the method throws in line 386, so the handlers will never be invoked in line 391.

So, again, I think the entire error handling needs simplification and streamlining. Let all exceptions throw. If you want to keep the concept of error handlers, invoke them from within a catch block in an appropriate place, and then re-throw.

[COCPORN]

Just merge the obvious fix.

This is embarrassing.

[rhegner]

@ebozduman these are the results of my testbench:

// Minio Version                                6.0.0   6.0.1   6.0.2   6.0.3   PR
// ------------------------------------------------------------------------------------------------
await TestExistsWithExistingObject();       //  ok      ok      ok      ok      ok
await TestExistsWithNonexistingObject();    //  ok      NOK     ok      ok      ok
await TestExistsFromInvalidEndpoint();      //  ok      ok      NOK     NOK     different exception
await TestDownloadWithExistingObject();     //  ok      ok      ok      ok      ok
await TestDownloadWithNonexistingObject();  //  ok      ok      ok      ok      ok
await TestDownloadFromInvalidEndpoint();    //  ok      ok      NOK     NOK     different exception

Previous versions did throw a ConnectionException, with your latest code a HttpRequestException is thrown.
In our environment it would probably be ok to receive a different exception in these cases, but generally I would consider this a breaking change.

[ebozduman]

@rhegner,
Thank you very much for testing the fix.
I'll look into the regressed exceptions.
I am also running our functional tests against 3 scenarios:

• Nonexisting endpoint
• Invalid access key
• Invalid secret key
  I'll work on all the anomalies and regressions I see in these tests.

[harshavardhana]

@rhegner, Thank you very much for testing the fix. I'll look into the regressed exceptions. I am also running our functional tests against 3 scenarios:

• Nonexisting endpoint
• Invalid access key
• Invalid secret key
  I'll work on all the anomalies and regressions I see in these tests.

@ebozduman is this done?

[ebozduman]

@harshavardhana

It is incomplete!

Replaced the existing hack fix and cleaned up all the extra code.
Also made changes according to Ramon's suggestions.
But since this change touches everybody, testing took quite sometime.
There is another tune up that needs to be done for a test, but before finishing and testing the required change, I switched to the AI storage testing. So, it is still incomplete.

[jonahbohlmann]

@ebozduman
Maybe I'm misunderstanding this, but from my side this issue makes the library unstable and unusable.
We have already encountered unexpected scenarios in our applications several times because there are no clear error messages.

Is there anything the community can do to help resolve the issue in the short term? Maybe it is possible to release a beta version so we can test current fixes?

[ebozduman]

@strelok372
Thank you for your code review approval, but the fix requested was to fix the whole error handling code and logic and to clean up the old code. So, this fix is not good enough.
The desired fix is 80% done and verification and testing still in progress as the fix is quite big.
I've also talked and asked about marking the package as unstable until this issue is fixed, or some other solution is proposed.

[strelok372]

Okay, I understand. I think it's a good idea to mark it as unstable, because I spent some time to find an issue and should return to 6.0.1 after all. It would be useful to notice other people to save their time.00:16, 4 февраля 2025 г., ebozduman ***@***.***>: @strelok372 Thank you for your code review approval, but the fix requested was to fix the whole error handling code and logic and to clean up the old code. So, this fix is not good enough. The desired fix is 80% done and verification and testing still in progress as the fix is quite big. I've also talked and asked about marking the package as unstable until this issue is fixed, or some other solution is proposed. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***> --с уважением, Дозоров Владимир Александрович

[rhegner]

and should return to 6.0.1 after all.

6.0.0 was actually the last version that did proper error handling, see my test results above:
#1141 (comment)

[Willimaendu]

What is the status of this PR?

[ebozduman]

@Willimaend
still in code review phase

[ebozduman]

@sivanantham-gnanavel
Could you please code review this PR?
This is an important and long awaited fix.

[IgorAtUnisys]

We are waiting patiently for this PR! I really hope more ressources are put into this.

[Willimaendu]

Any news?

[ebozduman]

Comments are incorporated.

[ebozduman]

@sivanantham-gnanavel
Could you please respond or mark the changes done for your review comments as "Resolved"?

[sivanantham-gnanavel]

@sivanantham-gnanavel Could you please respond or mark the changes done for your review comments as "Resolved"?

@ebozduman Two comments are marked done but I don't see the fixes. Could you help me?

[ebozduman]

@sivanantham-gnanavel
1st comment: My old comment was not good and clear. I fixed it and it is better now.
2nd comment: added the missing change.
In the meantime, I saw a test failure during my test runs, hence I've also included the fix for this recent failure.
Please check them all.

[sivanantham-gnanavel]

Reviewed the code flow and marking it as LGTM. Leaving the approach review to others.

The old review comments are outdated.
Please review again if you have time.

[rhegner]

Hi everyone, I just ran my testbench with v6.0.5, hoping the exception handling is finally fixed.

Unfortunately it is still broken compared to 6.0.0:

// Minio Version                                6.0.0   6.0.1   6.0.2   6.0.3   6.0.4   6.0.5
// ------------------------------------------------------------------------------------------
await TestExistsWithExistingObject();       //  ok      ok      ok      ok      ok      ok
await TestExistsWithNonexistingObject();    //  ok      NOK     ok      ok      ok      ok
await TestExistsFromInvalidEndpoint();      //  ok      ok      NOK     NOK     NOK     NOK
await TestDownloadWithExistingObject();     //  ok      ok      ok      ok      ok      ok
await TestDownloadWithNonexistingObject();  //  ok      ok      ok      ok      ok      ok
await TestDownloadFromInvalidEndpoint();    //  ok      ok      NOK     NOK     NOK     NOK
await TestWithInvalidCredentials();         //  ok      ok      NOK     NOK     NOK     NOK

In the two NOK cases with an invalid endpoint, the library now throws a Minio.Exceptions.InternalClientException, whereas it used to throw a Minio.Exceptions.ConnectionException with v6.0.0, which is a breaking change.

In the last case with invalid credentials, the library now throws a System.NullReferenceException, whereas it used to throw a Minio.Exceptions.AccessDeniedException with v6.0.0, which is a breaking change.