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

Fix objects with spaces in name not returned correctly fixes #1100 #1101

Merged

Conversation

heedfull
Copy link
Contributor

@heedfull heedfull commented Jun 5, 2024

The Uri.UnescapeDataString method has been replaced with the HttpUtility.UrlDecode method in the BucketOperations.cs file to correctly decode the item Key. Additionally, the ListObjects_Test7 function has been added to the functional tests to test whether ListObjects lists all objects matching a prefix with a space in a non-recursive manner.

The Uri.UnescapeDataString method has been replaced with the HttpUtility.UrlDecode method in the BucketOperations.cs file to correctly decode the URL. Additionally, the ListObjects_Test7 function has been added to the Program.cs and FunctionalTest.cs files to test whether ListObjects lists all objects matching a prefix with a space in a non-recursive manner.
Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

We need to add support for encoding-type=url for List() and keep the URL escape.

@heedfull
Copy link
Contributor Author

heedfull commented Jun 5, 2024

I'm not really sure what you mean. The new functional test I added fails without the change submitted. This is the same fix that was used for #343

There are no new failing tests because of these changes

@heedfull
Copy link
Contributor Author

heedfull commented Jun 6, 2024

We need to add support for encoding-type=url for List() and keep the URL escape.

that's not what I was hoping to achieve with this PR. I want to add support for objects with spaces in their name. Maybe if you can give more details in separate issue then I can take a look.

@harshavardhana
Copy link
Member

We need to add support for encoding-type=url for List() and keep the URL escape.

that's not what I was hoping to achieve with this PR. I want to add support for objects with spaces in their name. Maybe if you can give more details in separate issue then I can take a look.

Try uploading an object with + in its name this PR will break.

@heedfull
Copy link
Contributor Author

heedfull commented Jun 6, 2024

I've added a test for a prefix with a plus character in it. It passes before and after the change.

@heedfull heedfull force-pushed the ListObjectsEnumAsync-files-with-spaces branch from d431df7 to 7e07ca1 Compare June 6, 2024 12:46
@heedfull heedfull changed the title Fix objects with spaces in name not returned correctly (#1100) Fix objects with spaces in name not returned correctly (fixes #1100) Jun 6, 2024
@heedfull heedfull changed the title Fix objects with spaces in name not returned correctly (fixes #1100) Fix objects with spaces in name not returned correctly fixes #1100 Jun 6, 2024
@harshavardhana
Copy link
Member

The reason this works is its already added

requestMessageBuilder.AddQueryParameter("encoding-type", "url");

harshavardhana
harshavardhana previously approved these changes Jun 6, 2024
@heedfull heedfull closed this Jun 11, 2024
@heedfull heedfull reopened this Jun 11, 2024
@heedfull
Copy link
Contributor Author

Anything I can do to help get this PR merged?

@ebozduman
Copy link
Collaborator

ebozduman commented Jun 13, 2024

Sorry @heedfull
I did not notice the progress in this PR.
Running the build tests now.
As soon as the build test is complete, I'll merge it.

@ebozduman ebozduman merged commit 7a6533d into minio:master Jun 13, 2024
14 checks passed
@heedfull
Copy link
Contributor Author

no worries, thanks for merging.

Any plans to ship a new version of nuget package?

@ebozduman
Copy link
Collaborator

@heedfull

Any plans to ship a new version of nuget package?

Yes. It is in the plan.
There is a regression problem we are working on.
A new nuget package will be generated as soon as the regression issue is resolved.

@heedfull
Copy link
Contributor Author

do you have a failing test for the regression? anything I can help with?

@ebozduman
Copy link
Collaborator

@heedfull

Thank you.
I filed issue #1105 for it.
The culprit is a commit in PR#1057. I have a fix and will create the PR in a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants