Skip to content

Added a Plugin trait #536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Oct 3, 2023
Merged

Added a Plugin trait #536

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
804fd0d
Improved logging
mdashti Aug 2, 2023
1424388
Improved logging for more `Address` usages
mdashti Aug 2, 2023
a1926fa
Fixed lint issues.
mdashti Aug 2, 2023
b9ff2be
Reverted the `Address` logging changes.
mdashti Aug 11, 2023
dcabbbb
Applied the PR comment by @levkk.
mdashti Oct 3, 2023
2cb16d2
Applied the PR comment by @levkk.
mdashti Oct 3, 2023
dee386f
Applied the PR comment by @levkk.
mdashti Oct 3, 2023
c23a5d1
Applied the PR comment by @levkk.
mdashti Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ where
error_response(
&mut write,
&format!(
"No pool configured for database: {:?}, user: {:?}",
"No pool configured for database: {:?}, user: {:?} (in startup)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is debug level messaging we should not be returning to the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Reverted this.

pool_name, username
),
)
Expand Down Expand Up @@ -1241,7 +1241,7 @@ where
let _ = query_router.infer(&ast);
}
}
debug!("Sending query to server");
debug!("Sending query to server (in Query mode)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this.


self.send_and_receive_loop(
code,
Expand Down Expand Up @@ -1354,7 +1354,7 @@ where
// Sync
// Frontend (client) is asking for the query result now.
'S' => {
debug!("Sending query to server");
debug!("Sending query to server (in Sync mode)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No such thing as "sync" mode, this is the extended protocol. Maybe make the debug line something like:

Sending query to server with extended protocol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this.


match plugin_output {
Some(PluginOutput::Deny(error)) => {
Expand Down Expand Up @@ -1499,7 +1499,7 @@ where
error_response(
&mut self.write,
&format!(
"No pool configured for database: {}, user: {}",
"No pool configured for database: {}, user: {} (in get_pool)",
self.pool_name, self.username
),
)
Expand Down
43 changes: 39 additions & 4 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,15 +763,26 @@ pub struct Plugins {
pub prewarmer: Option<Prewarmer>,
}

pub trait Plugin {
Copy link
Contributor

@levkk levkk Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have a trait, but don't think you need it for this particular fix. We should discuss what trait we should have for a Plugin to implement, so we can get on our way to a real plugin system.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second thought I see why you added it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also thinking about the plugin system. I should write down my thoughts and share it with you. Please do the same if you have such notes.

fn is_enabled(&self) -> bool;
}

impl std::fmt::Display for Plugins {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
fn is_enabled<T: Plugin>(arg: Option<&T>) -> bool {
Copy link
Contributor

@levkk levkk Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaner and safer to write it this way:

if let Some(arg) = arg {
    arg.is_enabled()
} else {
    false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a valid suggestion. Thanks. Done.

if arg.is_some() {
arg.unwrap().is_enabled()
} else {
false
}
}
write!(
f,
"interceptor: {}, table_access: {}, query_logger: {}, prewarmer: {}",
self.intercept.is_some(),
self.table_access.is_some(),
self.query_logger.is_some(),
self.prewarmer.is_some(),
is_enabled(self.intercept.as_ref()),
is_enabled(self.table_access.as_ref()),
is_enabled(self.query_logger.as_ref()),
is_enabled(self.prewarmer.as_ref()),
)
}
}
Expand All @@ -782,23 +793,47 @@ pub struct Intercept {
pub queries: BTreeMap<String, Query>,
}

impl Plugin for Intercept {
fn is_enabled(&self) -> bool {
self.enabled
}
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, Hash, Eq)]
pub struct TableAccess {
pub enabled: bool,
pub tables: Vec<String>,
}

impl Plugin for TableAccess {
fn is_enabled(&self) -> bool {
self.enabled
}
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, Hash, Eq)]
pub struct QueryLogger {
pub enabled: bool,
}

impl Plugin for QueryLogger {
fn is_enabled(&self) -> bool {
self.enabled
}
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, Hash, Eq)]
pub struct Prewarmer {
pub enabled: bool,
pub queries: Vec<String>,
}

impl Plugin for Prewarmer {
fn is_enabled(&self) -> bool {
self.enabled
}
}

impl Intercept {
pub fn substitute(&mut self, db: &str, user: &str) {
for (_, query) in self.queries.iter_mut() {
Expand Down