Repository: meilisearch/meilisearch
File: crates/meilisearch/src/routes/indexes/documents.rs (Rust)

Code before:
    index: &Index,
    offset: usize,
    limit: usize,
    ids: Option<Vec<ExternalDocumentId>>,
    filter: Option<Value>,
    attributes_to_retrieve: Option<Vec<S>>,
    retrieve_vectors: RetrieveVectors,
    features: RoFeatures,
) -> Result<(u64, Vec<Document>), ResponseError> {
    let rtxn = index.read_txn()?;
    let filter = &filter;
    let filter = if let Some(filter) = filter {
        parse_filter(filter, Code::InvalidDocumentFilter, features)?
    } else {
        None
    };

    let mut candidates = if let Some(ids) = ids {
        let external_document_ids = index.external_documents_ids();
        let mut candidates = RoaringBitmap::new();
        for (index, id) in ids.iter().enumerate() {
            let Some(docid) = external_document_ids.get(&rtxn, id)? else {
                let error = MeilisearchHttpError::DocumentNotFound(id.clone().into_inner());
                let msg = format!("In `.ids[{index}]`: {error}");
                return Err(ResponseError::from_msg(msg, Code::NotFoundDocumentId));
            };
            candidates.insert(docid);
        }
        candidates
    } else {
        index.documents_ids(&rtxn)?
    };

    if let Some(filter) = filter {
        candidates &= filter.evaluate(&rtxn, index).map_err(|err| match err {
            milli::Error::UserError(milli::UserError::InvalidFilter(_)) => {
                ResponseError::from_msg(err.to_string(), Code::InvalidDocumentFilter)
            }
            e => e.into(),
        })?
    }

    let (it, number_of_documents) = {
        let number_of_documents = candidates.len();
        (
            some_documents(
                index,
                &rtxn,
                candidates.into_iter().skip(offset).take(limit),
                retrieve_vectors,
            )?,

Review comment (suggestion):
I should have reviewed the PRD earlier, but isn’t it strange to return an error when an ID is missing here?
It feels a bit counterintuitive, since the filters don’t work like that.
And a bit like a foot gun considering we index documents asynchronously and may know the list of ids we want to search on before they’re fully indexed 🤔 

But anyway, it’s in the PRD, if you think that’s the right thing to do let’s move on

Code after:
    index: &Index,
    offset: usize,
    limit: usize,
    ids: Option<Vec<ExternalDocumentId>>,
    filter: Option<Value>,
    attributes_to_retrieve: Option<Vec<S>>,
    retrieve_vectors: RetrieveVectors,
    features: RoFeatures,
) -> Result<(u64, Vec<Document>), ResponseError> {
    let rtxn = index.read_txn()?;
    let filter = &filter;
    let filter = if let Some(filter) = filter {
        parse_filter(filter, Code::InvalidDocumentFilter, features)?
    } else {
        None
    };

    let mut candidates = if let Some(ids) = ids {
        let external_document_ids = index.external_documents_ids();
        let mut candidates = RoaringBitmap::new();
        for id in ids.iter() {
            let Some(docid) = external_document_ids.get(&rtxn, id)? else {
                continue;
            };
            candidates.insert(docid);
        }
        candidates
    } else {
        index.documents_ids(&rtxn)?
    };

    if let Some(filter) = filter {
        candidates &= filter.evaluate(&rtxn, index).map_err(|err| match err {
            milli::Error::UserError(milli::UserError::InvalidFilter(_)) => {
                ResponseError::from_msg(err.to_string(), Code::InvalidDocumentFilter)
            }
            e => e.into(),
        })?
    }

    let (it, number_of_documents) = {
        let number_of_documents = candidates.len();
        (
            some_documents(
                index,
                &rtxn,
                candidates.into_iter().skip(offset).take(limit),
                retrieve_vectors,
            )?,
            number_of_documents,
        )