Isolate databases/queries from one-another

[marcua]

Resolves #41

[marcua]

Remove

[marcua]

Done

[sofiaritz]

I like how you ended up implementing this, a clean and simple implementation :)
Now that the architecture of ayb is getting a little bit more complex, maybe it's time to create something like an ARCHITECTURE.md file.

[sofiaritz]

If the builder idea I left in the previous comment is not done, you could add some comments here to explain what is the meaning of each limit :)

[marcua]

Perfect! Done!

[sofiaritz]

Is piping stdin required?

[marcua]

Good eye! :) I was able to remove it with no effect.

[sofiaritz]

Suggested change

	if result.stderr.len() > 0 {
	if !result.stderr.is_empty() {

[marcua]

Nice! Thank you!

(I noticed cargo clippy also caught this, and realized clippy isn't breaking the build, so I created #250)

[sofiaritz]

Suggested change

	} else if result.stdout.len() > 0 {
	} else if !result.stdout.is_empty() {

[marcua]

Done! Thank you!

[sofiaritz]

You could remove a level of nesting by doing the following at the top

if isolation.is_none() {
   return Ok(query_sqlite(path, query)?.into());
}

[marcua]

I'm conflicted on this one. We'd benefit from one lower level of nesting in a relatively short function, but in exchange

• We would stop using a language feature like match, which I feel is being used appropriately
• We'd have to introduce an unwrap, since the compiler won't be able to reason about the knowledge we'd gain with the if

Given we're using match in the way it was intended and forcing ourselves to enumerate all of the variants, I wonder if we'd gain much in exchange for reducing indentation in a short function.

[sofiaritz]

New idea! What about using if let Some(isolation) = isolation and returning the None case at the end of the function? Seems like a reasonable middleground :)

Note: Deleted a previous comment I made because this suggestion replaces it. The two key-points from that comment:

• Enums like Option or Result are stable and thus won't add any new variants.
• When ayb handles a request and panics the server keeps running, thus it's not as bad as it may look like :) (not saying that we should just panic :p)

pub async fn potentially_isolated_sqlite_query(
    path: &PathBuf,
    query: &str,
    isolation: &Option<AybConfigIsolation>,
) -> Result<QueryResult, AybError> {
    if let Some(isolation) = isolation {
        let result = run_in_sandbox(Path::new(&isolation.nsjail_path), path, query).await?;
        if !result.stderr.is_empty() {
            let error: AybError = serde_json::from_str(&result.stderr)?;
            return Err(error);
        }
        
        if result.status != 0 {
            return Err(AybError::Other {
                message: format!(
                    "Error status from sandboxed query runner: {}",
                    result.status
                ),
            });
        }
        
        if !result.stdout.is_empty() {
            let query_result: QueryResult = serde_json::from_str(&result.stdout)?;
            return Ok(query_result);
        }
        
        return Err(AybError::Other {
            message: "No results from sandboxed query runner".to_string(),
        });
    }
    
    Ok(query_sqlite(path, query)?)
}

[marcua]

Clean! I like it! I appreciated both of your bullet points on the broader point. I'll implement this and we should be good to go! Woohoo! Thank you!!!

[marcua]

Thank you for the very helpful review @sofiaritz! You made the implementation an documentation far more approachable, and I appreciate your feedback. Looking forward to your next round! :)

Re: ARCHITECTURE.md, great idea. I added basic documentation on isolation and testing, but also created #252 for a deeper pass.

[sofiaritz]

Done! Just a few comments about early returns and match. Really excited for this to land! :)

[sofiaritz]

Amazing work!! When this lands, I'll update aybDocker and start hosting a public instance :)

[marcua]

Thank you for the very helpful review, @sofiaritz! I'll merge now, and after #237 lands, I'll release the next version for your public instance! Woohooo! :)