feat: initial `Operator::from_uri` implementation #5482

jorgehermo9 · 2024-12-30T19:41:37Z

Relates to #5445

Left some doubts as // TODO in the source. I have little experience contributing to this repo so I'm sorry if there are a lot of doubts about this. Just want to be sure all the changes of this PR aligns with the current codebase. Please take a look to all the TODOs I left when reviewing.

I would like to add more tests, but I don't know in which place those should be placed. The core/tests folder seems like a good place, but I don't find any place suitable, as placing those in core/tests/behaviour seems weird to me. But as this implies various components, maybe we can have a core/tests/integration? Although I would like to write some unit tests at core/src/types/builder.rs, core/src/types/operator/builder.rs and core/src/types/operator/registry.rs, but didn't any existing unit tests there.

In this PR I implemented a single Configurator::from_uri method, which will serve as default and takes only the query parameters as options. Services which need a more specific configuration such as s3 or azblob can be implemented in follow-up PRs.

I also have pending documentating all the newly added public API, but will do that after an initial review round.

Thank you very much.

core/src/types/operator/builder.rs

core/src/types/operator/registry.rs

jorgehermo9 · 2024-12-30T19:55:18Z

core/src/types/operator/registry.rs

@@ -0,0 +1,226 @@
+use std::cell::LazyCell;


I added this file inside the crate::types::operator::registry module. Is it ok? I thought about adding a crate::types::operator_registry, but it seemed better this way.

jorgehermo9 · 2024-12-30T19:56:25Z

core/src/types/operator/registry.rs

+
+// TODO: thread local or use LazyLock instead? this way the access is lock-free
+// TODO: should we expose the `GLOBAL_OPERATOR_REGISTRY` as public API at `crate::types::operator::GLOBAL_OPERATOR_REGISTRY`?
+thread_local! {


What is the preferred way of having a global static variable such as this?

I prefer to have it thread_local so there is not need for a LazyLock, we can use LazyCell instead (LazyCell is lock-free but LazyLock is not, it synchronizes access through threads)

just curious, why do we need static var here?

In order to have a global initialized registry and to not have to initialize it every time whenever we want to parse a uri

opendal/core/src/types/operator/builder.rs

Line 118 in 088d7d0

GLOBAL_OPERATOR_REGISTRY.with(|registry| registry.parse(uri, options))

jorgehermo9 · 2024-12-30T20:08:58Z

core/src/types/operator/registry.rs

+// TODO: thread local or use LazyLock instead? this way the access is lock-free
+// TODO: should we expose the `GLOBAL_OPERATOR_REGISTRY` as public API at `crate::types::operator::GLOBAL_OPERATOR_REGISTRY`?
+thread_local! {
+    pub static GLOBAL_OPERATOR_REGISTRY: LazyCell<OperatorRegistry> = LazyCell::new(|| OperatorRegistry::with_enabled_services());


MSRV check fails due to the usage of LazyCell. Should we update the MSRV or use another thing instead?

I see this TODO about the once_cell crate usage:

opendal/core/Cargo.toml

Line 248 in 52c96bb

# TODO: remove once_cell when lazy_lock is stable: https://doc.rust-lang.org/std/cell/struct.LazyCell.html

If the replacement is planned, I think it would be better to use LazyCell than once_cell::Lazy in new code like this one, to not increase technical debt.

core/Cargo.toml

core/src/types/builder.rs

core/src/types/operator/registry.rs

core/src/types/builder.rs

core/src/types/operator/registry.rs

core/src/types/operator/mod.rs

some temp fix

asukaminato0721 · 2025-05-17T19:38:26Z

core/src/raw/http_util/uri.rs

+/// query_pairs will parse a query string encoded as key-value pairs separated by `&` to a vector of key-value pairs.
+/// It also does percent decoding for both key and value.
+///
+/// Note that `?` is not allowed in the query string, and it will be treated as a part of the first key if included.
+pub fn query_pairs(query: &str) -> Vec<(String, String)> {
+    query
+        .split('&')
+        .filter_map(|pair| {
+            let mut iter = pair.splitn(2, '=');
+            // TODO: should we silently ignore invalid pairs and filter them out without the user noticing?
+            // or should we return an error in the whole `query_pairs` function so the caller knows it failed?
+            let key = iter.next()?;
+            let value = iter.next().unwrap_or("");
+            Some((key, value))
+        })
+        .map(|(key, value)| (percent_decode_path(key), percent_decode_path(value)))
+        .collect()
+}
+


we have

pub struct QueryPairsWriter {

now, use this.

I don't get what you mean. QueryPairsWriter is used to write query param pairs to an url, not for reading them. The function query_pairs this PR adds is to *read query param pairs` from an url

core/src/types/operator/registry.rs

core/src/types/operator/mod.rs

core/src/types/builder.rs

core/src/types/operator/builder.rs

core/src/types/operator/registry.rs

asukaminato0721 · 2025-05-17T20:14:10Z

so many "todo"...

keep the code change small, clippy happy, test pass, and the code style unified (you can use llm for this kind of idea)

then you can answer many todos by yourself. :)

jorgehermo9 · 2025-05-18T16:37:43Z

Thank you very much for your review @asukaminato0721! I usually leave the TODOs in code as comments to the reviewer, will address everything asap!

jorgehermo9 · 2025-05-22T22:05:52Z

core/src/types/builder.rs

+    fn from_uri(uri: &Uri, options: impl IntoIterator<Item = (String, String)>) -> Result<Self> {
+        let query_pairs = uri.query().map(query_pairs).unwrap_or_default();
+
+        let merged_options = query_pairs.into_iter().chain(options);


Unsure about
let merged_options = query_pairs.into_iter().chain(options);
or
let merged_options = options.chain(query_pairs.into_iter());

Which one should take precedence? query_pairs should overwrite the options, or should the options overwrite the query_pairs? I'm thinking on the case that query_pairs and options contain the same key

Right now, I think that the behaviour with query_pairs.into_iter().chain(options) is that options takes precedence and them overwrite whatever goes in query_pairs if a key is present in both

jorgehermo9 · 2025-05-22T22:13:56Z

core/src/types/operator/mod.rs

+pub use registry::OperatorFactory;
+pub use registry::OperatorRegistry;
+pub use registry::GLOBAL_OPERATOR_REGISTRY;


should we expose these 3 as public api @Xuanwo? or should we pub(crate) them instead?

Or should we only expose OperatorFactory and OperatorRegistry but not GLOBAL_OPERATOR_REGISTRY?

jorgehermo9 · 2025-05-22T22:16:18Z

Hi @Xuanwo, I worked a bit more on this and I think it is ready for another review round. Once the implementation looks good to you, I will work on tests and documentation, is this okay?

@asukaminato0721 I think I addressed most of your comments

jorgehermo9 · 2025-05-22T22:18:08Z

core/src/types/builder.rs

@@ -137,6 +148,15 @@ pub trait Configurator: Serialize + DeserializeOwned + Debug + 'static {
        })
    }

+    /// TODO: document this.
+    fn from_uri(uri: &Uri, options: impl IntoIterator<Item = (String, String)>) -> Result<Self> {


Is it okay to accept &Uri here? This way, we don't have to parse the uri again (it is only done here right now)

Or should we change it to &str and parse the uri twice?

jorgehermo9 added 7 commits December 30, 2024 13:27

feat: initial OperatorRegistry implementation

f2267e6

feat: continue with operator registry implementation

f581316

feat: register enabled services & set global registry

b67be38

feat: glue together Operator and global OperatorRegistry

b8c4fb1

feat: glue together Operator and global OperatorRegistry

a50c058

feat: implement Configurator::from_uri

539e6ea

test: add small doctest to Operator::from_uri

3aba98b

jorgehermo9 requested a review from Xuanwo as a code owner December 30, 2024 19:41

github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Dec 30, 2024

jorgehermo9 added 2 commits December 30, 2024 20:42

chore: add comment

5979898

chore: remove changes

6eed396

jorgehermo9 commented Dec 30, 2024

View reviewed changes

core/src/types/operator/builder.rs Show resolved Hide resolved

jorgehermo9 commented Dec 30, 2024

View reviewed changes

core/src/types/operator/registry.rs Outdated Show resolved Hide resolved

chore: remove changes

0e582ee

jorgehermo9 commented Dec 30, 2024

View reviewed changes

jorgehermo9 added 2 commits December 30, 2024 21:03

chore: add license header

9d730c0

chore: fix typo

976e60c

jorgehermo9 commented Dec 30, 2024

View reviewed changes

jorgehermo9 added 2 commits December 30, 2024 21:10

fix: clippy lint

a4478a9

retrigger ci

a4e3866

Xuanwo reviewed Dec 31, 2024

View reviewed changes

jorgehermo9 added 7 commits January 5, 2025 11:25

feat: drop usage of the url crate

443c11d

chore: remove TODO

e7215b4

feat: wrap OperatorRegistry in Arc Mutex

b6dd1dc

feat: Initialize operator registry in new method

e887d24

chore: remove todo

41790a5

Merge branch 'main' into feature/operator-from-uri

27d65d9

feat: move service registration to builder methods

3b80f70

jorgehermo9 commented Jan 5, 2025

View reviewed changes

core/src/types/builder.rs Show resolved Hide resolved