Repository: inspektor-dev/inspektor (language: rust)

Issue:
table names are prefixed while using wild card select
Table names are prefixed if any column is considered as protected column.

eg: 
```
select * from actors -> select actor.ssn as NULL, actor.first_name from actors
```

This may retrieve the results. But it's breaking orm clients. 

Expected Behaviour: 

Table name should not be prefixed if not required. 


Discussion / hints:

Fix patch:
diff --git a/src/apiproto/api_grpc.rs b/src/apiproto/api_grpc.rs
index dcd9743..2835a97 100644
--- a/src/apiproto/api_grpc.rs
+++ b/src/apiproto/api_grpc.rs
@@ -4,7 +4,6 @@
 // https://github.com/Manishearth/rust-clippy/issues/702
 #![allow(unknown_lints)]
 #![allow(clippy::all)]
-
 #![allow(box_pointers)]
 #![allow(dead_code)]
 #![allow(missing_docs)]
@@ -16,25 +15,48 @@
 #![allow(unused_imports)]
 #![allow(unused_results)]
 
-const METHOD_INSPEKTOR_AUTH: ::grpcio::Method<super::api::AuthRequest, super::api::AuthResponse> = ::grpcio::Method {
-    ty: ::grpcio::MethodType::Unary,
-    name: "/api.Inspektor/Auth",
-    req_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
-    resp_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
-};
-
-const METHOD_INSPEKTOR_POLICY: ::grpcio::Method<super::api::Empty, super::api::InspektorPolicy> = ::grpcio::Method {
-    ty: ::grpcio::MethodType::ServerStreaming,
-    name: "/api.Inspektor/Policy",
-    req_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
-    resp_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
-};
-
-const METHOD_INSPEKTOR_GET_DATA_SOURCE: ::grpcio::Method<super::api::Empty, super::api::DataSourceResponse> = ::grpcio::Method {
+const METHOD_INSPEKTOR_AUTH: ::grpcio::Method<super::api::AuthRequest, super::api::AuthResponse> =
+    ::grpcio::Method {
+        ty: ::grpcio::MethodType::Unary,
+        name: "/api.Inspektor/Auth",
+        req_mar: ::grpcio::Marshaller {
+            ser: ::grpcio::pb_ser,
+            de: ::grpcio::pb_de,
+        },
+        resp_mar: ::grpcio::Marshaller {
+            ser: ::grpcio::pb_ser,
+            de: ::grpcio::pb_de,
+        },
+    };
+
+const METHOD_INSPEKTOR_POLICY: ::grpcio::Method<super::api::Empty, super::api::InspektorPolicy> =
+    ::grpcio::Method {
+        ty: ::grpcio::MethodType::ServerStreaming,
+        name: "/api.Inspektor/Policy",
+        req_mar: ::grpcio::Marshaller {
+            ser: ::grpcio::pb_ser,
+            de: ::grpcio::pb_de,
+        },
+        resp_mar: ::grpcio::Marshaller {
+            ser: ::grpcio::pb_ser,
+            de: ::grpcio::pb_de,
+        },
+    };
+
+const METHOD_INSPEKTOR_GET_DATA_SOURCE: ::grpcio::Method<
+    super::api::Empty,
+    super::api::DataSourceResponse,
+> = ::grpcio::Method {
     ty: ::grpcio::MethodType::Unary,
     name: "/api.Inspektor/GetDataSource",
-    req_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
-    resp_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
+    req_mar: ::grpcio::Marshaller {
+        ser: ::grpcio::pb_ser,
+        de: ::grpcio::pb_de,
+    },
+    resp_mar: ::grpcio::Marshaller {
+        ser: ::grpcio::pb_ser,
+        de: ::grpcio::pb_de,
+    },
 };
 
 #[derive(Clone)]
@@ -49,54 +71,111 @@ impl InspektorClient {
         }
     }
 
-    pub fn auth_opt(&self, req: &super::api::AuthRequest, opt: ::grpcio::CallOption) -> ::grpcio::Result<super::api::AuthResponse> {
+    pub fn auth_opt(
+        &self,
+        req: &super::api::AuthRequest,
+        opt: ::grpcio::CallOption,
+    ) -> ::grpcio::Result<super::api::AuthResponse> {
         self.client.unary_call(&METHOD_INSPEKTOR_AUTH, req, opt)
     }
 
-    pub fn auth(&self, req: &super::api::AuthRequest) -> ::grpcio::Result<super::api::AuthResponse> {
+    pub fn auth(
+        &self,
+        req: &super::api::AuthRequest,
+    ) -> ::grpcio::Result<super::api::AuthResponse> {
         self.auth_opt(req, ::grpcio::CallOption::default())
     }
 
-    pub fn auth_async_opt(&self, req: &super::api::AuthRequest, opt: ::grpcio::CallOption) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::AuthResponse>> {
-        self.client.unary_call_async(&METHOD_INSPEKTOR_AUTH, req, opt)
+    pub fn auth_async_opt(
+        &self,
+        req: &super::api::AuthRequest,
+        opt: ::grpcio::CallOption,
+    ) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::AuthResponse>> {
+        self.client
+            .unary_call_async(&METHOD_INSPEKTOR_AUTH, req, opt)
     }
 
-    pub fn auth_async(&self, req: &super::api::AuthRequest) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::AuthResponse>> {
+    pub fn auth_async(
+        &self,
+        req: &super::api::AuthRequest,
+    ) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::AuthResponse>> {
         self.auth_async_opt(req, ::grpcio::CallOption::default())
     }
 
-    pub fn policy_opt(&self, req: &super::api::Empty, opt: ::grpcio::CallOption) -> ::grpcio::Result<::grpcio::ClientSStreamReceiver<super::api::InspektorPolicy>> {
-        self.client.server_streaming(&METHOD_INSPEKTOR_POLICY, req, opt)
+    pub fn policy_opt(
+        &self,
+        req: &super::api::Empty,
+        opt: ::grpcio::CallOption,
+    ) -> ::grpcio::Result<::grpcio::ClientSStreamReceiver<super::api::InspektorPolicy>> {
+        self.client
+            .server_streaming(&METHOD_INSPEKTOR_POLICY, req, opt)
     }
 
-    pub fn policy(&self, req: &super::api::Empty) -> ::grpcio::Result<::grpcio::ClientSStreamReceiver<super::api::InspektorPolicy>> {
+    pub fn policy(
+        &self,
+        req: &super::api::Empty,
+    ) -> ::grpcio::Result<::grpcio::ClientSStreamReceiver<super::api::InspektorPolicy>> {
         self.policy_opt(req, ::grpcio::CallOption::default())
     }
 
-    pub fn get_data_source_opt(&self, req: &super::api::Empty, opt: ::grpcio::CallOption) -> ::grpcio::Result<super::api::DataSourceResponse> {
-        self.client.unary_call(&METHOD_INSPEKTOR_GET_DATA_SOURCE, req, opt)
+    pub fn get_data_source_opt(
+        &self,
+        req: &super::api::Empty,
+        opt: ::grpcio::CallOption,
+    ) -> ::grpcio::Result<super::api::DataSourceResponse> {
+        self.client
+            .unary_call(&METHOD_INSPEKTOR_GET_DATA_SOURCE, req, opt)
     }
 
-    pub fn get_data_source(&self, req: &super::api::Empty) -> ::grpcio::Result<super::api::DataSourceResponse> {
+    pub fn get_data_source(
+        &self,
+        req: &super::api::Empty,
+    ) -> ::grpcio::Result<super::api::DataSourceResponse> {
         self.get_data_source_opt(req, ::grpcio::CallOption::default())
     }
 
-    pub fn get_data_source_async_opt(&self, req: &super::api::Empty, opt: ::grpcio::CallOption) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::DataSourceResponse>> {
-        self.client.unary_call_async(&METHOD_INSPEKTOR_GET_DATA_SOURCE, req, opt)
+    pub fn get_data_source_async_opt(
+        &self,
+        req: &super::api::Empty,
+        opt: ::grpcio::CallOption,
+    ) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::DataSourceResponse>> {
+        self.client
+            .unary_call_async(&METHOD_INSPEKTOR_GET_DATA_SOURCE, req, opt)
     }
 
-    pub fn get_data_source_async(&self, req: &super::api::Empty) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::DataSourceResponse>> {
+    pub fn get_data_source_async(
+        &self,
+        req: &super::api::Empty,
+    ) -> ::grpcio::Result<::grpcio::ClientUnaryReceiver<super::api::DataSourceResponse>> {
         self.get_data_source_async_opt(req, ::grpcio::CallOption::default())
     }
-    pub fn spawn<F>(&self, f: F) where F: ::futures::Future<Output = ()> + Send + 'static {
+    pub fn spawn<F>(&self, f: F)
+    where
+        F: ::futures::Future<Output = ()> + Send + 'static,
+    {
         self.client.spawn(f)
     }
 }
 
 pub trait Inspektor {
-    fn auth(&mut self, ctx: ::grpcio::RpcContext, req: super::api::AuthRequest, sink: ::grpcio::UnarySink<super::api::AuthResponse>);
-    fn policy(&mut self, ctx: ::grpcio::RpcContext, req: super::api::Empty, sink: ::grpcio::ServerStreamingSink<super::api::InspektorPolicy>);
-    fn get_data_source(&mut self, ctx: ::grpcio::RpcContext, req: super::api::Empty, sink: ::grpcio::UnarySink<super::api::DataSourceResponse>);
+    fn auth(
+        &mut self,
+        ctx: ::grpcio::RpcContext,
+        req: super::api::AuthRequest,
+        sink: ::grpcio::UnarySink<super::api::AuthResponse>,
+    );
+    fn policy(
+        &mut self,
+        ctx: ::grpcio::RpcContext,
+        req: super::api::Empty,
+        sink: ::grpcio::ServerStreamingSink<super::api::InspektorPolicy>,
+    );
+    fn get_data_source(
+        &mut self,
+        ctx: ::grpcio::RpcContext,
+        req: super::api::Empty,
+        sink: ::grpcio::UnarySink<super::api::DataSourceResponse>,
+    );
 }
 
 pub fn create_inspektor<S: Inspektor + Send + Clone + 'static>(s: S) -> ::grpcio::Service {
@@ -106,12 +185,14 @@ pub fn create_inspektor<S: Inspektor + Send + Clone + 'static>(s: S) -> ::grpcio
         instance.auth(ctx, req, resp)
     });
     let mut instance = s.clone();
-    builder = builder.add_server_streaming_handler(&METHOD_INSPEKTOR_POLICY, move |ctx, req, resp| {
-        instance.policy(ctx, req, resp)
-    });
+    builder = builder
+        .add_server_streaming_handler(&METHOD_INSPEKTOR_POLICY, move |ctx, req, resp| {
+            instance.policy(ctx, req, resp)
+        });
     let mut instance = s;
-    builder = builder.add_unary_handler(&METHOD_INSPEKTOR_GET_DATA_SOURCE, move |ctx, req, resp| {
-        instance.get_data_source(ctx, req, resp)
-    });
+    builder = builder
+        .add_unary_handler(&METHOD_INSPEKTOR_GET_DATA_SOURCE, move |ctx, req, resp| {
+            instance.get_data_source(ctx, req, resp)
+        });
     builder.build()
 }
diff --git a/src/postgres_driver/message.rs b/src/postgres_driver/message.rs
index 6c47852..eca51f6 100644
--- a/src/postgres_driver/message.rs
+++ b/src/postgres_driver/message.rs
@@ -85,7 +85,7 @@ impl BackendMessage {
                 .unwrap();
                 buf
             }
-            BackendMessage::ReadyForQuery{state} => {
+            BackendMessage::ReadyForQuery { state } => {
                 buf.put_u8(b'Z');
                 buf.put_u32(5);
                 buf.put_u8(state.get_state_byte());
diff --git a/src/sql/ctx.rs b/src/sql/ctx.rs
index aa7bc1a..86de929 100644
--- a/src/sql/ctx.rs
+++ b/src/sql/ctx.rs
@@ -67,7 +67,7 @@ impl Ctx {
         let mut froms = self.from.clone().into_iter().collect::<Vec<String>>();
         froms.sort();
         for from in froms {
-            let exprs = self.column_expr_for_table(&from);
+            let exprs = self.column_expr_for_table(&from, false);
             if exprs.len() == 0 {
                 selections.push(SelectItem::QualifiedWildcard(ObjectName(vec![Ident::new(
                     from,
@@ -84,26 +84,47 @@ impl Ctx {
     }
 
     // column_expr_for_table returns accepted columne expression for the given table.
-    pub fn column_expr_for_table(&self, table_name: &String) -> Vec<SelectItem> {
+    pub fn column_expr_for_table(
+        &self,
+        table_name: &String,
+        prefix_table_name: bool,
+    ) -> Vec<SelectItem> {
+        // should_prefix will determine whether we should prefix
+        // table name as column name.
+        let mut should_prefix = prefix_table_name;
+        let splits = table_name.split(".").collect::<Vec<&str>>();
+        if splits.len() > 1 {
+            should_prefix = true;
+        }
         let mut selections = vec![];
         if let Some(protected_columns) = self.protected_columns.get(table_name) {
             let protected_columns_set = protected_columns.iter().collect::<HashSet<&String>>();
             let table_columns = self.table_info.get(table_name).unwrap();
             for col in table_columns {
                 if protected_columns_set.contains(col) || protected_columns.len() == 0 {
+                    let column_name = match should_prefix {
+                        true => format!("{}.{}", table_name, col),
+                        false => format!("{}", col),
+                    };
                     selections.push(SelectItem::ExprWithAlias {
                         expr: Expr::Value(Value::Null),
                         alias: Ident {
-                            value: format!("{}.{}", table_name, col),
+                            value: column_name,
                             quote_style: Some('"'),
                         },
                     });
                     continue;
                 }
-                selections.push(SelectItem::UnnamedExpr(Expr::CompoundIdentifier(vec![
-                    Ident::new(table_name.to_string()),
-                    Ident::new(col.to_string()),
-                ])));
+                if should_prefix {
+                    selections.push(SelectItem::UnnamedExpr(Expr::CompoundIdentifier(vec![
+                        Ident::new(table_name.to_string()),
+                        Ident::new(col.to_string()),
+                    ])));
+                    continue;
+                }
+                selections.push(SelectItem::UnnamedExpr(Expr::Identifier(Ident::new(
+                    col.to_string(),
+                ))));
             }
         }
         return selections;
diff --git a/src/sql/query_rewriter.rs b/src/sql/query_rewriter.rs
index d44c8c0..3bab40b 100644
--- a/src/sql/query_rewriter.rs
+++ b/src/sql/query_rewriter.rs
@@ -402,7 +402,7 @@ impl<T: RuleEngine + Clone> QueryRewriter<T> {
             SelectItem::QualifiedWildcard(object_name) => {
                 // first ident must be table.
                 let table_name = &object_name.0[0].value;
-                let selections = state.column_expr_for_table(table_name);
+                let selections = state.column_expr_for_table(table_name, true);
                 if selections.len() != 0 {
                     return Ok(selections);
                 }
@@ -728,7 +728,7 @@ mod tests {
             &rewriter,
             state.clone(),
             "select * from kids",
-            "SELECT NULL AS \"kids.phone\", kids.id, kids.name, kids.address FROM kids",
+            "SELECT NULL AS \"phone\", id, name, address FROM kids",
         );
 
         assert_rewriter(
@@ -791,7 +791,7 @@ mod tests {
             state,
             "WITH DUMMY AS (SELECT * FROM kids LIMIT 1)
             SELECT * FROM DUMMY",
-            "WITH DUMMY AS (SELECT NULL AS \"kids.phone\", kids.id, kids.name, kids.address FROM kids LIMIT 1) SELECT * FROM DUMMY",
+            "WITH DUMMY AS (SELECT NULL AS \"phone\", id, name, address FROM kids LIMIT 1) SELECT * FROM DUMMY",
         );
     }
 
@@ -828,7 +828,7 @@ mod tests {
             &rewriter,
             state,
             "select * from (with dummy as (select * from kids) select * from dummy)as nested limit 1;",
-            "SELECT * FROM (WITH dummy AS (SELECT NULL AS \"kids.phone\", kids.id, kids.name, kids.address FROM kids) SELECT * FROM dummy) AS nested LIMIT 1",
+            "SELECT * FROM (WITH dummy AS (SELECT NULL AS \"phone\", id, name, address FROM kids) SELECT * FROM dummy) AS nested LIMIT 1",
         );
     }
 
@@ -869,7 +869,7 @@ mod tests {
             &rewriter,
             state.clone(),
             "select * from kids UNION select * from public.kids2",
-            "SELECT NULL AS \"kids.phone\", kids.id, kids.name, kids.address FROM kids UNION SELECT * FROM public.kids2",
+            "SELECT NULL AS \"phone\", id, name, address FROM kids UNION SELECT * FROM public.kids2",
         );
 
         assert_rewriter(
@@ -943,7 +943,7 @@ mod tests {
             &rewriter,
             state.clone(),
             "select * from transactions join kids on transactions.kid_id = kids.id limit 10;",
-            r#"SELECT NULL AS "kids.phone", kids.id, kids.name, kids.address, transactions.* FROM transactions JOIN kids ON transactions.kid_id = kids.id LIMIT 10"#,
+            r#"SELECT NULL AS "phone", id, name, address, transactions.* FROM transactions JOIN kids ON transactions.kid_id = kids.id LIMIT 10"#,
         );
     }
 
@@ -1325,7 +1325,7 @@ mod tests {
             &rewriter,
             state.clone(),
             "select * from kids",
-            "SELECT NULL AS \"kids.phone\", NULL AS \"kids.id\", NULL AS \"kids.name\", NULL AS \"kids.address\" FROM kids",
+            "SELECT NULL AS \"phone\", NULL AS \"id\", NULL AS \"name\", NULL AS \"address\" FROM kids",
         );
 
         let rewriter = QueryRewriter::new(rule_engine, vec!["public".to_string()]);
@@ -1333,7 +1333,7 @@ mod tests {
             &rewriter,
             state,
             "select * from kids join transactions on transactions.kid_id = kids.id",
-            "SELECT NULL AS \"kids.phone\", NULL AS \"kids.id\", NULL AS \"kids.name\", NULL AS \"kids.address\", transactions.* FROM kids JOIN transactions ON transactions.kid_id = kids.id",
+            "SELECT NULL AS \"phone\", NULL AS \"id\", NULL AS \"name\", NULL AS \"address\", transactions.* FROM kids JOIN transactions ON transactions.kid_id = kids.id",
         );
     }
 }
diff --git a/src/sql/rule_engine.rs b/src/sql/rule_engine.rs
index 6fc27ae..e385cd7 100644
--- a/src/sql/rule_engine.rs
+++ b/src/sql/rule_engine.rs
@@ -37,7 +37,7 @@ pub struct HardRuleEngine {
     pub view_allowed: bool,
     pub copy_allowed_attributes: HashMap<String, Vec<String>>,
     pub insert_allowed_attributes: HashMap<String, Vec<String>>,
-    pub update_allowed_attributes: HashMap<String, Vec<String>>
+    pub update_allowed_attributes: HashMap<String, Vec<String>>,
 }
 
 impl RuleEngine for HardRuleEngine {
@@ -101,7 +101,7 @@ impl RuleEngine for HardRuleEngine {
     fn get_allowed_update_attributes(&self) -> &HashMap<String, Vec<String>> {
         &self.update_allowed_attributes
     }
-    
+
     fn is_update_allowed(&self) -> bool {
         self.update_allowed
     }
@@ -123,7 +123,7 @@ impl HardRuleEngine {
             view_allowed: false,
             copy_allowed_attributes: HashMap::default(),
             update_allowed_attributes: HashMap::default(),
-            insert_allowed_attributes: HashMap::default()
+            insert_allowed_attributes: HashMap::default(),
         }
     }
 }


Test patch:
diff --git a/systest/main_test.go b/systest/main_test.go
index 6bbf070..7548b39 100644
--- a/systest/main_test.go
+++ b/systest/main_test.go
@@ -108,6 +108,30 @@ func TestPostgresSelect(t *testing.T) {
 	}
 }
 
+func TestPostgresSelectWildCard(t *testing.T) {
+	actor := struct {
+		ActorID    int            `db:"actor_id"`
+		FirstName  sql.NullString `db:"first_name"`
+		LastName   sql.NullString `db:"last_name"`
+		LastUpdate *time.Time     `db:"last_update"`
+	}{}
+	db := getDB("postgres", "admin", t)
+	rows, err := db.Queryx("SELECT * FROM actor limit 1")
+	if err != nil {
+		t.Fatal(err)
+	}
+	for rows.Next() {
+		err := rows.StructScan(&actor)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert(actor.ActorID != 0, "expected actor id but got zero", t)
+		assert(actor.FirstName.String == "", "expected first name to be empty", t)
+		assert(actor.LastName.String != "", "expected last_name but got empty string", t)
+		assert(actor.LastUpdate != nil, "expected last_update but got nil", t)
+	}
+}
+
 // divine-butterfly
 // f3535f770dadb1