[CSHARP-1678] Update "set" operator ignores [BsonIgnoreIfNull] attribute Created: 27/May/16 Updated: 18/Aug/23 Resolved: 03/Dec/20 |
|
| Status: | Closed |
| Project: | C# Driver |
| Component/s: | Builders, Serialization |
| Affects Version/s: | 2.2.4 |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Eugeny Shchelkanov | Assignee: | Unassigned |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
Update set operator ignores [BsonIgnoreIfNull] attribute and sets value to null. Example code:
The result JSON in MongoDB is:
Expected:
|
| Comments |
| Comment by A B [ 18/Aug/23 ] | ||||||||||||||||||||||||||
| ||||||||||||||||||||||||||
| Comment by Robert Stam [ 17/Aug/23 ] | ||||||||||||||||||||||||||
|
I was investigating your statement: `UpdateDefinition<TDocument>.Unset` doesn't accept `Expression<Func<TDocument, TField>> It sort of does. It takes an `Expression<Func<TDocument, object>>`. Sometimes our Expression arguments are written in terms of `object` instead of `TField` when the type of the field is irrelevant. If you had simply written `update.Unset(x => x.X)` it would have worked. The problem is that your variable is of type `Expression<TDocument, TField>` and not of type `Expression<TDocument, object>`. I don't think I can suggest a better workaround than what you already discovered.
| ||||||||||||||||||||||||||
| Comment by A B [ 15/Aug/23 ] | ||||||||||||||||||||||||||
|
Yep, I have since changed it to:
| ||||||||||||||||||||||||||
| Comment by A B [ 15/Aug/23 ] | ||||||||||||||||||||||||||
|
btw, | ||||||||||||||||||||||||||
| Comment by Robert Stam [ 15/Aug/23 ] | ||||||||||||||||||||||||||
|
The issue here is that `UpdateDefinitionBuilder.Set` translates to the MQL `$set` update operator, which can only be used to set fields to a new value, not to remove fields. I think your extension method won't work correctly if the document in the database already has that field with an existing (non-null) value. As currently written your extension method will ignore nulls, leaving the existing value in the database, when what you probably wanted was to remove the field from the document in the database. You can use `UpdateDefinitionBuilder.Unset` to remove fields from a document in the database. I get that you think that `UpdateDefinitionBuilder.Set` should automatically handle nulls in some way, but our thinking when declining to make this change was that it is very unexpected for `UpdateDefinitionBuilder.Set` to magically become an `UpdateDefinitionBuilder.Unset`. So that means that when using UpdateDefinitions you do have to do the null check in your application and call either `UpdateDefinitionBuilder.Set` or `UpdateDefinitionBuilder.Unset`. | ||||||||||||||||||||||||||
| Comment by A B [ 13/Aug/23 ] | ||||||||||||||||||||||||||
|
I've decided to use these extension methods for the time being
| ||||||||||||||||||||||||||
| Comment by A B [ 13/Aug/23 ] | ||||||||||||||||||||||||||
| Comment by A B [ 13/Aug/23 ] | ||||||||||||||||||||||||||
I'm voting to reopen this, I was totally expecting `UpdateDefinition.Set` to respect `[BsonIgnoreIfNull]`, now I have to write more code to filter out the `NULL` values 😢☹️ | ||||||||||||||||||||||||||
| Comment by Rasmus Sigsgaard [ 22/Feb/22 ] | ||||||||||||||||||||||||||
|
We are using IgnoreIfNullConvention and have also just been hit by properties having a value not being unset when later changed to null using UpdateOneAsync. We cannot use Replace since we are using SetOnInsert for some properties, so now we have to unset all properties which are null using reflection. Perhaps to avoid a breaking change a new convention could be introduced, e.g. IgnoreIfNullUpdateUnsetConvention. | ||||||||||||||||||||||||||
| Comment by Ignacio Jócano [ 06/Jul/21 ] | ||||||||||||||||||||||||||
|
I came across this ticket while implementing partial updates with nested objects within a repository. I ended using object.ToBsonDocument() to have these rules enforced and then iterating over each BsonElement to build the UpdateDefinition | ||||||||||||||||||||||||||
| Comment by Jeffrey Yemin [ 03/Dec/20 ] | ||||||||||||||||||||||||||
|
Closing as Won't Fix. We can re-open if more users express an interest in this change. | ||||||||||||||||||||||||||
| Comment by Craig Wilson [ 01/Jun/16 ] | ||||||||||||||||||||||||||
|
Wow. Interesting. So, if [BsonIgnoreIfNull] is set, or [BsonIgnoreIfDefault], then you think we should change a .Set(x => x.S, null) into a .Unset(x => x.S). This is a much larger behavior change than what I had thought you wanted above, which was just to ignore the operation altogether. I was having a backwards compatibility problem with the initial ask, but this is even larger. Don't get my wrong, I totally get your point, but we may have to chalk this up to a backwards compatibility thing. The number of applications whose behavior would totally change is overwhelmingly larger than the number of people who have asked about this anomaly. I'll need to confer with my colleagues, but the chances that we change this behavior is low. I'd start planning for how to workaround this issue. Craig | ||||||||||||||||||||||||||
| Comment by Eugeny Shchelkanov [ 31/May/16 ] | ||||||||||||||||||||||||||
|
I don't think that Update.Set(x => x.S, null) is more explicit than ReplaceOne(new Entity{ Id = 1, S = null }). However, in the latter example the driver does not save null and "erases" field `S` if it was previously set to something. It is what I would expect from the first example as well. For example, if the field is marked [BsonIgnoreIfNull] and the value is null the driver can change above query to:
From my perspective this is a logical and consistent behaviour. | ||||||||||||||||||||||||||
| Comment by Craig Wilson [ 27/May/16 ] | ||||||||||||||||||||||||||
|
I understand what you are saying. I'm not sure we have the infrastructure available for this to be a quick fix. I'm also not sure it's the right thing to do. In this case, you've very explicitly told us to set this value to null. If the value in the database was 12, then we would do nothing and it would continue to be 12. In the above example, there would actually be no statement executed because there isn't anything to update. This seems to me like it would be very unexpected to a vast majority of users. I'll confer with my colleagues. You are the first to bring this up and we have a great many users and I'm concerned about what changing this behavior would mean to all the others. Thoughts? |