Docusign api integration

code review
1

Hey all, this is my first "real" project written in rust. In concept, it's meant to accept a batch of data and send a bunch of api calls to docusign. There's also a webhook implementation that listens for updates from docusign. I wanted to make a user interface and some form of authentication for this, but I haven't worked up the motivation to do so quite yet. Any feedback is welcome, though I'm least happy with server/src/db.rs as there is a lot of nesting, and it feels quite verbose. Maybe it's a side affect of using rusqlite instead of a more high level database library like sqlx? Anyway, here's the code.

2

I don't think that's the case. The problem is that you are just doing a lot of nested pattern matching where it might be more prudent to split the inner bodies into their own functions or even flatten the nesting with Result/Option combinator methods like flat_map()/and_then(). Using a different crate is not going to make structural nesting any prettier by itself. Your code is structured as a tree, when it could be structured as a sequence [1]. Otherwise, I agree that this file could use some TLC.

But that isn't the only place where you do a lot of nested pattern matching. Consider docu_integration/server/src/batch_processor.rs at master · abdavis/docu_integration (github.com) I would rewrite this as:

    let await_time = resp
        .headers()
        .get("X-RateLimit-Reset")
        .and_then(|time| time.to_str().ok())
        .and_then(|time| time.parse::<u64>().ok())
        .map(|time| {
            Instant::now()
                + Duration::from_secs(
                    time - SystemTime::now()
                        .duration_since(UNIX_EPOCH)
                        .expect("System Clock Error")
                        .as_secs(),
                )
        });

Although, the way you are handling timestamps is questionable. Using chrono to handle the Unix timestamps, this could be:

    let await_time = resp
        .headers()
        .get("X-RateLimit-Reset")
        .and_then(|time| time.to_str().ok())
        .and_then(|time| time.parse::<i64>().ok())
        .and_then(|time| DateTime::from_timestamp(time, 0))
        .and_then(|time| time.signed_duration_since(Utc::now()).to_std().ok())
        .map(|dur| Instant::now() + dur);

In general, you're creating more rightward drift with nested pattern matching than you really need. This is especially apparent where you are discarding the None and Err(_) arms.

There are also cases like docu_integration/server/src/create_server/login.rs at master · abdavis/docu_integration (github.com) which use the long way to unwrap:

match some_result {
    Ok(stuff) => do_something(stuff),
    _ => panic!(),
}

Use the shorter but equivalent [2]:

do_something(some_result.unwrap());

You'll find that this breaks out of the nesting habit and produces nicer sequential code. It also gives you more debug information when the unwrap does panic.

I think rustfmt gave up on some of these because the lines are just too long (and most of it is whitespace).

Your server might be useful as a library, but it's currently only structured as an executable. One of the problems with this layout is that you can't do doctests in executables. Most projects put library stuff that can be integration tested (and doc tested) into a library for this reason. Then main.rs is just a very thin wrapper that imports from the library. You almost have this, except your server's main.rs has modules. If you wanted to address this, you would move the modules to lib.rs and have main.rs import from the library like this:

use docu_integration::{Config, batch_processor, ...};

#[tokio::main]
async fn main() {
	let config: Config = ...
}

Overall, there are some tests, but very little documentation.

Everything is public. This is not bad per se, but it's something to be mindful of when designing an API. How much of this surface area is an internal implementation detail? How many ways can callers accidentally break invariants when they have mutable access to the entire state?

You also have some obviously-not-production-ready modules under server/create_server that confused me. I nearly commented on a lot of "don't do's" before deciding to check if this code was even used in the project. The modules are commented out!

You have a Cargo.lock in every subdirectory with a Cargo.toml. Consider using a workspace instead.

There are probably many other suggestions, but these stood out to me.

  1. I'm oversimplifying this statement. It's often the case that flattening a tree is easier said than done. ↩︎

  2. This is only available if the error type in some_result implements Debug... But if your errors do not implement Debug, you have much bigger problems! ↩︎

4 Likes
3

The choice of using rusqlite is perfectly valid, but it can lead to verbose code when dealing with low-level database operations. If you find that dealing with SQL directly in your code is becoming unwieldy, you might consider using a higher-level database library like diesel or sqlx. These libraries can help you write more concise and type-safe database queries. Additionally, when working with company info API, having a more streamlined and efficient database interaction becomes crucial, and these libraries can greatly simplify the process.