feat: implement REST API database and table CRUD operations with DLF authentication#147
feat: implement REST API database and table CRUD operations with DLF authentication#147discivigour wants to merge 23 commits intoapache:mainfrom
Conversation
…tion - Add full REST API support for database and table operations - Implement DLF (Alibaba Cloud Data Lake Formation) authentication provider - Add bearer token authentication support - Refactor mock server for better testing - Add comprehensive test coverage for REST API operations - Fix clippy warnings and apply code formatting Database operations: - list_databases, create_database, get_database, alter_database, drop_database Table operations: - list_tables, create_table, get_table, rename_table, drop_table Authentication: - DLF authentication with signature generation - Bearer token authentication - Configurable token provider factory
- Rename BearTokenAuthProvider to BearerTokenAuthProvider for correctness - Update all references in auth module and API exports - Fix typo in naming to match Bearer token authentication standard
luoyuxia
left a comment
There was a problem hiding this comment.
@discivigour Haven't review all files. But left some comments firstly.
Makefile
Outdated
| docker compose -f dev/docker-compose.yaml down -v | ||
|
|
||
| # Code quality checks | ||
| check: |
There was a problem hiding this comment.
why add in this pr? I'd perfer to make the pr focus
.licenserc.yaml
Outdated
| spdx-id: Apache-2.0 | ||
| copyright-owner: Apache Software Foundation | ||
|
|
||
| paths-ignore: |
There was a problem hiding this comment.
why add in this pr? I'd perfer to make the pr focus
.gitignore
Outdated
| .vscode | ||
| **/.DS_Store | ||
| dist/* | ||
| crates/paimon/examples/dlf_*.rs |
There was a problem hiding this comment.
I think we can remove this since seem we have no dlf_*.rs
dev/check.sh
Outdated
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| set -e |
There was a problem hiding this comment.
why add in this pr? I'd perfer to make the pr focus
There was a problem hiding this comment.
ok, I will add the shell script in a separate pr.
luoyuxia
left a comment
There was a problem hiding this comment.
@discivigour Thanks for the pr. Left some comments. PTAL
crates/paimon/examples/rest_crud.rs
Outdated
|
|
||
| // List databases | ||
| println!("Listing databases..."); | ||
| match api.list_databases().await { |
There was a problem hiding this comment.
I'm concerned about introduing this example. Do use really care about using api ? Users only need to care about the catalog interface. So, two suggestion in here
- remove this example, and add example in following pr if we found it's useful
- keep this example, but use catalog interface.
There was a problem hiding this comment.
Got it, I will remove it and add examples of catalog interface in next pr.
| access_key_id: impl Into<String>, | ||
| access_key_secret: impl Into<String>, | ||
| security_token: Option<String>, | ||
| expiration: Option<String>, |
There was a problem hiding this comment.
nit:
change the order to
expiration_at_millis: Option<i64>,
expiration: Option<String>,
since it prefer expiration_at_millis
| .get(CatalogOptions::DLF_TOKEN_ECS_METADATA_URL) | ||
| .cloned() | ||
| .unwrap_or_else(|| { | ||
| "http://100.100.100.200/latest/meta-data/Ram/security-credentials/".to_string() |
There was a problem hiding this comment.
just double check what's http://100.100.100.200. Is it correct to hardcode it as the default value?
There was a problem hiding this comment.
I have checked it. It's defaut value for aliyun ecs token auth.
| /// - Load a new token if current token is None | ||
| /// - Refresh the token if it's about to expire (within TOKEN_EXPIRATION_SAFE_TIME_MILLIS) | ||
| fn get_token(&self) -> Result<DLFToken, String> { | ||
| if let Some(ref loader) = self.token_loader { |
There was a problem hiding this comment.
nit:
if let Some(loader) = &self.token_loader {
let need_reload = {
let token = self.token.borrow();
match token.as_ref() {
None => true,
Some(token) => match token.expiration_at_millis {
Some(expiration_at_millis) => {
let now = chrono::Utc::now().timestamp_millis();
expiration_at_millis - now < TOKEN_EXPIRATION_SAFE_TIME_MILLIS
}
None => false,
},
}
};
if need_reload {
let new_token = loader.load_token()?;
*self.token.borrow_mut() = Some(new_token);
}
}
Change this to make it more idiomatic Rust.
| /// If token_loader is configured, this method will: | ||
| /// - Load a new token if current token is None | ||
| /// - Refresh the token if it's about to expire (within TOKEN_EXPIRATION_SAFE_TIME_MILLIS) | ||
| fn get_token(&self) -> Result<DLFToken, String> { |
There was a problem hiding this comment.
rename to. get_or_refresh_token?
Also, I'm wondering can it be mut self so that we don't need RefCell<Option<DLFToken>>, RefCell feel a little hack to me considering it's not friendly to async block
There was a problem hiding this comment.
Agreed. I will change it.
| let token = match self.get_token() { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| eprintln!("Failed to get token: {}", e); |
There was a problem hiding this comment.
Is it expected we can just ignore this error and return base_header diectly?
If so, use log::info instead of eprintln
| } | ||
|
|
||
| /// Perform HTTP GET request with retry logic (sync version). | ||
| fn get(&self, url: &str) -> Result<String, String> { |
There was a problem hiding this comment.
we always use the Result define in paimon, please use crates/paimon/src/error.rs
crates/paimon/src/api/api_request.rs
Outdated
| use crate::{catalog::Identifier, spec::Schema}; | ||
|
|
||
| /// Base trait for REST requests. | ||
| pub trait RESTRequest {} |
There was a problem hiding this comment.
do we really need this trait currently?
| token_loader: Option<Arc<dyn DLFTokenLoader>>, | ||
| ) -> Self { | ||
| if token.is_none() && token_loader.is_none() { | ||
| panic!("Either token or token_loader must be provided"); |
There was a problem hiding this comment.
not panic in here, return error instead
| "{}/{}/{}/{}", | ||
| self.base_path, | ||
| Self::DATABASES, | ||
| database_name, |
There was a problem hiding this comment.
shoudn't it do encode_string?
There was a problem hiding this comment.
You're right, I missed that.
| let host = host.split('/').next().unwrap_or(""); | ||
| let host = host.split(':').next().unwrap_or(""); | ||
|
|
||
| if host.starts_with("dlfnext") || host.contains("openapi") { |
There was a problem hiding this comment.
host.contains("openapi") should not be needed, I will check with Tu Ming.
| Ok(t) => t, | ||
| Err(e) => { | ||
| eprintln!("Failed to get token: {}", e); | ||
| return base_header; |
There was a problem hiding this comment.
Agreed. I will change it.

Purpose
Linked issue: #119
This PR implements complete REST API CRUD operations and DLF authentication for the Paimon Rust project.
Key Features:
Brief change log
Core Feature Implementation
REST API CRUD Operations
DLF Authentication Support
Tests
API and Format
Documentation