meilisearch-rust icon indicating copy to clipboard operation
meilisearch-rust copied to clipboard

Remove Options on task details fields

Open bidoubiwa opened this issue 3 years ago • 3 comments

Depending on which async action you are triggering, the Details fields of a Task can return different information. See spec. These field are always returned and are not Optional.

Thus, we should remove the Option. Example:

https://github.com/meilisearch/meilisearch-rust/blob/46d38e61ddf29347b87d59a77e0c69906fb32dbc/src/tasks.rs#L57-L67

Should be changed to

#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct IndexCreation {
    pub primary_key: String,
}

#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct IndexUpdate {
    pub primary_key: String,
}

bidoubiwa avatar Nov 16 '22 11:11 bidoubiwa

These field are always returned and are not Optional.

According to the doc of indexCreation and indexUpdate, primaryKey can be null in some situations:

primaryKey: Value for the primaryKey field into the POST index payload. null if no primaryKey has been specified at the time of the index creation.

I wonder if Option<String> was chosen instead of String for such situations?

choznerol avatar May 21 '23 01:05 choznerol

Hey @choznerol

Definitely Option<string> should be used in this specific situation. My example was not the best 😅

Some others are impacted, for example, based on the specs these one: https://github.com/meilisearch/meilisearch-rust/blob/fc1d41dbb1c03683cf49a44fdb7c9738b8b37d30/src/tasks.rs#L64-L69

and probably others that should be checked.

bidoubiwa avatar May 22 '23 09:05 bidoubiwa

That’s not an absolute source of truth, but this code might help in meilisearch; it’s how we represent the details on disk. https://github.com/meilisearch/meilisearch/blob/918ce1dd674f99a41545c50272507a275ca53baf/meilisearch-types/src/tasks.rs#L499-L511

irevoire avatar May 23 '23 08:05 irevoire

Hello,

I had a look at the code and I don't believe this issue is still relevant today. From what I could see, the only place in the code where the Option<> could be removed is TaskType however since TaskType is used for both successedtasks and failedtasks it means there's always a possibility for a Null return so the Option is necessary.

Let me know if I misunderstood anything. Cheers!

NoodleSamaChan avatar Apr 17 '24 09:04 NoodleSamaChan

That’s true; we would need to duplicate the entire TaskType object or the Succeeded task type into the FailedTask to make something work, and it wouldn’t improve the experience much. I believe that parts of the issue were fixed in the past, and only the Detail thing remains but isn’t applicable.

Closing, thanks for investigating it @NoodleSamaChan

irevoire avatar Apr 17 '24 09:04 irevoire