ronantakizawa/codereview-bench huggingface dataset:
Repository: medusajs/medusa
File: packages/core/core-flows/src/order/workflows/cancel-order.ts (TypeScript)

Code before:

    const uncapturedPaymentIds = transform({ order }, ({ order }) => {
      const payments = deepFlatMap(
        order,
        "payment_collections.payments",
        ({ payments }) => payments
      )

      const uncapturedPayments = payments.filter(
        (payment) => payment.captures.length === 0
      )

      return uncapturedPayments.map((payment) => payment.id)
    })

    const creditLineAmount = transform({ order }, ({ order }) => {
      const payments = deepFlatMap(
        order,
        "payment_collections.payments",
        ({ payments }) => payments
      )

      return payments
        .flatMap((payment) => payment.captures)
        .reduce(
          (acc, capture) => MathBN.sum(acc, capture.amount),
          MathBN.convert(0)
        )
    })

    const lineItemIds = transform({ order }, ({ order }) => {
      return order.items?.map((i) => i.id)
    })

    parallelize(
      createOrderRefundCreditLinesWorkflow.runAsStep({
        input: {
          order_id: order.id,
          amount: creditLineAmount,
        },
      }),
      deleteReservationsByLineItemsStep(lineItemIds),
      cancelPaymentStep({ paymentIds: uncapturedPaymentIds }),
      refundCapturedPaymentsWorkflow.runAsStep({
        input: { order_id: order.id, created_by: input.canceled_by },
      }),
      emitEventStep({
        eventName: OrderWorkflowEvents.CANCELED,
        data: { id: order.id },
      })
    )

Review comment (suggestion):
```suggestion
          (acc, capture) => MathBN.sum(acc, capture.raw_amount),
```

Code after:
    cancelValidateOrder({ order, input })

    const uncapturedPaymentIds = transform({ order }, ({ order }) => {
      const payments = deepFlatMap(
        order,
        "payment_collections.payments",
        ({ payments }) => payments
      )

      const uncapturedPayments = payments.filter(
        (payment) => payment.captures.length === 0
      )

      return uncapturedPayments.map((payment) => payment.id)
    })

    const creditLineAmount = transform({ order }, ({ order }) => {
      const payments = deepFlatMap(
        order,
        "payment_collections.payments",
        ({ payments }) => payments
      )

      return payments
        .flatMap((payment) => payment.captures)
        .reduce(
          (acc, capture) => MathBN.sum(acc, capture.raw_amount),
          MathBN.convert(0)
        )
    })

    const lineItemIds = transform({ order }, ({ order }) => {
      return order.items?.map((i) => i.id)
    })

    parallelize(
      createOrderRefundCreditLinesWorkflow.runAsStep({
        input: {
          order_id: order.id,
          amount: creditLineAmount,
        },
      }),
      deleteReservationsByLineItemsStep(lineItemIds),
      cancelPaymentStep({ paymentIds: uncapturedPaymentIds }),
      refundCapturedPaymentsWorkflow.runAsStep({
        input: { order_id: order.id, created_by: input.canceled_by },
      }),
      emitEventStep({
        eventName: OrderWorkflowEvents.CANCELED,
        data: { id: order.id },
      })
Repository: medusajs/medusa
File: www/utils/packages/scripts/create-cloud-announcement.ts (TypeScript)

Code before:
  }

  if (!content) {
    console.warn(
      "No content found in release body. Using default message as fallback."
    )
  }

  console.log(`Downloading image from: ${imageUrl}`)

  // Download and convert image to base64
  const image = await downloadImageAsBase64(imageUrl)

  console.log(`Image downloaded: ${image.filename} (${image.mimeType})`)

  // Prepare announcement payload
  const payload: AnnouncementPayload = {
    title: "New Medusa Version Released",
    subtitle: subtitle || release.name || undefined,
    image,
    content: content || "Learn more about the latest Medusa release",
    type: "feature",
    link_url: release.html_url,
    published: true,
    published_at: new Date().toISOString(),
  }

  console.log("Sending announcement to API...")

  // Send to API
  await sendAnnouncement(payload)

  core.setOutput("announcement_created", true)
  core.setOutput("release_tag", release.tag_name)
}

void createCloudAnnouncement()


Review comment (suggestion):
I would probably use type `release` or type `info`, it's not a cloud `feature` and don't auto publish for now.

Code after:
  }

  if (!content) {
    console.warn(
      "No content found in release body. Using default message as fallback."
    )
  }

  console.log(`Downloading image from: ${imageUrl}`)

  // Download and convert image to base64
  const image = await downloadImageAsBase64(imageUrl)

  console.log(`Image downloaded: ${image.filename} (${image.mimeType})`)

  // Prepare announcement payload
  const payload: AnnouncementPayload = {
    title: "New Medusa Version Released",
    subtitle: subtitle || release.name || undefined,
    image,
    content: content || "Learn more about the latest Medusa release",
    type: "release",
    link_url: release.html_url,
    published: false,
    published_at: new Date().toISOString(),
  }

  console.log("Sending announcement to API...")

  // Send to API
  await sendAnnouncement(payload)

  core.setOutput("announcement_created", true)
  core.setOutput("release_tag", release.tag_name)
}

void createCloudAnnouncement()
Repository: meilisearch/meilisearch
File: meilisearch-http/tests/content_type.rs (Rust)

Code before:
mod common;

use crate::common::{GetAllDocumentsOptions, Server};
use actix_web::test;
use meilisearch_http::create_app;
use serde_json::{json, Value};

#[actix_rt::test]
async fn strict_json_bad_content_type() {
    let routes = [
        // all the POST route except the dumps that can be created without any body or content-type
        // and the search that is not a strict json
        "/indexes",
        "/indexes/doggo/documents/delete-batch",
        "/indexes/doggo/search",
        "/indexes/doggo/settings",
        "/indexes/doggo/settings/displayed-attributes",
        "/indexes/doggo/settings/distinct-attribute",
        "/indexes/doggo/settings/filterable-attributes",
        "/indexes/doggo/settings/ranking-rules",
        "/indexes/doggo/settings/searchable-attributes",
        "/indexes/doggo/settings/sortable-attributes",
        "/indexes/doggo/settings/stop-words",
        "/indexes/doggo/settings/synonyms",
    ];
    let bad_content_types = [
        "application/csv",
        "application/x-ndjson",
        "application/x-www-form-urlencoded",
        "text/plain",
        "json",
        "application",
        "json/application",
    ];

    let document = "{}";

Review comment (suggestion):
```suggestion
        // all the POST routes except the dumps that can be created without any body or content-type
```

Code after:
#![allow(dead_code)]

mod common;

use crate::common::Server;
use actix_web::test;
use meilisearch_http::create_app;
use serde_json::{json, Value};

#[actix_rt::test]
async fn strict_json_bad_content_type() {
    let routes = [
        // all the POST routes except the dumps that can be created without any body or content-type
        // and the search that is not a strict json
        "/indexes",
        "/indexes/doggo/documents/delete-batch",
        "/indexes/doggo/search",
        "/indexes/doggo/settings",
        "/indexes/doggo/settings/displayed-attributes",
        "/indexes/doggo/settings/distinct-attribute",
        "/indexes/doggo/settings/filterable-attributes",
        "/indexes/doggo/settings/ranking-rules",
        "/indexes/doggo/settings/searchable-attributes",
        "/indexes/doggo/settings/sortable-attributes",
        "/indexes/doggo/settings/stop-words",
        "/indexes/doggo/settings/synonyms",
    ];
    let bad_content_types = [
        "application/csv",
        "application/x-ndjson",
        "application/x-www-form-urlencoded",
        "text/plain",
        "json",
        "application",
        "json/application",
    ];
Repository: meilisearch/meilisearch
File: meilisearch-lib/src/index_controller/index_resolver/index_store.rs (Rust)

Code before:
                // drop the guard here so we can perform the write after without deadlocking;
                drop(guard);
                let path = self.path.join(format!("{}", uuid));
                if !path.exists() {
                    return Ok(None);
                }

                let index_size = self.index_size;
                let file_store = self.update_file_store.clone();
                let update_handler = self.update_handler.clone();
                let index = spawn_blocking(move || {
                    Index::open(path, index_size, file_store, uuid, update_handler)
                })
                .await??;
                self.index_store.write().await.insert(uuid, index.clone());
                Ok(Some(index))
            }
        }
    }

    async fn delete(&self, uuid: Uuid) -> Result<Option<Index>> {
        let db_path = self.path.join(format!("{}", uuid));
        fs::remove_dir_all(db_path).await?;
        let index = self.index_store.write().await.remove(&uuid);
        if let Some(index) = &index {
            index.inner.as_ref().clone().prepare_for_closing();
        }
        Ok(index)
    }
}


Review comment (suggestion):
[`prepare_for_closing` must be awaited](https://docs.rs/heed/0.11.0/heed/struct.EnvClosingEvent.html#method.wait) and [will only be able to close and index if there is only one instance of it](https://docs.rs/heed/0.11.0/heed/struct.Env.html#method.prepare_for_closing) so cloning this index and calling this method on the clone seems... strange.

Also:
> Make sure that you don't have any copy of the environment in the thread that is waiting for a close event, if you do, you will have a dead-lock.

Code after:
            Some(index) => Ok(Some(index.clone())),
            None => {
                // drop the guard here so we can perform the write after without deadlocking;
                drop(guard);
                let path = self.path.join(format!("{}", uuid));
                if !path.exists() {
                    return Ok(None);
                }

                let index_size = self.index_size;
                let file_store = self.update_file_store.clone();
                let update_handler = self.update_handler.clone();
                let index = spawn_blocking(move || {
                    Index::open(path, index_size, file_store, uuid, update_handler)
                })
                .await??;
                self.index_store.write().await.insert(uuid, index.clone());
                Ok(Some(index))
            }
        }
    }

    async fn delete(&self, uuid: Uuid) -> Result<Option<Index>> {
        let db_path = self.path.join(format!("{}", uuid));
        fs::remove_dir_all(db_path).await?;
        let index = self.index_store.write().await.remove(&uuid);
        Ok(index)
    }
}
Repository: meilisearch/meilisearch
File: meilisearch-http/src/task.rs (Rust)

Code before:
        Self { results }
    }
}

#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct SummarizedTaskView {
    uid: TaskId,
    index_uid: String,
    status: TaskStatus,
    #[serde(rename = "type")]
    task_type: TaskType,
    enqueued_at: DateTime<Utc>,
}

impl From<Task> for SummarizedTaskView {
    fn from(mut other: Task) -> Self {
        let created_event = other
            .events
            .drain(..1)
            .next()
            .expect("Task must have an enqueued event.");

        let enqueued_at = match created_event {
            TaskEvent::Created(ts) => ts,
            _ => unreachable!("Thsi first event of a task must always be 'Created'"),
        };

        Self {
            uid: other.id,
            index_uid: other.index_uid.to_string(),
            status: TaskStatus::Enqueued,
            task_type: other.content.into(),
            enqueued_at,
        }
    }
}


Review comment (suggestion):
```suggestion
            _ => unreachable!("The first event of a task must always be 'Created'"),
```

Code after:
        Self { results }
    }
}

#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct SummarizedTaskView {
    uid: TaskId,
    index_uid: String,
    status: TaskStatus,
    #[serde(rename = "type")]
    task_type: TaskType,
    enqueued_at: DateTime<Utc>,
}

impl From<Task> for SummarizedTaskView {
    fn from(mut other: Task) -> Self {
        let created_event = other
            .events
            .drain(..1)
            .next()
            .expect("Task must have an enqueued event.");

        let enqueued_at = match created_event {
            TaskEvent::Created(ts) => ts,
            _ => unreachable!("The first event of a task must always be 'Created'"),
        };

        Self {
            uid: other.id,
            index_uid: other.index_uid.to_string(),
            status: TaskStatus::Enqueued,
            task_type: other.content.into(),
            enqueued_at,
        }
    }
}
Repository: meilisearch/meilisearch
File: meilisearch-http/src/routes/indexes/documents.rs (Rust)

Code before:
    let update = Update::DeleteDocuments(ids);
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}

pub async fn clear_all_documents(
    meilisearch: GuardedData<Private, MeiliSearch>,
    path: web::Path<String>,
) -> Result<HttpResponse, ResponseError> {
    let update = Update::ClearDocuments;
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}

/// Returns the Content-Type header value without the charset=utf-8 or anything,
/// only the value that is before the first semicolon (;).
fn extract_content_type(headers: &HeaderMap) -> Option<String> {
    match headers.get(CONTENT_TYPE) {
        Some(value) => match value.to_str() {
            Ok(content) => Some(
                content
                    .split_once(";")
                    .map_or(content, |(ct, _)| ct.trim())
                    .to_string(),
            ),
            Err(_) => Some(format!("{}", value.as_bytes().as_bstr())), // convenient bytes display
        },
        None => None,
    }
}


Review comment (suggestion):
Is it forbidden to put the charset as the first value?
```
charset=utf-8;type=application/json
```

Code after:

    let task = meilisearch.register_update(index_uid, update).await?.into();

    debug!("returns: {:?}", task);
    Ok(task)
}

pub async fn delete_documents(
    meilisearch: GuardedData<Private, MeiliSearch>,
    path: web::Path<String>,
    body: web::Json<Vec<Value>>,
) -> Result<HttpResponse, ResponseError> {
    debug!("called with params: {:?}", body);
    let ids = body
        .iter()
        .map(|v| {
            v.as_str()
                .map(String::from)
                .unwrap_or_else(|| v.to_string())
        })
        .collect();

    let update = Update::DeleteDocuments(ids);
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}

pub async fn clear_all_documents(
    meilisearch: GuardedData<Private, MeiliSearch>,
    path: web::Path<String>,
) -> Result<HttpResponse, ResponseError> {
    let update = Update::ClearDocuments;
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}
Repository: meilisearch/meilisearch
File: meilisearch-http/src/routes/indexes/documents.rs (Rust)

Code before:
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}

/// Returns the Content-Type header value without the charset=utf-8 or anything,
/// only the value that is before the first semicolon (;).
///
/// Here is the composition of an HTTP Content-Type header
/// <https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1>.
fn extract_content_type(headers: &HeaderMap) -> Option<String> {
    match headers.get(CONTENT_TYPE) {
        Some(value) => match value.to_str() {
            Ok(content) => Some(
                content
                    .split_once(";")
                    .map_or(content, |(ct, _)| ct.trim())
                    .to_string(),
            ),
            Err(_) => Some(format!("{}", value.as_bytes().as_bstr())), // convenient bytes display
        },
        None => None,
    }
}


Review comment (suggestion):
I would prefer that we use [this method](https://docs.rs/actix-web/3.3.2/actix_web/trait.HttpMessage.html#method.mime_type) that is implemented on `HttpRequest`, and leverage the Mine library, instead of doing this ourselves

Code after:
        .map(|v| {
            v.as_str()
                .map(String::from)
                .unwrap_or_else(|| v.to_string())
        })
        .collect();

    let update = Update::DeleteDocuments(ids);
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}

pub async fn clear_all_documents(
    meilisearch: GuardedData<Private, MeiliSearch>,
    path: web::Path<String>,
) -> Result<HttpResponse, ResponseError> {
    let update = Update::ClearDocuments;
    let task: SummarizedTaskView = meilisearch
        .register_update(path.into_inner(), update)
        .await?
        .into();

    debug!("returns: {:?}", task);
    Ok(HttpResponse::Accepted().json(task))
}
Repository: meilisearch/meilisearch
File: meilisearch-lib/src/snapshot.rs (Rust)

Code before:
        )
    } else if !snapshot_path.as_ref().exists() && !ignore_missing_snapshot {
        bail!(
            "snapshot doesn't exist at {:?}",
            snapshot_path
                .as_ref()
                .canonicalize()
                .unwrap_or_else(|_| snapshot_path.as_ref().to_owned())
        )
    } else {
        Ok(())
    }
}

#[derive(Debug)]
pub struct SnapshotJob {
    dest_path: PathBuf,
    src_path: PathBuf,

    meta_env_size: usize,
    index_size: usize,
}

impl SnapshotJob {
    pub async fn run(self) -> anyhow::Result<()> {
        tokio::task::spawn_blocking(|| dbg!(self.run_sync())).await??;

        Ok(())
    }

    fn run_sync(self) -> anyhow::Result<()> {
        trace!("Performing snapshot.");

        let snapshot_dir = self.dest_path.clone();
        std::fs::create_dir_all(&snapshot_dir)?;
        let temp_snapshot_dir = tempfile::tempdir()?;
        let temp_snapshot_path = temp_snapshot_dir.path();

        self.snapshot_version_file(temp_snapshot_path)?;
        self.snapshot_meta_env(temp_snapshot_path)?;
        self.snapshot_file_store(temp_snapshot_path)?;
        self.snapshot_indexes(temp_snapshot_path)?;

        let db_name = self
            .src_path
            .file_name()
            .and_then(|n| n.to_str())
            .unwrap_or("data.ms")
            .to_string();

        let snapshot_path = self.dest_path.join(format!("{}.snapshot", db_name));

Review comment (suggestion):
```suggestion
        tokio::task::spawn_blocking(|| self.run_sync()).await??;
```

Code after:
        )
    } else if !snapshot_path.as_ref().exists() && !ignore_missing_snapshot {
        bail!(
            "snapshot doesn't exist at {:?}",
            snapshot_path
                .as_ref()
                .canonicalize()
                .unwrap_or_else(|_| snapshot_path.as_ref().to_owned())
        )
    } else {
        Ok(())
    }
}

#[derive(Debug)]
pub struct SnapshotJob {
    dest_path: PathBuf,
    src_path: PathBuf,

    meta_env_size: usize,
    index_size: usize,
}

impl SnapshotJob {
    pub async fn run(self) -> anyhow::Result<()> {
        tokio::task::spawn_blocking(|| self.run_sync()).await??;

        Ok(())
    }

    fn run_sync(self) -> anyhow::Result<()> {
        trace!("Performing snapshot.");

        let snapshot_dir = self.dest_path.clone();
        std::fs::create_dir_all(&snapshot_dir)?;
        let temp_snapshot_dir = tempfile::tempdir()?;
        let temp_snapshot_path = temp_snapshot_dir.path();

        self.snapshot_version_file(temp_snapshot_path)?;
        self.snapshot_meta_env(temp_snapshot_path)?;
        self.snapshot_file_store(temp_snapshot_path)?;
        self.snapshot_indexes(temp_snapshot_path)?;

        let db_name = self
            .src_path
            .file_name()
            .and_then(|n| n.to_str())
            .unwrap_or("data.ms")
            .to_string();

        let snapshot_path = self.dest_path.join(format!("{}.snapshot", db_name));
Repository: meilisearch/meilisearch
File: meilisearch-lib/src/lib.rs (Rust)

Code before:
pub use milli;

mod compression;
pub mod document_formats;

use walkdir::WalkDir;

pub trait EnvSizer {
    fn size(&self) -> u64;
}

impl EnvSizer for heed::Env {
    fn size(&self) -> u64 {
        WalkDir::new(self.path())
            .into_iter()
            .filter_map(|entry| entry.ok())
            .filter_map(|entry| entry.metadata().ok())
            .filter(|metadata| metadata.is_file())
            .fold(0, |acc, m| acc + m.len())
    }
}

/// Check if a db is empty. It does not provide any information on the
/// validity of the data in it.
/// We consider a database as non empty when it's a non empty directory.
pub fn empty_db(db_path: impl AsRef<Path>) -> bool {
    let db_path = db_path.as_ref();

    if !db_path.exists() {
        true
    // if we encounter an error or if the db is a file we consider the db non empty
    } else if let Ok(dir) = db_path.read_dir() {
        dir.count() == 0
    } else {
        true
    }
}


Review comment (suggestion):
```suggestion
pub fn is_empty_db(db_path: impl AsRef<Path>) -> bool {
```

Code after:
pub use milli;

mod compression;
pub mod document_formats;

use walkdir::WalkDir;

pub trait EnvSizer {
    fn size(&self) -> u64;
}

impl EnvSizer for heed::Env {
    fn size(&self) -> u64 {
        WalkDir::new(self.path())
            .into_iter()
            .filter_map(|entry| entry.ok())
            .filter_map(|entry| entry.metadata().ok())
            .filter(|metadata| metadata.is_file())
            .fold(0, |acc, m| acc + m.len())
    }
}

/// Check if a db is empty. It does not provide any information on the
/// validity of the data in it.
/// We consider a database as non empty when it's a non empty directory.
pub fn is_empty_db(db_path: impl AsRef<Path>) -> bool {
    let db_path = db_path.as_ref();

    if !db_path.exists() {
        true
    // if we encounter an error or if the db is a file we consider the db non empty
    } else if let Ok(dir) = db_path.read_dir() {
        dir.count() == 0
    } else {
        true
    }
}