Skip to content

Comments

feat(services/onedrive): implement read_with_if_none_match#5763

Merged
Xuanwo merged 3 commits intoapache:mainfrom
erickguan:onedrive-features
Mar 15, 2025
Merged

feat(services/onedrive): implement read_with_if_none_match#5763
Xuanwo merged 3 commits intoapache:mainfrom
erickguan:onedrive-features

Conversation

@erickguan
Copy link
Member

@erickguan erickguan commented Mar 13, 2025

Which issue does this PR close?

Part of #5486.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

I encountered an issue with the HTTP client when working with OneDrive. Specifically, OneDrive returns a 302 Location response when downloading an item. See onedrive_get_content. This has two implications:

  1. OpenDAL cannot implement "presign" for OneDrive without controlling or requiring the HTTP client to handle retries. While this is not an urgent issue, it may be worth tracking in an issue to gather requirements for the HTTP client.
  2. In my implementation, I must assume that the HTTP client will automatically follow 302 redirects. If redirection does not occur, the OneDrive backend will return an error, which is not ideal. This is also not a good position for developers building services where developers can't predict how http clients will perform with redirections.

@erickguan erickguan requested a review from Xuanwo as a code owner March 13, 2025 20:36
@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Mar 13, 2025
@erickguan
Copy link
Member Author

erickguan commented Mar 13, 2025

Behavior tests pass.

OPENDAL_TEST=onedrive cargo test behavior --features tests,services-onedrive -- --show-output 
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.16s
     Running unittests src/lib.rs (target/debug/deps/opendal-274f328c08983e2c)

running 0 tests

successes:

successes:

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 136 filtered out; finished in 0.00s

     Running tests/behavior/main.rs (target/debug/deps/behavior-452541199097b1cc)

running 105 tests
test behavior::test_delete_with_version                       ... ok
test behavior::test_delete_with_not_existing_version          ... ok
test behavior::test_batch_delete                              ... ok
test behavior::test_batch_delete_with_version                 ... ok
test behavior::test_list_with_start_after                     ... ok
test behavior::test_list_with_versions_and_start_after        ... ok
test behavior::test_list_files_with_deleted                   ... ok
test behavior::test_list_files_with_versions                  ... ok
test behavior::test_reader_with_if_modified_since             ... ok
test behavior::test_reader_with_if_match                      ... ok
test behavior::test_reader_with_if_unmodified_since           ... ok
test behavior::test_read_with_if_match                        ... ok
test behavior::test_read_with_if_modified_since               ... ok
test behavior::test_read_with_if_unmodified_since             ... ok
test behavior::test_list_with_versions_and_limit              ... ok
test behavior::test_read_with_override_cache_control          ... ok
test behavior::test_read_with_override_content_disposition    ... ok
test behavior::test_read_with_override_content_type           ... ok
test behavior::test_read_with_version                         ... ok
test behavior::test_read_with_not_existing_version            ... ok
test behavior::test_list_non_exist_dir_with_recursive         ... ok
test behavior::test_read_not_exist                            ... ok
test behavior::test_list_non_exist_dir                        ... ok
test behavior::test_delete_not_existing                       ... ok
test behavior::test_delete_empty_dir                          ... ok
test behavior::test_read_with_dir_path                        ... ok
test behavior::test_stat_with_if_match                        ... ok
test behavior::test_stat_with_if_none_match                   ... ok
test behavior::test_stat_with_if_modified_since               ... ok
test behavior::test_stat_with_if_unmodified_since             ... ok
test behavior::test_stat_with_override_cache_control          ... ok
test behavior::test_stat_with_override_content_disposition    ... ok
test behavior::test_stat_with_override_content_type           ... ok
test behavior::test_stat_root                                 ... ok
test behavior::test_stat_with_version                         ... ok
test behavior::stat_with_not_existing_version                 ... ok
test behavior::test_stat_not_exist                            ... ok
test behavior::test_write_with_empty_content                  ... ok
test behavior::test_write_with_dir_path                       ... ok
test behavior::test_create_dir                                ... ok
test behavior::test_write_with_cache_control                  ... ok
test behavior::test_write_with_content_type                   ... ok
test behavior::test_write_with_content_disposition            ... ok
test behavior::test_write_with_content_encoding               ... ok
test behavior::test_write_with_if_none_match                  ... ok
test behavior::test_write_with_if_not_exists                  ... ok
test behavior::test_write_with_if_match                       ... ok
test behavior::test_write_with_user_metadata                  ... ok
test behavior::test_read_with_special_chars                   ... ok
test behavior::test_writer_write                              ... ok
test behavior::test_create_dir_existing                       ... ok
test behavior::test_writer_write_with_concurrent              ... ok
test behavior::test_writer_sink                               ... ok
test behavior::test_writer_sink_with_concurrent               ... ok
test behavior::test_check                                     ... ok
test behavior::test_stat_nested_parent_dir                    ... ok
test behavior::test_writer_futures_copy                       ... ok
test behavior::test_writer_futures_copy_with_concurrent       ... ok
test behavior::test_writer_return_metadata                    ... ok
test behavior::test_stat_not_cleaned_path                     ... ok
test behavior::test_read_range                                ... ok
test behavior::test_writer_abort_with_concurrent              ... ok
test behavior::test_writer_abort                              ... ok
test behavior::test_stat_dir                                  ... ok
test behavior::test_write_only                                ... ok
test behavior::test_stat_with_special_chars                   ... ok
test behavior::test_delete_with_special_chars                 ... ok
test behavior::test_list_prefix                               ... ok
test behavior::test_read_with_if_none_match                   ... ok
test behavior::test_blocking_read_not_exist                   ... ok
test behavior::test_reader                                    ... ok
test behavior::test_blocking_write_with_dir_path              ... ok
test behavior::test_list_dir_with_file_path                   ... ok
test behavior::test_write_returns_metadata                    ... ok
test behavior::test_delete_file                               ... ok
test behavior::test_list_sub_dir                              ... ok
test behavior::test_blocking_create_dir                       ... ok
test behavior::test_blocking_stat_not_exist                   ... ok
test behavior::test_list_file_with_recursive                  ... ok
test behavior::test_blocking_create_dir_existing              ... ok
test behavior::test_blocking_delete_file                      ... ok
test behavior::test_stat_file                                 ... ok
test behavior::test_blocking_remove_one_file                  ... ok
test behavior::test_write_with_special_chars                  ... ok
test behavior::test_read_full                                 ... ok
test behavior::test_blocking_stat_file                        ... ok
test behavior::test_blocking_write_with_special_chars         ... ok
test behavior::test_blocking_stat_with_special_chars          ... ok
test behavior::test_blocking_read_range                       ... ok
test behavior::test_blocking_stat_dir                         ... ok
test behavior::test_list_dir                                  ... ok
test behavior::test_reader_with_if_none_match                 ... ok
test behavior::test_blocking_write_returns_metadata           ... ok
test behavior::test_blocking_read_full                        ... ok
test behavior::test_blocking_write_file                       ... ok
test behavior::test_remove_one_file                           ... ok
test behavior::test_list_nested_dir                           ... ok
test behavior::test_writer_write_with_overwrite               ... ok
test behavior::test_list_dir_with_recursive_no_trailing_slash ... ok
test behavior::test_list_dir_with_recursive                   ... ok
test behavior::test_remove_all                                ... ok
test behavior::test_list_rich_dir                             ... ok
test behavior::test_list_root_with_recursive                  ... ok
test behavior::test_list_empty_dir                            ... ok
test behavior::test_delete_stream                             ... ok

test result: ok. 105 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 164.10s

Xuanwo
Xuanwo reviewed Mar 14, 2025
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me!

core/src/services/onedrive/backend.rs Outdated
let response = self.core.onedrive_get_content(path, args.range()).await?;
let response = self
.core
.onedrive_get_content(path, args.range(), args.if_none_match())
Copy link
Member

@Xuanwo Xuanwo Mar 14, 2025

Choose a reason for hiding this comment

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

How about using OpRead directly?

Copy link
Member Author

@erickguan erickguan Mar 14, 2025

Choose a reason for hiding this comment

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

Good idea!

core/src/services/onedrive/backend.rs Outdated
StatusCode::OK | StatusCode::PARTIAL_CONTENT => {
Ok((RpRead::default(), response.into_body()))
}
StatusCode::NOT_MODIFIED => {
Copy link
Member

@Xuanwo Xuanwo Mar 14, 2025

Choose a reason for hiding this comment

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

Maybe we can handle this in parse_error?

Copy link
Member Author

@erickguan erickguan Mar 14, 2025

Choose a reason for hiding this comment

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

Ok.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 14, 2025

2. In my implementation, I must assume that the HTTP client will automatically follow 302 redirects. If redirection does not occur, the OneDrive backend will return an error, which is not ideal. This is also not a good position for developers building services where developers can't predict how http clients will perform with redirections.

This assumition is mostly true.

@erickguan
Copy link
Member Author

erickguan commented Mar 14, 2025

Thanks! Then I will add documentation on the client to remember this.

@erickguan
Copy link
Member Author

erickguan commented Mar 14, 2025

Updated:

OPENDAL_TEST=onedrive cargo test behavior --features tests,services-onedrive -- --show-output 
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running unittests src/lib.rs (target/debug/deps/opendal-274f328c08983e2c)

running 0 tests

successes:

successes:

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 136 filtered out; finished in 0.00s

     Running tests/behavior/main.rs (target/debug/deps/behavior-452541199097b1cc)

running 105 tests
test behavior::test_batch_delete_with_version                 ... ok
test behavior::test_delete_with_not_existing_version          ... ok
test behavior::test_batch_delete                              ... ok
test behavior::test_delete_with_version                       ... ok
test behavior::test_list_with_start_after                     ... ok
test behavior::test_list_files_with_versions                  ... ok
test behavior::test_list_with_versions_and_start_after        ... ok
test behavior::test_list_files_with_deleted                   ... ok
test behavior::test_list_with_versions_and_limit              ... ok
test behavior::test_reader_with_if_match                      ... ok
test behavior::test_reader_with_if_modified_since             ... ok
test behavior::test_reader_with_if_unmodified_since           ... ok
test behavior::test_read_with_if_match                        ... ok
test behavior::test_read_with_if_modified_since               ... ok
test behavior::test_read_with_if_unmodified_since             ... ok
test behavior::test_read_with_override_cache_control          ... ok
test behavior::test_read_with_override_content_disposition    ... ok
test behavior::test_read_with_override_content_type           ... ok
test behavior::test_read_with_version                         ... ok
test behavior::test_read_with_not_existing_version            ... ok
test behavior::test_list_non_exist_dir_with_recursive         ... ok
test behavior::test_read_not_exist                            ... ok
test behavior::test_delete_not_existing                       ... ok
test behavior::test_list_non_exist_dir                        ... ok
test behavior::test_read_with_dir_path                        ... ok
test behavior::test_create_dir                                ... ok
test behavior::test_stat_with_if_match                        ... ok
test behavior::test_stat_with_if_none_match                   ... ok
test behavior::test_stat_with_if_modified_since               ... ok
test behavior::test_stat_with_if_unmodified_since             ... ok
test behavior::test_stat_with_override_cache_control          ... ok
test behavior::test_stat_with_override_content_disposition    ... ok
test behavior::test_stat_with_override_content_type           ... ok
test behavior::test_stat_root                                 ... ok
test behavior::test_stat_with_version                         ... ok
test behavior::stat_with_not_existing_version                 ... ok
test behavior::test_delete_empty_dir                          ... ok
test behavior::test_write_with_empty_content                  ... ok
test behavior::test_write_with_dir_path                       ... ok
test behavior::test_create_dir_existing                       ... ok
test behavior::test_write_with_cache_control                  ... ok
test behavior::test_write_with_content_type                   ... ok
test behavior::test_write_with_content_disposition            ... ok
test behavior::test_write_with_content_encoding               ... ok
test behavior::test_write_with_if_none_match                  ... ok
test behavior::test_write_with_if_not_exists                  ... ok
test behavior::test_write_with_if_match                       ... ok
test behavior::test_write_with_user_metadata                  ... ok
test behavior::test_stat_nested_parent_dir                    ... ok
test behavior::test_writer_write                              ... ok
test behavior::test_read_with_special_chars                   ... ok
test behavior::test_writer_write_with_concurrent              ... ok
test behavior::test_writer_sink                               ... ok
test behavior::test_writer_sink_with_concurrent               ... ok
test behavior::test_stat_not_exist                            ... ok
test behavior::test_delete_with_special_chars                 ... ok
test behavior::test_writer_futures_copy                       ... ok
test behavior::test_writer_futures_copy_with_concurrent       ... ok
test behavior::test_writer_return_metadata                    ... ok
test behavior::test_stat_with_special_chars                   ... ok
test behavior::test_list_root_with_recursive                  ... ok
test behavior::test_delete_file                               ... ok
test behavior::test_writer_abort_with_concurrent              ... ok
test behavior::test_writer_abort                              ... ok
test behavior::test_reader_with_if_none_match                 ... ok
test behavior::test_stat_not_cleaned_path                     ... ok
test behavior::test_stat_dir                                  ... ok
test behavior::test_check                                     ... ok
test behavior::test_write_returns_metadata                    ... ok
test behavior::test_read_full                                 ... ok
test behavior::test_read_range                                ... ok
test behavior::test_blocking_write_with_dir_path              ... ok
test behavior::test_write_only                                ... ok
test behavior::test_blocking_read_not_exist                   ... ok
test behavior::test_list_file_with_recursive                  ... ok
test behavior::test_list_dir                                  ... ok
test behavior::test_blocking_stat_not_exist                   ... ok
test behavior::test_stat_file                                 ... ok
test behavior::test_list_sub_dir                              ... ok
test behavior::test_write_with_special_chars                  ... ok
test behavior::test_reader                                    ... ok
test behavior::test_blocking_remove_one_file                  ... ok
test behavior::test_blocking_stat_dir                         ... ok
test behavior::test_blocking_create_dir                       ... ok
test behavior::test_blocking_write_with_special_chars         ... ok
test behavior::test_blocking_create_dir_existing              ... ok
test behavior::test_list_prefix                               ... ok
test behavior::test_blocking_read_full                        ... ok
test behavior::test_read_with_if_none_match                   ... ok
test behavior::test_blocking_stat_with_special_chars          ... ok
test behavior::test_list_dir_with_file_path                   ... ok
test behavior::test_blocking_delete_file                      ... ok
test behavior::test_blocking_write_returns_metadata           ... ok
test behavior::test_blocking_write_file                       ... ok
test behavior::test_blocking_read_range                       ... ok
test behavior::test_remove_one_file                           ... ok
test behavior::test_blocking_stat_file                        ... ok
test behavior::test_writer_write_with_overwrite               ... ok
test behavior::test_list_nested_dir                           ... ok
test behavior::test_list_dir_with_recursive                   ... ok
test behavior::test_list_dir_with_recursive_no_trailing_slash ... ok
test behavior::test_remove_all                                ... ok
test behavior::test_list_rich_dir                             ... ok
test behavior::test_list_empty_dir                            ... ok
test behavior::test_delete_stream                             ... ok

test result: ok. 105 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 150.33s

@erickguan erickguan requested a review from Xuanwo March 14, 2025 20:19
Xuanwo
Xuanwo approved these changes Mar 15, 2025
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo 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!

@Xuanwo Xuanwo merged commit 1ecd699 into apache:main Mar 15, 2025
247 checks passed
@erickguan erickguan deleted the onedrive-features branch March 15, 2025 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees

No one assigned

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat"

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erickguan @Xuanwo