Skip to content
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.

Sign up for GitHub

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

Fix ADT Patch APIs to return Response rather than Response<String> #14729

Merged
merged 2 commits into from
Sep 1, 2020
Merged

Fix ADT Patch APIs to return Response rather than Response<String> #14729

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce64730
Fix ADT Patch APIs to return Response rather than Response<String>
timtay-microsoft Sep 1, 2020
c5cb58b
to squash
timtay-microsoft Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix ADT Patch APIs to return Response rather than Response<String> 
updateComponent and updateDigital twin will never get an HTTP response payload by design, but our custom code mistakenly assumed they would. This fixes the customization code so that they return Response rather than Response<String>
  • Loading branch information
@timtay-microsoft
timtay-microsoft committed Sep 1, 2020
commit ce6473073092c321ae65b71a88192a6cec78b23a
8 changes: 4 additions & 4 deletions sdk/digitaltwins/Azure.DigitalTwins.Core/api/Azure.DigitalTwins.Core.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ public DigitalTwinsClient(System.Uri endpoint, Azure.Core.TokenCredential creden
public virtual System.Threading.Tasks.Task<Azure.Response> PublishTelemetryAsync(string digitalTwinId, string payload, Azure.DigitalTwins.Core.TelemetryOptions options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Pageable<string> Query(string query, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.AsyncPageable<string> QueryAsync(string query, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response<string> UpdateComponent(string digitalTwinId, string componentPath, string componentUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response<string>> UpdateComponentAsync(string digitalTwinId, string componentPath, string componentUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response<string> UpdateDigitalTwin(string digitalTwinId, string digitalTwinUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response<string>> UpdateDigitalTwinAsync(string digitalTwinId, string digitalTwinUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response UpdateComponent(string digitalTwinId, string componentPath, string componentUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response> UpdateComponentAsync(string digitalTwinId, string componentPath, string componentUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response UpdateDigitalTwin(string digitalTwinId, string digitalTwinUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response> UpdateDigitalTwinAsync(string digitalTwinId, string digitalTwinUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response UpdateRelationship(string digitalTwinId, string relationshipId, string relationshipUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response> UpdateRelationshipAsync(string digitalTwinId, string relationshipId, string relationshipUpdateOperations, Azure.DigitalTwins.Core.RequestOptions requestOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
}
Expand Down
48 changes: 15 additions & 33 deletions sdk/digitaltwins/Azure.DigitalTwins.Core/src/Customized/DigitalTwinsRestClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ internal Response<string> GetById(string id, CancellationToken cancellationToken
}
}

internal async Task<Response<string>> UpdateAsync(string id, string patchDocument, string ifMatch = null, CancellationToken cancellationToken = default)
internal async Task<Response> UpdateAsync(string id, string patchDocument, string ifMatch = null, CancellationToken cancellationToken = default)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's generated by default? Can we get rid of the customized code altogether?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the patchDocument argument to this method takes an object and tries to serialize that object. Our library deliberately customizes this layer to not serialize this particular argument and to just take the payload directly as a string. As such, we still can't remove this customization

{
if (id == null)
{
Expand All @@ -201,14 +201,8 @@ internal async Task<Response<string>> UpdateAsync(string id, string patchDocumen
switch (message.Response.Status)
{
case 202:
{
string value = default;
using JsonDocument document = await JsonDocument.ParseAsync(message.Response.ContentStream, default, cancellationToken).ConfigureAwait(false);
value = document.RootElement.GetRawText();
return Response.FromValue(value, message.Response);
}
case 204:
return Response.FromValue<string>(null, message.Response);
return message.Response;

default:
throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
Expand All @@ -221,7 +215,7 @@ internal async Task<Response<string>> UpdateAsync(string id, string patchDocumen
}
}

internal Response<string> Update(string id, string patchDocument, string ifMatch = null, CancellationToken cancellationToken = default)
internal Response Update(string id, string patchDocument, string ifMatch = null, CancellationToken cancellationToken = default)
{
if (id == null)
{
Expand All @@ -241,14 +235,8 @@ internal Response<string> Update(string id, string patchDocument, string ifMatch
switch (message.Response.Status)
{
case 202:
{
string value = default;
using var document = JsonDocument.Parse(message.Response.ContentStream);
value = document.RootElement.GetRawText();
return Response.FromValue(value, message.Response);
}
case 204:
return Response.FromValue<string>(null, message.Response);
return message.Response;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


default:
throw _clientDiagnostics.CreateRequestFailedException(message.Response);
Expand Down Expand Up @@ -549,7 +537,7 @@ internal Response<string> GetComponent(string id, string componentPath, Cancella
}
}

internal async Task<Response<string>> UpdateComponentAsync(string id, string componentPath, string patchDocument = null, string ifMatch = null, CancellationToken cancellationToken = default)
internal async Task<Response> UpdateComponentAsync(string id, string componentPath, string patchDocument = null, string ifMatch = null, CancellationToken cancellationToken = default)
{
if (id == null)
{
Expand All @@ -569,14 +557,8 @@ internal async Task<Response<string>> UpdateComponentAsync(string id, string com
switch (message.Response.Status)
{
case 202:
{
string value = default;
using JsonDocument document = await JsonDocument.ParseAsync(message.Response.ContentStream, default, cancellationToken).ConfigureAwait(false);
value = document.RootElement.GetRawText();
return Response.FromValue(value, message.Response);
}
case 204:
return Response.FromValue<string>(null, message.Response);
return message.Response;

default:
throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
Expand All @@ -589,7 +571,7 @@ internal async Task<Response<string>> UpdateComponentAsync(string id, string com
}
}

internal Response<string> UpdateComponent(string id, string componentPath, string patchDocument = null, string ifMatch = null, CancellationToken cancellationToken = default)
internal Response UpdateComponent(string id, string componentPath, string patchDocument = null, string ifMatch = null, CancellationToken cancellationToken = default)
{
if (id == null)
{
Expand All @@ -609,14 +591,8 @@ internal Response<string> UpdateComponent(string id, string componentPath, strin
switch (message.Response.Status)
{
case 202:
{
string value = default;
using var document = JsonDocument.Parse(message.Response.ContentStream);
value = document.RootElement.GetRawText();
return Response.FromValue(value, message.Response);
}
case 204:
return Response.FromValue<string>(null, message.Response);
return message.Response;

default:
throw _clientDiagnostics.CreateRequestFailedException(message.Response);
Expand Down Expand Up @@ -654,6 +630,7 @@ internal async Task<Response> SendTelemetryAsync(string id, string dtId, string
{
case 204:
return message.Response;

default:
throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
}
Expand Down Expand Up @@ -690,6 +667,7 @@ internal Response SendTelemetry(string id, string dtId, string telemetry, string
{
case 204:
return message.Response;

default:
throw _clientDiagnostics.CreateRequestFailedException(message.Response);
}
Expand Down Expand Up @@ -730,6 +708,7 @@ internal async Task<Response> SendComponentTelemetryAsync(string id, string comp
{
case 204:
return message.Response;

default:
throw await _clientDiagnostics.CreateRequestFailedExceptionAsync(message.Response).ConfigureAwait(false);
}
Expand Down Expand Up @@ -770,6 +749,7 @@ internal Response SendComponentTelemetry(string id, string componentPath, string
{
case 204:
return message.Response;

default:
throw _clientDiagnostics.CreateRequestFailedException(message.Response);
}
Expand Down Expand Up @@ -931,6 +911,7 @@ private HttpMessage CreateSendComponentTelemetryRequest(string id, string compon
}

#region null overrides

// The following methods are only declared so that autorest does not create these functions in the generated code.
// For methods that we need to override, when the parameter list is the same, autorest knows not to generate them again.
// When the parameter list changes, autorest generates the methods again.
Expand Down Expand Up @@ -977,6 +958,7 @@ private HttpMessage CreateSendComponentTelemetryRequest(string id, string compon
private HttpMessage CreateAddRequest(string id, object twin) => null;

#pragma warning restore CA1801, IDE0051, IDE0060 // Remove unused parameter
#endregion

#endregion null overrides
}
}
8 changes: 4 additions & 4 deletions sdk/digitaltwins/Azure.DigitalTwins.Core/src/DigitalTwinsClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ public virtual Response DeleteDigitalTwin(string digitalTwinId, RequestOptions r
/// <exception cref="ArgumentNullException">
/// The exception is thrown when <paramref name="digitalTwinId"/> or <paramref name="digitalTwinUpdateOperations"/> is <c>null</c>.
/// </exception>
public virtual Task<Response<string>> UpdateDigitalTwinAsync(string digitalTwinId, string digitalTwinUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
public virtual Task<Response> UpdateDigitalTwinAsync(string digitalTwinId, string digitalTwinUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough our method comments didn't mention this string response!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely because we couldn't figure out what it was, haha

{
return _dtRestClient.UpdateAsync(digitalTwinId, digitalTwinUpdateOperations, requestOptions?.IfMatch, cancellationToken);
}
Expand All @@ -325,7 +325,7 @@ public virtual Task<Response<string>> UpdateDigitalTwinAsync(string digitalTwinI
/// <seealso cref="UpdateDigitalTwinAsync(string, string, RequestOptions, CancellationToken)">
/// See the asynchronous version of this method for examples.
/// </seealso>
public virtual Response<string> UpdateDigitalTwin(string digitalTwinId, string digitalTwinUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
public virtual Response UpdateDigitalTwin(string digitalTwinId, string digitalTwinUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
{
return _dtRestClient.Update(digitalTwinId, digitalTwinUpdateOperations, requestOptions?.IfMatch, cancellationToken);
}
Expand Down Expand Up @@ -410,7 +410,7 @@ public virtual Response<string> GetComponent(string digitalTwinId, string compon
/// Console.WriteLine($&quot;Updated component for digital twin &apos;{basicDtId}&apos;.&quot;);
/// </code>
/// </example>
public virtual Task<Response<string>> UpdateComponentAsync(string digitalTwinId, string componentPath, string componentUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
public virtual Task<Response> UpdateComponentAsync(string digitalTwinId, string componentPath, string componentUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
{
// TODO how can we make this patch easier to construct?
return _dtRestClient.UpdateComponentAsync(digitalTwinId, componentPath, componentUpdateOperations, requestOptions?.IfMatch, cancellationToken);
Expand All @@ -437,7 +437,7 @@ public virtual Task<Response<string>> UpdateComponentAsync(string digitalTwinId,
/// <seealso cref="UpdateComponentAsync(string, string, string, RequestOptions, CancellationToken)">
/// See the asynchronous version of this method for examples.
/// </seealso>
public virtual Response<string> UpdateComponent(string digitalTwinId, string componentPath, string componentUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
public virtual Response UpdateComponent(string digitalTwinId, string componentPath, string componentUpdateOperations, RequestOptions requestOptions = default, CancellationToken cancellationToken = default)
{
return _dtRestClient.UpdateComponent(digitalTwinId, componentPath, componentUpdateOperations, requestOptions?.IfMatch, cancellationToken);
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/digitaltwins/Azure.DigitalTwins.Core/tests/ComponentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,15 @@ public async Task Component_Lifecycle()
getComponentResponse.GetRawResponse().Status.Should().Be((int)HttpStatusCode.OK);

// Patch component
Response<string> updateComponentResponse = await client
Response updateComponentResponse = await client
.UpdateComponentAsync(
roomWithWifiTwinId,
wifiComponentName,
TestAssetsHelper.GetWifiComponentUpdatePayload())
.ConfigureAwait(false);

// The response to the Patch request should be 204 (No content)
updateComponentResponse.GetRawResponse().Status.Should().Be((int)HttpStatusCode.NoContent);
updateComponentResponse.Status.Should().Be((int)HttpStatusCode.NoContent);
}
finally
{
Expand Down