Fixed error handling in `RequestExtensions` #1057

holterbades · 2024-04-18T08:56:56Z

Response error handling was unintentionally removed in a previous commit. Added it back to the ExecuteTaskCoreAsync method. Fixed NullReferenceException-bug in the HandleIfErrorResponse method - throw response.Exception; does not make sense if the exception is null.

Fixes #1041 , #1020 , and probably #1027

holterbades · 2024-04-18T09:01:06Z

To the reviewer: I also noticed a minor codesmell: In line 110 and 113 there are null-checks to the path variable, which are unnecessary, because the assignment via Split('/') doesn't return null.

ebozduman · 2024-04-19T08:22:46Z

I'll start reviewing this in the morning, but first thing is to rebase the branch.
Please rebase.

holterbades · 2024-04-19T11:24:48Z

Maybe i gonna add some tests so that this error does not happen again.
Is there a special reason why in FunctionalTest only Streams are used as data source (GenerateStreamFromSeed)?
I would like to add a consistency check with an hash value or a full content check to the test case Minio.Functional.Tests.FunctionalTest.GetObject_Test2 so that there is a validation that the upload matches the download.

ebozduman · 2024-04-19T19:14:20Z

Sounds great @holterbades !

ebozduman · 2024-04-19T19:26:16Z

To the reviewer: I also noticed a minor codesmell: In line 110 and 113 there are null-checks to the path variable, which are unnecessary, because the assignment via Split('/') doesn't return null.

Those are not null checks.
They are there to see if there is 1 or more than 1 elements in the path.
So, 1 means the command deals with a bucket, and more than 1 element in the path means the command deals with an object.

Is there a special reason why in FunctionalTest only Streams are used as data source (GenerateStreamFromSeed)?

No, there is no special reason.

I would like to add a consistency check with an hash value or a full content check to the test case Minio.Functional.Tests.FunctionalTest.GetObject_Test2 so that there is a validation that the upload matches the download.

I think we do have size checks for an uploaded and for a downloaded object, but a hash value check would also be good.

Response error handling was unintentionally removed in a previous commit. Added it back to the `ExecuteTaskCoreAsync` method. Fixed `NullReferenceException`-bug in the `HandleIfErrorResponse` method - `throw response.Exception;` does not make sense if the exception is null.

`Split(...)` does not return `null`, so a `null`-check is not necessary when accessing `path`. Furthermore, converting an array twice to a list only to check the element count is inefficient - use `path.Length` directly instead.

holterbades · 2024-04-22T04:53:00Z

Those are not null checks.
They are there to see if there is 1 or more than 1 elements in the path.

Sure there are null-checks; I mean the ? in the path?.ToList(). If you have var path = someUri.Split('/'); in line 104, path can't be null because Split(...) does not return null. The least .Split() can return is a string[1] { "" } if the input is "".

Also it would be more efficient to access the element count of an array via path.Length and not convert it to a list twice.

I changed it accordingly.

holterbades · 2024-04-22T04:55:26Z

I would hand in changes to the tests in a separate PullRequest from my private account some time later.

ebozduman · 2024-04-22T08:49:57Z

@holterbades

You have a functional test failure in CopyObject_Test7. So, it seems to be your changes cause it.
Here are the log messages fyi:

{"Name":"minio-dotnet : CopyObject_Test7","Function":"Task<CopyObjectResult> CopyObjectAsync(CopyObjectArgs
args, CancellationToken cancellationToken = default(CancellationToken))","Description":"Tests whether CopyObject with
 negative test for modified date passes","Args":{"bucketName":"minio-dotnet-example-
a292twph5ezbqrd","objectName":"#+@+{$&@{[","destBucketName":"minio-dotnet-example-
3uvmhkjpxp0ndmg","destObjectName":"minio-dotnet-example-
yhrrlm29wq","data":"1KB","size":"1KB"},"Duration":1032,"Status":"FAIL","Alert":"Assert.AreEqual failed. Expected:
<MinIO API responded with message=At least one of the pre-conditions you specified did not hold>. Actual:<MinIO API 
responded with message=At least one of the pre-conditions you specified did not hold for object: \"/minio-dotnet-
example-3uvmhkjpxp0ndmg/minio-dotnet-example-yhrrlm29wq\">. 
","Message":"Microsoft.VisualStudio.TestTools.UnitTesting.AssertFailedException: Assert.AreEqual failed. Expected:
<MinIO API responded with message=At least one of the pre-con

The Minio-server is returning an error for a specific object (`"MinIO API responded with message=At least one of the pre-conditions you specified did not hold for object: \"/minio-dotnet- example-3uvmhkjpxp0ndmg/minio-dotnet-example-yhrrlm29wq\""`). Therefore, the error message should not be exactly the expected string, but rather it should **start** with the expected string.

If you attempt to delete a bucket if it does not exist, the API returns a `404` for the not existent bucket, which is correct. So before the attempt to remove the bucket, there should be a check if it even exists. This test-bug was introduced in commit 9772a37, which also disabled the error handling for all requests - which is the reason this bug was not discovered so far.

holterbades · 2024-04-22T11:47:42Z

I don't get it why the assertion is now failing but did not fail the last executions of this testrun before the error handling bug . But in my opinion this testcase is wrong: The server responds with an error, which is expected in this test case, but the error message which is returned is checked if it is exactly the expected message. In my opinion the test case should only check if the error message starts with the expected text.

Furthermore there was another test bug which was introduced in the same commit mentioned in #1041 and was not discovered because of the removed error handling.

The functional test run throws an `AssertFailedException` on `Assert.IsTrue` when executing using GitHub Actions - but executing locally works as expected. Without a hint what assertion is failing, it's not possible to fix a possible error.

holterbades · 2024-04-23T07:36:35Z

hmmmh ... The test is now failing at a new location. Locally this testcase is working just fine. Maybe a race condition in the test itself? I really don't think this has something to do with my fix.

holterbades · 2024-04-29T06:06:36Z

@ebozduman Hey, I would need some help with that PR. I don't know what is going on with this tests.

ebozduman · 2024-04-29T08:58:03Z

@holterbades

There is another PR waiting for to be reviewed, PR#1043, and I think it can help resolve some of these test failure issues.
Let's wait for it to be merged, then rebase your/this PR and let's see how it goes with this PR.
You are more then welcome to check it out and review it, if you have time.

holterbades · 2024-05-22T14:01:04Z

@ebozduman
Since I suspect that this #1043 will take even longer, I would strongly suggest unlisting the currently broken nuget package from nuget.org, so that no user can install this broken version.

ebozduman · 2024-05-24T02:06:47Z

I just ran dotnet regitlint on your branch for the format issue(s).
I think the build test will run smoothly and all finish successfully.
Then I can merge the PR.

ebozduman

LGTM

holterbades mentioned this pull request Apr 18, 2024

Minio 6.0.2: Unexpected Behavior with Unavailable Endpoints #1041

Closed

holterbades mentioned this pull request Apr 19, 2024

No exception when getting objects with invalid credentials #1020

Open

holterbades changed the title ~~Fixed error handling in RequestExtensions #1041~~ Fixed error handling in RequestExtensions Apr 19, 2024

holterbades force-pushed the ErrorHandlingBugfix branch from 95d4add to be10cc2 Compare April 22, 2024 04:40

Replaced inefficient array length check

a0c4344

`Split(...)` does not return `null`, so a `null`-check is not necessary when accessing `path`. Furthermore, converting an array twice to a list only to check the element count is inefficient - use `path.Length` directly instead.

holterbades added 2 commits April 22, 2024 13:25

adds lint check changes

ba7cb60

ebozduman approved these changes May 24, 2024

View reviewed changes

ebozduman merged commit 62f4e54 into minio:master May 24, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed error handling in `RequestExtensions` #1057

Fixed error handling in `RequestExtensions` #1057

holterbades commented Apr 18, 2024 •

edited

holterbades commented Apr 18, 2024

ebozduman commented Apr 19, 2024 •

edited

holterbades commented Apr 19, 2024

ebozduman commented Apr 19, 2024

ebozduman commented Apr 19, 2024 •

edited

holterbades commented Apr 22, 2024

holterbades commented Apr 22, 2024

ebozduman commented Apr 22, 2024 •

edited

holterbades commented Apr 22, 2024

holterbades commented Apr 23, 2024

holterbades commented Apr 29, 2024

ebozduman commented Apr 29, 2024

holterbades commented May 22, 2024

ebozduman commented May 24, 2024

ebozduman left a comment

Fixed error handling in RequestExtensions #1057

Fixed error handling in RequestExtensions #1057

Conversation

holterbades commented Apr 18, 2024 • edited

holterbades commented Apr 18, 2024

ebozduman commented Apr 19, 2024 • edited

holterbades commented Apr 19, 2024

ebozduman commented Apr 19, 2024

ebozduman commented Apr 19, 2024 • edited

holterbades commented Apr 22, 2024

holterbades commented Apr 22, 2024

ebozduman commented Apr 22, 2024 • edited

holterbades commented Apr 22, 2024

holterbades commented Apr 23, 2024

holterbades commented Apr 29, 2024

ebozduman commented Apr 29, 2024

holterbades commented May 22, 2024

ebozduman commented May 24, 2024

ebozduman left a comment

Choose a reason for hiding this comment

Fixed error handling in `RequestExtensions` #1057

Fixed error handling in `RequestExtensions` #1057

holterbades commented Apr 18, 2024 •

edited

ebozduman commented Apr 19, 2024 •

edited

ebozduman commented Apr 19, 2024 •

edited

ebozduman commented Apr 22, 2024 •

edited