[JsonPatch] Implementation of the Move Operation in accordance with the JSONPATCH RFC6902. #3262
Conversation
Added testcase block.
…m/amaanhaque/azure-cosmos-dotnet-v3 into users/t-amaanhaque/move-operation
amaanhaque
requested review from
FabianMeiswinkel,
ealsur,
j82w,
khdang,
kirankumarkolli,
kirillg,
neildsh and
sboshra
as code owners
...zure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Batch/BatchSinglePartitionKeyTests.cs
Outdated
Show resolved
Hide resolved
...zure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Batch/BatchSinglePartitionKeyTests.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Commenter does not have sufficient privileges for PR 3262 in repo Azure/azure-cosmos-dotnet-v3 |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
ealsur
left a comment
ealsur
left a comment
There was a problem hiding this comment.
Emulator tests are failing with:
Microsoft.Azure.Cosmos.CosmosException: Response status code does not indicate success: BadRequest (400); Substatus: 0; ActivityId: d48a11a6-af63-47ff-b6ea-1146e212e8a1; Reason: (Message: {"Errors":["Invalid value of the patch operation 'op' property in patch request: 'move'"]}
Does this require a new Emulator release?
It doesn't. |
| /// Source location reference (used in case of move) | ||
| /// </summary> | ||
| [JsonProperty(PropertyName = PatchConstants.PropertyNames.From)] | ||
| public abstract string From { get; } |
There was a problem hiding this comment.
This is a breaking change. Adding any new abstract member on a public non-sealed class is considered breaking. Because customers could have inherited from that class and now would hit a compiler error. Make this property virtual please with a default (I assume returning null when you follow what you did in Java)
|
|
||
| this.From = string.IsNullOrWhiteSpace(value.ToString()) | ||
| ? throw new ArgumentNullException(nameof(value)) | ||
| : value.ToString(); |
There was a problem hiding this comment.
Nothing prevents value form being null in the first place. And that would result in a NullReferenceException here. String.IsNullOrWhiteSpace(value) instead of String.IsNullOrWhiteSpace(value.ToString()) fixes that
| [EnumMember(Value = PatchConstants.OperationTypeNames.Increment)] | ||
| Increment, | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Strictly speaking adding new enum values is also breaking - I think this is acceptable here because it clearly is a contract class - but that is the reason why usually it is a bad idea in frameworks to expose enums in Json contract classes - and usage of Strings is safer.
Please add a comment on this enum - class level to clearly call out that customers should expect to values to be added - and any switch / case assuming new enums would only be added for new major (breaking) versions is discouraged.
| { | ||
| writer.WritePropertyName(PatchConstants.PropertyNames.From); | ||
| writer.WriteValue(operation.From); | ||
| } |
There was a problem hiding this comment.
shouldn't this be else if here - if it is move value would never be needed
| /// <param name="from">The source location of the object/value.</param> | ||
| /// <returns>PatchOperation instance for specified input.</returns> | ||
| public static PatchOperation Move( | ||
| string path, |
There was a problem hiding this comment.
In Java you have switched the order PatchOperation Move(string from, string path) - I personally think that is much more intuitive. And would be my preference for .Net as well.
FabianMeiswinkel
left a comment
FabianMeiswinkel
left a comment
There was a problem hiding this comment.
Mostly looks good - few comments that need to be addressed
|
Duplicate of #3389 |
3 participants
Pull Request Template
Description
The move operation removes the value at a specified location and adds it to the target location.
It takes in 2 parameters "path" (destination path) and "from" (source path).
Functionally it is identical to an add operation at the destination followed by a remove at the source path.
This operation is discussed in the JSONPATCH RFC6902.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber