-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Search SDK: Cannot merge null values using Index<T>/IndexAsync<T> #1804
Comments
One workaround would be to use |
@brjohnstmsft I think using a Document will do the same. |
It should work; we have test coverage for the |
@brjohnstmsft My apology, it is working yes. I thought Document was using a dynamic, but it is a Dictionary so it's treated a little different. Here is some code for people who wants to convert their
|
…odel This is in response to the following issue: Azure#1804 The new test shows how to set NullValueHandling on a per-property basis to allow for merging of null values.
I did some testing and found a way to get the right merge semantics, at least on a per-field basis. The trick is to specify We looked at more comprehensive ways of fixing this (e.g. -- introducing some type of |
@brjohnstmsft Thanks for the update. This is exactly how I wanted to treat my null values, but back when I opened the bug, your serializer was somehow overriding my settings... maybe this changed, or I wasn't aware of this contract serializer. |
@jsgoupil Nothing changed in the implementation, I just found a way to override |
@brjohnstmsft I found it finally, I'm not familiar with your code structure on github (it seems that most of the code is generated), but I found this by decompiling the DLL. // Microsoft.Azure.Search.DocumentsOperations JsonSerializerSettings settings = JsonUtility.CreateDocumentSerializerSettings(this.Client.SerializationSettings); And this line overrides the NullValueHandler. |
Yes, the behavior of
Point 2 is especially important. That's why it's better to explicitly opt in to |
@brjohnstmsft I am not sure I see the scenario... Let me explain this scenario. Imagine you have this saved in Azure Search
If I load a bunch of A from the search, do some manipulation on the properties, including setting some to NULL, my intent is definitely to propagate the correct data to the service. In your current case, you are saying I should have this heavy JsonProperty on every single nullable properties. I would never set a property to NULL because I don't want it to be updated on the server or improve performance. It makes no sense. Now the scenario I think you are trying to cover is if I do a "Select" in the Search Parameter. In this case, if I request Z and Y, obviously I don't have X and V so them being sent with NULL would be incorrect. However, if I'm about to request only Z and Y, I should have another class so I don't carry null properties around in my app. So I would have
In this case, once again, if I change Z to be null, and Y to be another integer, my intent is pretty clear what I'm trying to do. This is why I'm proposing that if I set this NullValueHandling.Include globally, I am well aware of what I am doing. I can handle your points 1 and 2 by having my prime classes. I don't think the SDK should forbid me from doing this. This null value handling is such a big deal for your SDK, you should not rely solely on JSON.net for deciding of this behavior. I propose that you should have your own setting for controlling such thing. All properties on Azure Search are Nullable. Which means if I decide to have all my properties Nullable in my concrete class, it means I expect that I can set these values to NULL. I can't code a class with Nullable that I give to my developer and expect them to know that they are NOT ALLOWED to set it to NULL, because NULL actually means "performance". If my intent is to NOT have nullable properties, then I'll have a normal "int". Then if I want to control what is being sent to Azure, I will have prime classes like above. Sure you can keep NullValueHandling.Ignore being the default, but I should be able to override it. The SDK is suppose to help not hinder. |
@jsgoupil Your analysis is ignoring one important dimension: The problem with // I want to change the values of Z and X, but leave Y and V unchanged.
doc.Z = "new value";
doc.X = 123;
client.Documents.Index(IndexBatch.Merge(new[] { doc }));
// If we use NullValueHandling.Include, then Y and V get clobbered and data is lost. To recap, if you want to set all the properties, use I agree that these options are not perfect, and that some way to override the Other than this thread, we haven't had any customers report this as an issue. Right now we're focused on shipping a new preview version of the SDK that will include new features; It's unlikely we're going to add a global way to override Likewise, if more customers let us know that this is a real pain point, we'll re-open this. |
@brjohnstmsft I appreciate your time. I will start using In the example that you wrote above. This is performing only "faster" if you actually loaded the doc without V and X. It is a bit confusing, but I see that all scenarios seem to be supported. Thanks again 👍 |
After more investigation, I found that the client serializer is overriding my NullValueHandling variable by setting it to Ignore.
This is highly problematic as if I try to do "IndexAsync" with null values, they simply do not go to the index! This is for the case or MergeOrUpload.
Basically, an upload with property set to null not being sent is fine. But the merge is not. I need to send the null!
The text was updated successfully, but these errors were encountered: