Conversion of attribute names to lower case causes subsequent error

[leotohill]

Description
When creating the uri for a POST, the client changes the first character of a class property name to lower case, so for example "Id" becomes "id". If the property is the primary key property, an AddDocumentsAsync() will fail if the index was created to define the primary key as "Id".

The cause would seem to be this line in ObjectExtensions.ToQueryString
var key = Uri.EscapeDataString(char.ToLowerInvariant(field.Name[0]) + field.Name.Substring(1));
I'm quite curious - why does it tolower() the first character of the field name?

Expected behavior
No error in this situation.

Current behavior
see code sample below:
AddDocumentsAsync fails with error "Document doesn't have a Id attribute: {"id":1,"name":"Item 1"}."

Screenshots or Logs
If applicable, add screenshots or logs to help explain your problem.

Environment (please complete the following information):
Windows Server 2019
dotnet 6
meilisearch-dotnet v0.14.7
meilisearch 1.5.1

public static async Task TestCaseSensitivePrimaryKey(MeilisearchClient client)
{
	var createTask = await client.CreateIndexAsync("CsTest", "Id");
	await client.WaitForTaskAsync(createTask.TaskUid);
	var index = await  client.GetIndexAsync("CsTest");
	var items = new [] {new CsTest() {Id=1, Name="Item 1"}};
	var addTask = await index.AddDocumentsAsync(items);
	var result = await client.WaitForTaskAsync(addTask.TaskUid);
	if (result.Status != TaskInfoStatus.Succeeded)
	{
		Console.WriteLine(result.Error["message"]);
	}
}

[curquiza]

Hello @leotohill
I'm not sure I exactly get the issue, maybe @ahmednfwela has a better understanding as me, but I will try to answer.

The client changes the first letter to be able to send it to Meilisearch, otherwise Meilisearch will throw an error. For instance, when updating your settings, you have so send searchableAttributes and not SearchableAttributes.

[leotohill]

The client changes the first letter to be able to send it to Meilisearch, otherwise Meilisearch will throw an error. For instance, when updating your settings, you have so send searchableAttributes and not SearchableAttributes.

This doesn't sound like a proper responsibility of the client. If some code specified "SearchableAttributes" instead of "searchableAttributes", it's a defect in that code, and it's not the responsiblity of this library to correct it.

[ahmednfwela]

@leotohill The way the client was designed enforced constant JsonSerializationOptions

see https://github.com/meilisearch/meilisearch-dotnet/blob/d4c715c01c40fe203155e842e4823723755aa821/src/Meilisearch/Constants.cs

I agree that a better API should be introduced that lets the user override the settings as they want.

However currently to "workaround" this, instead of passing in your model directly to AddDocumentsAsync, you can serialize it beforehand (using JsonSerializer.SerializeToDocument), which lets you use your own options

[leotohill]

Hmm, Ok, but why set PropertyNamingPolicy to anything? This seems wrong. The default would leave the property name unchanged.

[ahmednfwela]

@leotohill as @curquiza explained, these settings are used to serialize meili-related options as well, which all use camel case.

BUT a better option is to just rely on [JsonPropertyName("....")] attributes for meilisearch.

[leotohill]

Thanks ahmed. It still seems wrong to me, as evidenced by the error I encountered. Hidden renames of specified properties is a suprising behavior.

And as you know, in dotnet, public property names are commonly PascalCase. So I won't be the last person to encounter this problem. The workaround is easy, but every person who runs into this situation will have to research what went wrong.
:(

And even when there is no immediate error, the hidden renames will surface as issues for other clients to the data: they will likely have received doc that specifies property names in PascalCase, because nobody knew that those renames occurred.

Of course it would be a breaking change, to change this now. I don't know if that's acceptable, but since it's not yet 1.x, maybe.

[ahmednfwela]

@leotohill I will research a different non-breaking API that prevents people from falling to this problem again, thanks for feedback