Code Review Comments Rust

Code Review Comments/Preferences for mSupply Rust Code

Run Clippy

Installing and running clippy can help identify a lot of common rust style issues, see https://github.com/rust-lang/rust-clippy

Unfortunately, a lot of our code doesn't follow clippy style guides right now, so it can be hard to identify where your code represented amongst other errors.

The list of clippy lints can be found here: https://rust-lang.github.io/rust-clippy/master/index.html

Prefer .to_string()

When working with strings in rust, there's a number of ways to convert a str value into a Owned String.

Given this code: let hello = "Hello World";

We prefer using

let hello = hello.to_string();

This is fast to type, is easy to read, and logically the easiest to follow for less experienced rust developers.

Avoid alternatives such as let hello = hello.to_owned(); or let hello = String::from(hello);

Soft Delete

Any record that is deletable and used in sync must be soft deleted. This means that instead of entirely removing the record from the database, it is marked as deleted. This Delete status of the record can be synced across to any related sites. This allows a record to essentially be removed for new data entry, but available for reference.

• When soft deleting a new record type, we recommend creating a nullable database field deleted_datetime. This allows for recording the status of deleted, but also tells us when it happened which can be useful if tracing back an issue related to the record.

HOWEVER if we are migrating a record from legacy mSupply and it uses another field type such as is_active or is_enabled we should keep this naming to avoid confusion across the databases.

• If a record is soft deleted, the repository delete method should be named mark_deleted to signal that it is using a soft delete
• Repository query methods should filter out all deleted records by default
• Id based requests such as find_one_by_id should still succeed even if the record is deleted, as it's often used to look up associated records that might be needed for viewing in an historical context (e.g. Invoices should still show the associated supplier and receiver even if one of them is now deleted).

Record modified_datetime and created_datetime in database records

When investigating an issue, it is often helpful to see when a record was first created and when it was last modified. For most records that can be updated (e.g. master_data records, user accounts, items, etc) modified_datetime and created_datetime should be recorded in the database.

Record user_id in database records

When investigating an issue, it is often helpful to see who last modified a record.

Don't use unwrap() unless in tests

When you make a call to unwrap() if an error was to occur in the code the whole server could crash. In tests this is ok, but in production code it should almost never be used. If you have a use case where unwrap does make sense (say if you can't connect to the database on server start up) this useage must include a comment and justification. See Also: https://levelup.gitconnected.com/rust-never-use-unwrap-in-production-c123b311f620

Avoid indirection with From implementation

Avoid using From implementation, unless it's for a common type like RepositoryError.

Generally in Rust, From implementation is used to translate between types, then translations are done with .into() or ? (for errors), this created a bit of indirection which is harder to trace (navigation to implementation or definition in IDE brings you to trait implementation). Although it's possible to navigate to actual implementation of the trait (by examining implementations of a type), it's much easier to navigate to a standard method for a type, like to_domain. And when re-mapping errors, it's more clearly done inline.

// Preferred:

let result = validate_parameter(&parameter).map_err(|e| match e {
     ValidateParameterResult::One(extra) => ValidateResult::One(extra),
     ValidateParameterResult::Two => ValidateResult::Two,
     ValidateParameterResult::Three => ValidateResult::Three,
})?;

// vs To be avoided:

let result = validate_parameter(&parameter)?; // Can't navigate to `?`

// ..

impl From<ValidateParameterResult> for ValidateResult {
    fn from(result: ValidateParameterResult) -> ValidateResult {
        match e {
            ValidateParameterResult::One(extra) => ValidateResult::One(extra),
            ValidateParameterResult::Two => ValidateResult::Two,
            ValidateParameterResult::Three => ValidateResult::Three,
        }
    }
}


// Preferred:

let result = get_record().map(RecordNode::to_domain)?;

// ..

impl RecordNode {
    fn to_domain(row: RecordRow) -> RecordNode {
        //  mapper
    }
}

// vs To be avoided:

let result = get_record().map(RecordNode::from)?;

// or

let result = get_record().map(|r| r.into())?;

// ..

impl From<RecordRow> for RecordNode {
    fn from(row: RecordRow) -> RecordNode {
        //  mapper
    }
}