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
Support writing nested delta resource sets with non-delta writer. #2250
Conversation
e431407
to
60e843a
Compare
60e843a
to
00e996f
Compare
@@ -3264,8 +3269,6 @@ await thisParam.StartResourceSetAsync(resourceSetParam) | |||
private async Task VerifyCanWriteStartDeltaResourceSetAsync(bool synchronousCall, ODataDeltaResourceSet deltaResourceSet) | |||
{ | |||
ExceptionUtils.CheckArgumentNotNull(deltaResourceSet, "deltaResourceSet"); | |||
|
|||
this.VerifyWritingDelta(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why was this removed
#ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this checked to see whether the writer was created with isDelta=true.
Such a check was unnecessary, and was legacy from when we had different writers under the covers for delta versus non delta collections.
It should be valid to start writing a delta resource set regardless of how the writer was created. In fact, calling writeStartDeltaResourceSet is what sets the writingDelta to true. The only thing writingDelta should validate is whether we are currently in the context of writing a delta collection (in which case we can write a deletedResource or added/deleted link) or we are not (in which case we can't).
This is particularly important for enabling writing a nested delta set within a single resource (for example, for a patch request/response) since there is no way to specify writingDelta when creating a reader/writer for a single resource.
@@ -811,7 +810,7 @@ protected override void StartDeletedResource(ODataDeletedResource resource) | |||
{ | |||
// Writing a deleted resource within an entity set | |||
DeltaResourceSetScope deltaResourceSetScope = this.ParentScope as DeltaResourceSetScope; | |||
Debug.Assert(deltaResourceSetScope != null, "Writing child of delta set and parent scope is not DeltaResourceSetScope"); | |||
Debug.Assert(this.ParentScope.State == WriterState.Start || deltaResourceSetScope != null, "Writing child of delta set and parent scope is not DeltaResourceSetScope"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we also update the error sentence to refresh the changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the error text is still valid (and even more correct than previously). Previously the assert only checked that the parent scope was a delta resource set scope, not that what was being written was being written as the child of a delta set. The new assert validates that either the parent is scope is a deltaResourceSetScope or we're not attempting to write a child of a delta set (i.e., if the current state is start then we're writing at the top level, not as a child of a delta set.)
@@ -33,7 +33,7 @@ internal abstract class ODataWriterCore : ODataWriter, IODataOutputInStreamError | |||
private readonly bool writingResourceSet; | |||
|
|||
/// <summary>True if the writer was created for writing a delta response; false otherwise.</summary> | |||
private readonly bool writingDelta; | |||
private readonly Stack<bool> writingDelta = new Stack<bool>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cannot we put the status into the Scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably could, I was just minimizing the change for this PR, with the knowledge that this is a good candidate for clean-up in 8.0.
} | ||
|
||
var writer = messageWriter.CreateODataResourceWriter(this.GetCustomers(), this.GetCustomerType()); | ||
// ODataJsonLightWriter writer = new ODataJsonLightWriter(GetV401OutputContext(isResponse), this.GetCustomers(), this.GetCustomerType(), false, false, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or why don't use this line code?
4e73223
to
1e2446d
Compare
1e2446d
to
0eb4b9a
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
*This pull request fixes #2252 *
Description
A valid response to PATCH against a single resource with a collection is a response containing a nested delta resource set.
Today that is not possible because you can only write a delta response if you declare in your top-level resource set writer that you are writing a delta payload.
There is no longer any reason to declare up-front whether a writer is writing a delta request/response versus a non-delta request/response. A user should be able to call WriteStartDeltaResourceSet and write a delta resource set at the top level or nested within a resource.
This PR enables writing a delta resource set within a non-delta resource set writer. It retains the checks to verify that a deleted resource is only supported within a delta resource set, and an added/deleted link within a top-level delta resource set. Everything else should be common between a resourceSet and deltaResourceSet, at the top level or nested within a resource.
Checklist (Uncheck if it is not completed)
Additional work necessary
In 8.0, remove the writingDelta flag from creating resource set writer