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.

Sign up for GitHub

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

Jump to bottom

GH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing slash issues on hierarchical namespace accounts #40054

Merged
merged 4 commits into from
Feb 13, 2024
Merged

GH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing slash issues on hierarchical namespace accounts #40054

merged 4 commits into from
Feb 13, 2024

Conversation

Tom-Newton
Copy link
Contributor

@Tom-Newton Tom-Newton commented Feb 12, 2024 •
edited by github-actions bot
Loading

Rationale for this change

There were the following failure cases

fs->CreateDir("directory/")

fs->DeleteDir("directory/")

They fail with

Failed to delete a directory: directory/: https://tomtesthns.blob.core.windows.net/ea119933-c9d3-11ee-989a-71cec6336ac8/directory/ Azure Error: [InvalidUri] 400 The request URI is invalid.
The request URI is invalid.
RequestId:c9ad826a-101f-0005-5be0-5d0db4000000
Time:2024-02-12T18:24:12.9974541Z
Request ID: c9ad826a-101f-0005-5be0-5d0db4000000

What changes are included in this PR?

Add tests to cover these cases.
Remove trailing slashes to avoid these issues.

Are these changes tested?

Yes. I added new test cases to cover these cases. I was rather torn about whether to add precise tests like I've done or to duplicate every test case we had and run it a second time with trailing slashes.

Are there any user-facing changes?

Fixed bug on CreateDir and DeleteDir.

felipecrv
felipecrv reviewed Feb 13, 2024
View reviewed changes
cpp/src/arrow/filesystem/azurefs_test.cc Outdated
@@ -844,6 +861,21 @@ class TestAzureFileSystem : public ::testing::Test {
AssertFileInfo(fs(), blob_path, FileType::NotFound);
}

void TestDeleteDirSuccessHaveBlobWithTrailingSlash() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this to TestNonEmptyDirWithTrailingSlash. "have blob with trailing slash" can be misinterpreted as the blob path itself having a trailing slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 13, 2024
felipecrv
felipecrv approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

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

Thank you for catching and fixing this. I was careful with trailing slashes in the Move() implementation and tests, but missed this one in CreateDir.

@felipecrv felipecrv merged commit 0dbbd43 into apache:main Feb 13, 2024
35 of 36 checks passed
@felipecrv felipecrv removed the awaiting committer review Awaiting committer review label Feb 13, 2024
@conbench-apache-arrow conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 0dbbd43.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@Tom-Newton @dgreiss
apacheGH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing…
eacec51 
… slash issues on hierarchical namespace accounts (apache#40054)

### Rationale for this change
There were the following failure cases
```
fs->CreateDir("directory/")
``` 
```
fs->DeleteDir("directory/")
``` 
They fail with 
```
Failed to delete a directory: directory/: https://tomtesthns.blob.core.windows.net/ea119933-c9d3-11ee-989a-71cec6336ac8/directory/ Azure Error: [InvalidUri] 400 The request URI is invalid.
The request URI is invalid.
RequestId:c9ad826a-101f-0005-5be0-5d0db4000000
Time:2024-02-12T18:24:12.9974541Z
Request ID: c9ad826a-101f-0005-5be0-5d0db4000000
```

### What changes are included in this PR?
Add tests to cover these cases. 
Remove trailing slashes to avoid these issues. 

### Are these changes tested?
Yes. I added new test cases to cover these cases. I was rather torn about whether to add precise tests like I've done or to duplicate every test case we had and run it a second time with trailing slashes.

### Are there any user-facing changes?
Fixed bug on `CreateDir` and `DeleteDir`.

* Closes: apache#40052

Authored-by: Thomas Newton <thomas.w.newton@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Feb 28, 2024
@Tom-Newton @zanmato1984
apacheGH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing…
4565552 
… slash issues on hierarchical namespace accounts (apache#40054)

### Rationale for this change
There were the following failure cases
```
fs->CreateDir("directory/")
``` 
```
fs->DeleteDir("directory/")
``` 
They fail with 
```
Failed to delete a directory: directory/: https://tomtesthns.blob.core.windows.net/ea119933-c9d3-11ee-989a-71cec6336ac8/directory/ Azure Error: [InvalidUri] 400 The request URI is invalid.
The request URI is invalid.
RequestId:c9ad826a-101f-0005-5be0-5d0db4000000
Time:2024-02-12T18:24:12.9974541Z
Request ID: c9ad826a-101f-0005-5be0-5d0db4000000
```

### What changes are included in this PR?
Add tests to cover these cases. 
Remove trailing slashes to avoid these issues. 

### Are these changes tested?
Yes. I added new test cases to cover these cases. I was rather torn about whether to add precise tests like I've done or to duplicate every test case we had and run it a second time with trailing slashes.

### Are there any user-facing changes?
Fixed bug on `CreateDir` and `DeleteDir`.

* Closes: apache#40052

Authored-by: Thomas Newton <thomas.w.newton@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Mar 8, 2024
@Tom-Newton @thisisnic
apacheGH-40052: [C++][FS][Azure] Fix CreateDir and DeleteDir trailing…
03dbbc6 
… slash issues on hierarchical namespace accounts (apache#40054)

### Rationale for this change
There were the following failure cases
```
fs->CreateDir("directory/")
``` 
```
fs->DeleteDir("directory/")
``` 
They fail with 
```
Failed to delete a directory: directory/: https://tomtesthns.blob.core.windows.net/ea119933-c9d3-11ee-989a-71cec6336ac8/directory/ Azure Error: [InvalidUri] 400 The request URI is invalid.
The request URI is invalid.
RequestId:c9ad826a-101f-0005-5be0-5d0db4000000
Time:2024-02-12T18:24:12.9974541Z
Request ID: c9ad826a-101f-0005-5be0-5d0db4000000
```

### What changes are included in this PR?
Add tests to cover these cases. 
Remove trailing slashes to avoid these issues. 

### Are these changes tested?
Yes. I added new test cases to cover these cases. I was rather torn about whether to add precise tests like I've done or to duplicate every test case we had and run it a second time with trailing slashes.

### Are there any user-facing changes?
Fixed bug on `CreateDir` and `DeleteDir`.

* Closes: apache#40052

Authored-by: Thomas Newton <thomas.w.newton@gmail.com>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipecrv felipecrv felipecrv approved these changes

Assignees
No one assigned
Labels
Component: C++
Projects
None yet
Milestone
No milestone
2 participants
@Tom-Newton @felipecrv