refactor: don't set readonly by default (#377)

Author: abonander

src/lib.rs

@@ -605,7 +605,6 @@ mod settings {
     pub(crate) const DEFAULT_FORMAT: &str = "default_format";
     pub(crate) const COMPRESS: &str = "compress";
     pub(crate) const DECOMPRESS: &str = "decompress";
-    pub(crate) const READONLY: &str = "readonly";
     pub(crate) const ROLE: &str = "role";
     pub(crate) const QUERY: &str = "query";
 }

src/query.rs

@@ -59,7 +59,7 @@ impl Query {
 
     /// Executes the query.
     pub async fn execute(self) -> Result<()> {
-        self.do_execute(false, None)?.finish().await
+        self.do_execute(None)?.finish().await
     }
 
     /// Executes the query, returning a [`RowCursor`] to obtain results.
@@ -93,7 +93,7 @@ impl Query {
             formats::ROW_BINARY
         };
 
-        let response = self.do_execute(true, Some(format))?;
+        let response = self.do_execute(Some(format))?;
         Ok(RowCursor::new(response, validation))
     }
 
@@ -144,15 +144,11 @@ impl Query {
     ///
     /// [provided format]: https://clickhouse.com/docs/en/interfaces/formats
     pub fn fetch_bytes(self, format: impl AsRef<str>) -> Result<BytesCursor> {
-        let response = self.do_execute(true, Some(format.as_ref()))?;
+        let response = self.do_execute(Some(format.as_ref()))?;
         Ok(BytesCursor::new(response))
     }
 
-    pub(crate) fn do_execute(
-        self,
-        readonly: bool,
-        default_format: Option<&str>,
-    ) -> Result<Response> {
+    pub(crate) fn do_execute(self, default_format: Option<&str>) -> Result<Response> {
         let query = self.sql.finish()?;
 
         let mut url =
@@ -168,17 +164,6 @@ impl Query {
             pairs.append_pair(settings::DATABASE, database);
         }
 
-        // Normally, we enforce `readonly` for all `fetch_*` operations.
-        // However, we still allow overriding it to support several niche use-cases,
-        // e.g., temporary tables usage. See https://github.com/ClickHouse/clickhouse-rs/issues/230
-        if readonly {
-            let readonly_value = match self.client.options.get(settings::READONLY) {
-                None => "1",
-                Some(value) => value,
-            };
-            pairs.append_pair(settings::READONLY, readonly_value);
-        }
-
         if self.client.compression.is_lz4() {
             pairs.append_pair(settings::COMPRESS, "1");
         }

tests/it/query_readonly.rs

@@ -40,120 +40,130 @@ async fn readonly_user() {
 async fn test_fetch(client: &Client) {
     let query = select_readonly_setting_query(client);
     let initial_readonly_row = run_fetch(query).await;
+
+    // As of 0.14.2, we won't be implicitly setting `readonly` anymore.
     assert_eq!(
-        initial_readonly_row.value, "1",
-        "initial `fetch` readonly setting value should be 1"
+        initial_readonly_row.value, "0",
+        "initial `fetch` readonly setting value should be 0"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "0");
+    let query = select_readonly_setting_query(client).with_option("readonly", "1");
     let disabled_readonly_row = run_fetch(query).await;
     assert_eq!(
-        disabled_readonly_row.value, "0",
-        "`fetch` modified readonly setting value should be 0"
+        disabled_readonly_row.value, "1",
+        "`fetch` modified readonly setting value should be 1"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "1");
+    let query = select_readonly_setting_query(client).with_option("readonly", "0");
     let same_readonly_row = run_fetch(query).await;
     assert_eq!(
-        same_readonly_row.value, "1",
+        same_readonly_row.value, "0",
         "`fetch` should allow setting the same readonly setting value"
     );
 }
 
 async fn test_fetch_bytes(client: &Client) {
     let query = select_readonly_setting_query(client);
     let initial_readonly_value = run_fetch_bytes(query).await;
+
+    // As of 0.14.2, we won't be implicitly setting `readonly` anymore.
     assert_eq!(
-        initial_readonly_value, b"1\n",
-        "initial `fetch_bytes` readonly setting value should be 1"
+        initial_readonly_value, b"0\n",
+        "initial `fetch_bytes` readonly setting value should be 0"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "0");
+    let query = select_readonly_setting_query(client).with_option("readonly", "1");
     let disabled_readonly_value = run_fetch_bytes(query).await;
     assert_eq!(
-        disabled_readonly_value, b"0\n",
-        "`fetch_bytes` modified readonly setting value should be 0"
+        disabled_readonly_value, b"1\n",
+        "`fetch_bytes` modified readonly setting value should be 1"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "1");
+    let query = select_readonly_setting_query(client).with_option("readonly", "0");
     let same_readonly_value = run_fetch_bytes(query).await;
     assert_eq!(
-        same_readonly_value, b"1\n",
+        same_readonly_value, b"0\n",
         "`fetch_bytes` should allow setting the same readonly setting value"
     );
 }
 
 async fn test_fetch_one(client: &Client) {
     let query = select_readonly_setting_query(client);
     let initial_readonly_value: String = run_fetch_one(query).await;
+
+    // As of 0.14.2, we won't be implicitly setting `readonly` anymore.
     assert_eq!(
-        initial_readonly_value, "1",
-        "initial `fetch_one` readonly setting value should be 1"
+        initial_readonly_value, "0",
+        "initial `fetch_one` readonly setting value should be 0"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "0");
+    let query = select_readonly_setting_query(client).with_option("readonly", "1");
     let disabled_readonly_value: String = run_fetch_one(query).await;
     assert_eq!(
-        disabled_readonly_value, "0",
-        "`fetch_one` modified readonly setting value should be 0"
+        disabled_readonly_value, "1",
+        "`fetch_one` modified readonly setting value should be 1"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "1");
+    let query = select_readonly_setting_query(client).with_option("readonly", "0");
     let same_readonly_value: String = run_fetch_one(query).await;
     assert_eq!(
-        same_readonly_value, "1",
+        same_readonly_value, "0",
         "`fetch_one` should allow setting the same readonly setting value"
     );
 }
 
 async fn test_fetch_optional(client: &Client) {
     let query = select_readonly_setting_query(client);
     let initial_readonly_value: Option<String> = run_fetch_optional(query).await;
+
+    // As of 0.14.2, we won't be implicitly setting `readonly` anymore.
     assert_eq!(
         initial_readonly_value.as_deref(),
-        Some("1"),
-        "initial `fetch_optional` readonly setting value should be 1"
+        Some("0"),
+        "initial `fetch_optional` readonly setting value should be 0"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "0");
+    let query = select_readonly_setting_query(client).with_option("readonly", "1");
     let disabled_readonly_value: Option<String> = run_fetch_optional(query).await;
     assert_eq!(
         disabled_readonly_value.as_deref(),
-        Some("0"),
-        "`fetch_optional` modified readonly setting value should be 0"
+        Some("1"),
+        "`fetch_optional` modified readonly setting value should be 1"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "1");
+    let query = select_readonly_setting_query(client).with_option("readonly", "0");
     let same_readonly_value: Option<String> = run_fetch_optional(query).await;
     assert_eq!(
         same_readonly_value.as_deref(),
-        Some("1"),
+        Some("0"),
         "`fetch_optional` should allow setting the same readonly setting value"
     );
 }
 
 async fn test_fetch_all(client: &Client) {
     let query = select_readonly_setting_query(client);
     let initial_readonly_value: Vec<String> = run_fetch_all(query).await;
+
+    // As of 0.14.2, we won't be implicitly setting `readonly` anymore.
     assert_eq!(
         initial_readonly_value,
-        vec!["1"],
-        "initial `fetch_all` readonly setting value should be 1"
+        vec!["0"],
+        "initial `fetch_all` readonly setting value should be 0"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "0");
+    let query = select_readonly_setting_query(client).with_option("readonly", "1");
     let disabled_readonly_value: Vec<String> = run_fetch_all(query).await;
     assert_eq!(
         disabled_readonly_value,
-        vec!["0"],
-        "`fetch_all` modified readonly setting value should be 0"
+        vec!["1"],
+        "`fetch_all` modified readonly setting value should be 1"
     );
 
-    let query = select_readonly_setting_query(client).with_option("readonly", "1");
+    let query = select_readonly_setting_query(client).with_option("readonly", "0");
     let same_readonly_value: Vec<String> = run_fetch_all(query).await;
     assert_eq!(
         same_readonly_value,
-        vec!["1"],
+        vec!["0"],
         "`fetch_all` should allow setting the same readonly setting value"
     );
 }