Validate user messages on chat completions endpoint

[mdevino]

Closes #382.

[evaline-ju]

For this particular endpoint currently, where /content detectors are invokable on chat completions input or output, we would expect text content to be provided for the last message, with any of the roles that this endpoint will work for. The role validation is done here

fms-guardrails-orchestrator/src/orchestrator/handlers/chat_completions_detection/unary.rs

Line 121 in 6bc045f

Some(Role::User) | Some(Role::Assistant) | Some(Role::System)

on input, so I’m curious why we want to check at the deserialization separately, if the deserialization is not where the error occurs?

[mdevino]

For this particular endpoint currently, where /content detectors are invokable on chat completions input or output, we would expect text content to be provided for the last message, with any of the roles that this endpoint will work for. The role validation is done here https://github.com/evaline-ju/fms-guardrails-orchestrator/blob/6bc045f0768bc9f9688c47a2dc5f7605a01b4d96/src/orchestrator/handlers/chat_completions_detection/unary.rs#L121 on input, so I’m curious why we want to check at the deserialization separately, if the deserialization is not where the error occurs?

Thanks for catching that. I'm not super familiar with the codebase for this endpoint yet, so I wasn't sure that was the best place to put the validation.
I've updated the code to validate it where you suggested. The one thing I noticed is that, when made the change requested, it doesn't fix the issue if no input detectors are provided (it does fix it if an input detector is provided, though).

Request

{
		"messages": [
				{
					"role": "user",
					"name": "string"
				}
			],
			"model": "my_model_id",
			"n": 1,
			"temperature": 1,
			"top_p": 1,
			"user": "user-1234",
			"detectors": {}
}

Response

{
	"code": 422,
	"details": "chat completion request failed for `my_model_id`: list index out of range"
}

[declark1]

Not sure if this is finished yet, but a better way to write this would be below. It's good to use any / all iterator methods for stuff like this.

let messages = if let Some(messages) = value.get("messages") {
    Vec::<Message>::deserialize(messages)
        .map_err(|_| ValidationError::Invalid("error deserializing `messages`".into()))
} else {
    Err(ValidationError::Required("messages".into()))
}?;
if messages
    .iter()
    .any(|message| message.role == Role::User && message.content.is_none())
{
    return Err(ValidationError::Invalid(
        "`content` is required for user messages".into(),
    ));
};

Btw, this just validates that Content is provided, but the inner value Content::Text(text) could still be an empty String. Do we want to validate that it isn't empty?

[mdevino]

It's not the final code yet. I just pushed what I have before going on vacations, so someone could pick up form there if needed. I like the any() suggestion!