fix: cannot share a db which is created from share, and test cases (#…

…11081) * chore: disable share statefull test and check bugs * chore: enable share statefull test and fix read share table bug * chore: enable share statefull test and fix read share table bug * fix: cannot share a db which is created from share, and test cases
databendlabs · Apr 15, 2023 · 8ade21e · 8ade21e · vercel · Apr 15, 2023
1 parent 7d37937
commit 8ade21e
Show file tree

Hide file tree

Showing 10 changed files with 104 additions and 16 deletions.
diff --git a/src/common/exception/src/exception_code.rs b/src/common/exception/src/exception_code.rs
@@ -253,6 +253,7 @@ build_exceptions! {
     UnknownShareEndpoint(2715),
     UnknownShareEndpointId(2716),
     UnknownShareTable(2717),
+    CannotShareDatabaseCreatedFromShare(2718),
 
     // Variable error codes.
     UnknownVariable(2801),

diff --git a/src/meta/api/src/share_api_impl.rs b/src/meta/api/src/share_api_impl.rs
@@ -15,6 +15,7 @@
 use chrono::DateTime;
 use chrono::Utc;
 use common_meta_app::app_error::AppError;
+use common_meta_app::app_error::CannotShareDatabaseCreatedFromShare;
 use common_meta_app::app_error::ShareAccountsAlreadyExists;
 use common_meta_app::app_error::ShareAlreadyExists;
 use common_meta_app::app_error::ShareEndpointAlreadyExists;
@@ -555,7 +556,8 @@ impl<KV: kvapi::KVApi<Error = MetaError>> ShareApi for KV {
             };
 
             let seq_and_id =
-                get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant).await?;
+                get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant, true)
+                    .await?;
 
             check_share_object(&share_meta.database, &seq_and_id, &req.object)?;
 
@@ -676,7 +678,8 @@ impl<KV: kvapi::KVApi<Error = MetaError>> ShareApi for KV {
             };
 
             let seq_and_id =
-                get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant).await?;
+                get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant, false)
+                    .await?;
 
             check_share_object(&share_meta.database, &seq_and_id, &req.object)?;
 
@@ -1503,6 +1506,7 @@ async fn get_share_object_seq_and_id(
     kv_api: &(impl kvapi::KVApi<Error = MetaError> + ?Sized),
     obj_name: &ShareGrantObjectName,
     tenant: &str,
+    grant: bool,
 ) -> Result<ShareGrantObjectSeqAndId, KVAppError> {
     match obj_name {
         ShareGrantObjectName::Database(db_name) => {
@@ -1517,6 +1521,16 @@ async fn get_share_object_seq_and_id(
             )
             .await?;
 
+            if grant && db_meta.from_share.is_some() {
+                return Err(KVAppError::AppError(
+                    AppError::CannotShareDatabaseCreatedFromShare(
+                        CannotShareDatabaseCreatedFromShare::new(
+                            db_name,
+                            format!("get_share_object_seq_and_id: {}", name_key),
+                        ),
+                    ),
+                ));
+            }
             Ok(ShareGrantObjectSeqAndId::Database(
                 db_meta_seq,
                 db_id,

diff --git a/src/meta/api/src/share_api_test_suite.rs b/src/meta/api/src/share_api_test_suite.rs
@@ -1243,6 +1243,7 @@ impl ShareApiTestSuite {
         let tenant1 = "tenant1";
         let share1 = "share1";
         let share2 = "share2";
+        let share3 = "share3";
         let db_name = "db1";
         let tbl_name = "table1";
         let share_id;
@@ -1255,6 +1256,10 @@ impl ShareApiTestSuite {
             tenant: tenant1.to_string(),
             share_name: share2.to_string(),
         };
+        let share_name3 = ShareNameIdent {
+            tenant: tenant1.to_string(),
+            share_name: share3.to_string(),
+        };
 
         info!("--- get unknown object");
         {
@@ -1435,6 +1440,34 @@ impl ShareApiTestSuite {
             info!("create database res: {:?}", res);
             assert!(res.is_ok());
 
+            // cannot share a database created from a share
+            {
+                let req = CreateShareReq {
+                    if_not_exists: false,
+                    share_name: share_name3.clone(),
+                    comment: None,
+                    create_on,
+                };
+
+                let res = mt.create_share(req).await;
+                assert!(res.is_ok());
+
+                let req = GrantShareObjectReq {
+                    share_name: share_name3.clone(),
+                    object: ShareGrantObjectName::Database(db2.to_string()),
+                    grant_on,
+                    privilege: ShareGrantObjectPrivilege::Usage,
+                };
+
+                let res = mt.grant_share_object(req).await;
+                assert!(res.is_err());
+                let err = res.unwrap_err();
+                assert_eq!(
+                    ErrorCode::CannotShareDatabaseCreatedFromShare("").code(),
+                    ErrorCode::from(err).code()
+                );
+            }
+
             let req = DropShareReq {
                 if_exists: true,
                 share_name: share_name1.clone(),

diff --git a/src/meta/app/src/app_error.rs b/src/meta/app/src/app_error.rs
@@ -502,6 +502,24 @@ impl UnknownShareEndpointId {
     }
 }
 
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, thiserror::Error)]
+#[error(
+    "CannotShareDatabaseCreatedFromShare: cannot share database {database_name} which created from share while {context}"
+)]
+pub struct CannotShareDatabaseCreatedFromShare {
+    database_name: String,
+    context: String,
+}
+
+impl CannotShareDatabaseCreatedFromShare {
+    pub fn new(database_name: impl Into<String>, context: impl Into<String>) -> Self {
+        Self {
+            database_name: database_name.into(),
+            context: context.into(),
+        }
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, thiserror::Error)]
 #[error("TxnRetryMaxTimes: Txn {op} has retry {max_retry} times, abort.")]
 pub struct TxnRetryMaxTimes {
@@ -616,6 +634,9 @@ pub enum AppError {
 
     #[error(transparent)]
     UnknownShareEndpointId(#[from] UnknownShareEndpointId),
+
+    #[error(transparent)]
+    CannotShareDatabaseCreatedFromShare(#[from] CannotShareDatabaseCreatedFromShare),
 }
 
 impl AppErrorMessage for UnknownDatabase {
@@ -775,6 +796,15 @@ impl AppErrorMessage for UnknownShareEndpointId {
     }
 }
 
+impl AppErrorMessage for CannotShareDatabaseCreatedFromShare {
+    fn message(&self) -> String {
+        format!(
+            "Cannot share database '{}' which created from share",
+            self.database_name
+        )
+    }
+}
+
 impl AppErrorMessage for TxnRetryMaxTimes {
     fn message(&self) -> String {
         format!(
@@ -864,6 +894,9 @@ impl From<AppError> for ErrorCode {
             AppError::UnknownShareEndpointId(err) => {
                 ErrorCode::UnknownShareEndpointId(err.message())
             }
+            AppError::CannotShareDatabaseCreatedFromShare(err) => {
+                ErrorCode::CannotShareDatabaseCreatedFromShare(err.message())
+            }
             AppError::TxnRetryMaxTimes(err) => ErrorCode::TxnRetryMaxTimes(err.message()),
             AppError::DuplicatedUpsertFiles(err) => ErrorCode::DuplicatedUpsertFiles(err.message()),
         }

diff --git a/src/query/sharing-endpoint/src/accessor/share_spec_accessor.rs b/src/query/sharing-endpoint/src/accessor/share_spec_accessor.rs
@@ -13,6 +13,7 @@
 // limitations under the License.
 
 use common_exception::Result;
+use common_storages_share::get_share_spec_location;
 
 use crate::accessor::SharingAccessor;
 use crate::models;
@@ -23,7 +24,7 @@ impl SharingAccessor {
     #[async_backtrace::framed]
     pub async fn get_share_spec(tenant: &String) -> Result<Vec<ShareSpec>> {
         let sharing_accessor = Self::instance();
-        let path = sharing_accessor.get_share_spec_location();
+        let path = get_share_spec_location(&sharing_accessor.config.tenant);
         let data = sharing_accessor.op.read(&path).await?;
         let share_specs: models::SharingConfig = serde_json::from_slice(data.as_slice())?;
         let mut share_spec_vec = vec![];

diff --git a/src/query/sharing-endpoint/src/accessor/share_table_accessor.rs b/src/query/sharing-endpoint/src/accessor/share_table_accessor.rs
@@ -15,7 +15,7 @@
 use std::time::Duration;
 
 use common_exception::Result;
-use common_storages_share::SHARE_CONFIG_PREFIX;
+use common_storages_share::get_share_spec_location;
 
 use crate::accessor::truncate_root;
 use crate::accessor::SharingAccessor;
@@ -35,7 +35,7 @@ impl SharingAccessor {
         input: &models::LambdaInput,
     ) -> Result<Option<SharedTableResponse>> {
         let sharing_accessor = Self::instance();
-        let path = sharing_accessor.get_share_spec_location();
+        let path = get_share_spec_location(&sharing_accessor.config.tenant);
         let data = sharing_accessor.op.read(&path).await?;
         let share_specs: models::SharingConfig = serde_json::from_slice(data.as_slice())?;
         share_specs.get_tables(input)
@@ -76,13 +76,6 @@ impl SharingAccessor {
         Ok(PresignFileResponse::new(&s, input.file_name.clone()))
     }
 
-    pub fn get_share_spec_location(&self) -> String {
-        format!(
-            "{}/{}/share_specs.json",
-            self.config.tenant, SHARE_CONFIG_PREFIX
-        )
-    }
-
     #[async_backtrace::framed]
     pub async fn get_share_table_spec_presigned_files(
         input: &models::LambdaInput,

diff --git a/src/query/storages/share/src/lib.rs b/src/query/storages/share/src/lib.rs
@@ -16,7 +16,7 @@
 
 mod share;
 
+pub use share::get_share_spec_location;
 pub use share::save_share_spec;
 pub use share::save_share_table_info;
 pub use share::share_table_info_location;
-pub use share::SHARE_CONFIG_PREFIX;
diff --git a/src/query/storages/share/src/share.rs b/src/query/storages/share/src/share.rs
@@ -23,17 +23,21 @@ use common_meta_app::share::ShareTableInfoMap;
 use common_meta_app::share::ShareTableSpec;
 use opendal::Operator;
 
-pub const SHARE_CONFIG_PREFIX: &str = "_share_config";
+const SHARE_CONFIG_PREFIX: &str = "_share_config";
 
 #[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Default, Eq, PartialEq)]
 pub struct ShareSpecVec {
     share_specs: BTreeMap<String, ext::ShareSpecExt>,
 }
 
+pub fn get_share_spec_location(tenant: &String) -> String {
+    format!("{}/{}/share_specs.json", SHARE_CONFIG_PREFIX, tenant,)
+}
+
 pub fn share_table_info_location(tenant: &str, share_name: &str) -> String {
     format!(
         "{}/{}/{}_table_info.json",
-        tenant, SHARE_CONFIG_PREFIX, share_name
+        SHARE_CONFIG_PREFIX, tenant, share_name
     )
 }
 
@@ -69,7 +73,7 @@ pub async fn save_share_spec(
     share_table_info: Option<Vec<ShareTableInfoMap>>,
 ) -> Result<()> {
     if let Some(share_spec) = spec_vec {
-        let location = format!("{}/{}/share_specs.json", tenant, SHARE_CONFIG_PREFIX);
+        let location = get_share_spec_location(tenant);
         let mut share_spec_vec = ShareSpecVec::default();
         for spec in share_spec {
             let share_name = spec.name.clone();

diff --git a/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.result b/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.result
@@ -18,6 +18,8 @@ INBOUND test_share test_database shared_tenant to_tenant
 4
 5
 6
+cannot grant a database which create from share
+ERROR 1105 (HY000) at line 1: Code: 2718, Text = Cannot share database 'shared_db' which created from share.
 alter table add column and query table data again
 1,1
 2,1

diff --git a/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.sh b/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.sh
@@ -5,6 +5,7 @@ CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 
 echo "drop test share and database"
 echo "drop share if exists test_share" | $MYSQL_CLIENT_SHARE_1_CONNECT
+echo "drop share if exists my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT
 echo "drop database if exists test_database" | $MYSQL_CLIENT_SHARE_1_CONNECT
 echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_2_CONNECT
 echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_3_CONNECT
@@ -39,6 +40,11 @@ echo "CREATE DATABASE if not exists shared_db FROM SHARE shared_tenant.test_shar
 echo "SELECT * FROM shared_db.t1" | $MYSQL_CLIENT_SHARE_2_CONNECT
 echo "SELECT * FROM shared_db.t2" | $MYSQL_CLIENT_SHARE_2_CONNECT
 
+# cannot grant a database which create from share
+echo "cannot grant a database which create from share"
+echo "CREATE SHARE my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT
+echo "GRANT USAGE ON DATABASE shared_db TO SHARE my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT
+
 # alter table and query table data again
 echo "alter table add column and query table data again"
 echo "ALTER table test_database.t1 add column b UInt64 default 1" | $MYSQL_CLIENT_SHARE_1_CONNECT
@@ -66,6 +72,7 @@ echo "SELECT * FROM shared_db.t2" | $MYSQL_CLIENT_SHARE_3_CONNECT
 ## Drop database and share.
 echo "all is good"
 echo "drop share if exists test_share" | $MYSQL_CLIENT_SHARE_1_CONNECT
+echo "drop share if exists my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT
 echo "drop database if exists test_database" | $MYSQL_CLIENT_SHARE_1_CONNECT
 echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_2_CONNECT
 echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_3_CONNECT
-Original file line number
+Diff line change
@@ Expand Up / @@ -18,6 +18,8 @@ INBOUND test_share test_database shared_tenant to_tenant @@
+    cannot grant a database which create from share
+    ERROR 1105 (HY000) at line 1: Code: 2718, Text = Cannot share database 'shared_db' which created from share.
     alter table add column and query table data again
 ,1
 ,1
@@ Expand Down @@