-
Notifications
You must be signed in to change notification settings - Fork 50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the code looks good, but I think the DB query requires changes.
@@ -535,6 +535,14 @@ impl Database { | |||
Ok(()) | |||
} | |||
|
|||
pub async fn get_acl_whitelist(&self) -> Result<Vec<entity::PublicKey>> { | |||
let dbkey_set: Vec<entity::PublicKey> = sqlx::query("SELECT 1 FROM acl_whitelist") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this? This query returns literal 1
for each row that exists in the DB. I'd expect this to yield error when trying to parse as public key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I copy and paste from another query. I thought the 1 was interpreted as a prepared statement, the first return field. I didn't test the read in fact because I start only one time.
I correct and to a read test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've to add a test too, when the public key parse fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've corrected the query and added the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Please apply the typo fix for the variable name 😉
Co-authored-by: Tuomas Mäkinen <[email protected]>
Done I merge |
I let the code in the network module. Not sure where to put it.
I define the merge function alone so that I can test it without the db and network.
I put some rules about the merge: