-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Initial GA work for digital twins #16039
Conversation
GA service swagger comes with new optional parameters for all service requests. Thankfully the swagger also conveniently groups them together with existing options so that we can directly use the generated options objects in our CL APIs (except for a few cases)
@@ -40,4 +40,5 @@ java: | |||
context-client-method-parameter: true | |||
custom-types-subpackage: models | |||
required-fields-as-ctor-args: true | |||
custom-types: DigitalTwinModelsAddOptions,DigitalTwinModelsDeleteOptions,DigitalTwinModelsGetByIdOptions,DigitalTwinModelsUpdateOptions,DigitalTwinsAddOptions,DigitalTwinsAddRelationshipOptions,DigitalTwinsDeleteOptions,DigitalTwinsDeleteRelationshipOptions,DigitalTwinsGetByIdOptions,DigitalTwinsGetComponentOptions,DigitalTwinsGetRelationshipByIdOptions,DigitalTwinsListIncomingRelationshipsOptions,DigitalTwinsListRelationshipsOptions,DigitalTwinsUpdateComponentOptions,DigitalTwinsUpdateOptions,DigitalTwinsUpdateRelationshipOptions,EventRoutesAddOptions,EventRoutesDeleteOptions,EventRoutesGetByIdOptions,EventRoutesListOptions,QueryTwinsOptions |
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.
By declaring these as custom types, they are moved to the appropriate namespace so that we can use them in our convenience layer APIs. Note that some options classes aren't here on purpose since we have to create wrapper classes for some of them instead in order to add more options to them
*/ | ||
public final class EventRouteListOptionsConverter { | ||
|
||
/** |
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 no longer need a wrapper class for EventRouteListOptions, so the the converter from wrapper to generated is no longer needed
* @param filter the filter value to set. | ||
* @return the EventRoute object itself. | ||
*/ | ||
public EventRoute setFilter(String filter) { |
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.
This field became mandatory in the latest swagger, so it is taken in at constructor time only
|
||
package com.azure.digitaltwins.core.models; | ||
|
||
import com.azure.digitaltwins.core.DigitalTwinsClient; |
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 get this type generated for free, so no need for this manually defined type anymore
/** | ||
* Optional settings that are specific to calls to {@link DigitalTwinsClient#updateComponent(String, String, List)} and its overloads. | ||
*/ | ||
public final class DeleteRelationshipRequestOptions extends DigitalTwinsRequestOptions { |
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 get this type generated for free, so no need for this manually defined type anymore
@@ -115,7 +113,7 @@ public DigitalTwinsServiceVersion getServiceVersion() { | |||
@ServiceMethod(returns = ReturnType.SINGLE) | |||
public <T> Mono<T> createDigitalTwin(String digitalTwinId, T digitalTwin, Class<T> clazz) | |||
{ | |||
return createDigitalTwinWithResponse(digitalTwinId, digitalTwin, clazz) | |||
return createDigitalTwinWithResponse(digitalTwinId, digitalTwin, clazz, null) |
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.
so we are defaulting options to null? is there a scenario where we would want these options to have a default value instead?
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.
Since they are optional, it is always allowed. It saves us from instantiating new options classes on each call. I didn't see any particular cases where it made more sense to create a default options object instead.
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.
If "null" is the expected value, then yes, I like leaving it as is. I was thinking of the scenario where we have some request Id in options, which we would want to have a default value, rather than null. But if that is not relevant for our API calls, then what we have is good.
@@ -244,14 +244,14 @@ public DigitalTwinsServiceVersion getServiceVersion() { | |||
* {@codesnippet com.azure.digitaltwins.core.asyncClient.updateDigitalTwin#String-List} | |||
* | |||
* @param digitalTwinId The Id of the digital twin. | |||
* @param digitalTwinUpdateOperations The JSON patch to apply to the specified digital twin. | |||
* @param jsonPatch The JSON patch to apply to the specified digital twin. |
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.
q - do we have the update operations util from azure-core available for us to use here?
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.
It is still in azure core's experimental package, so not yet. Hopefully they will have it out before we GA though
I've seen other SDKs in this repo that keep all the preview release changelogs mixed in with the non-preview changelogs, so we'll do the same
payload, | ||
publishTelemetryRequestOptions.getTimestamp().toString(), | ||
options.getTimestamp().toString(), |
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.
q - what is the default value of OffsetDateTime
?
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.
this feels like another parameter that we should send a default value (current timestamp), if the user doesn't supply a value
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.
OffsetDateTime.now(ZoneOffset.UTC);
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.
It's possible that we could make the default value null, and just not send that header though
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.
Oh, I actually prefer sending the current time as default value. Why do you feel sending null is better?
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 prefer sending the timestamp header with the default value by default, not omitting the header. The current implementation sends the timestamp by default. The only value I'd see in not sending the timestamp by default is to save on bytes over the wire
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.
Sure, but for this case, I'd say that the timestamp information is valuable. My vote is to send the current timestamp, if user doesn't provide it.
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.
Then I'll keep the implementation as is. If the user passes in a null options object, we will send the current timestamp for them
* Request * DTDLParserError - The models provided are not valid DTDL. * InvalidArgument - The model id is invalid. | ||
* * LimitExceeded - The maximum number of model ids allowed in 'dependenciesFor' has been reached. * | ||
* ModelVersionNotSupported - The version of DTDL used is not supported. * 409 Conflict * ModelAlreadyExists - The | ||
* model provided already exists. |
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.
Do we call out these error types in our CL? They might be helpful for customers, since they get a list of expected errors. Might be worth adding a link to any documentation that the service team maintains.
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 don't explicitly call these out in our CL, which does feel odd considering the swagger has that information. I'm not sure what the appropriate way to call this out would be. I'd rather not copy the comments from the PL into the CL since they would get out of date quickly
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.
Yeah, maintaining copies is definitely cumbersome. We could ask the service team to upload this info to some MS doc, which we could link from our CL. (if they haven't already). They might not want to maintain this solely in a swagger doc anyway.
This reverts commit cb117c7.
Dev sql microsoft.sql 2021 05 01 preview release (Azure#16289) * add base for Microsoft.sql * Update Readme.md * update version in swagger example files * remove ss * update swagger files * update readme.md * add missing json files * update custom-words.txt to address the spelling check * remove 201 define in ServerUpdate.json * Sync sql 2021 05 01 with main branch (Azure#16236) * Update comment.yml for publish pr (Azure#16166) * Update comment.yml * Update comment.yml * Bump dependencies (Azure#16170) * bump avocado * bump mocha * remove submodule (Azure#16171) * add azure-resource-manager-schema to servicelinker (Azure#16176) Co-authored-by: Nan Jiang <[email protected]> * Update pr assignment config (Azure#16175) * [Synapse] - fix scheme for Artifacts and update readme to include KQL scripts for October release (Azure#16035) * [Synapse] - fix scheme for Notebook, Spark Job definition and SQL script * Update dataset, linked service and pipeline * Update release tag for october release * Correct type of result limit * Add arguments for SynapseSparkJobActivityTypeProperties Co-authored-by: Dongwei Wang <[email protected]> * fix (Azure#16164) Merging the change to have accurate Swagger * Remove requirement of identity field in the request for TURN. (Azure#15966) * Remove requirement of identity field in the request. * Update PR * Address comment * change api version in readme for sdk release (Azure#16187) * Add blockchain to latest profile * Add additional types * api version change Co-authored-by: Mark Cowlishaw <[email protected]> Co-authored-by: Ping Zhu <[email protected]> * [Search] Rename types for better SDK code (Azure#16039) * Rename types for better SDK code * Rename Speller and Captions search options * Reorder parameters to have consistency across APIs * Remove 'SearchIndexer' prefix from projection selectors Remove 'global' prefix from default flags on custom entities * Change enum name AdlsGen2->AzureDataLakeStorageGen2 Change property name storageContainer->storageContainerName * move additional .NET SDK renames to the REST spec * Revert changes in KnowledgeStore which has GAed * Revert changes in CustomEntityLookupSkill which has GAed * Revert changes in SIKSBlobProjectionSelector which has GAed * Revert changes in SearchIndexerDataSourceType which has GAed * Rename ignoreResetRequirements->skipIndexerResetRequirementForCache * fix validStreamingUnits (Azure#16143) Co-authored-by: Roslyn Lu <[email protected]> * Fix schemas readme files (Azure#16207) * Fix schemas readme files * fix ref * add resourcemanager in module-name (Azure#16206) * add resourcemanager in module-name * add go track2 repo in readme.md * fix * Minor description update for createorupdate & update slot (Azure#15457) * add scmMinTlsVersion * modify description for createorupdate & update slot for all API versions Co-authored-by: Edwin Diaz <[email protected]> * [Hub Generated] Review request for Microsoft.Security to add version preview/2021-08-01-preview (Azure#16096) * Adds base for updating Microsoft.Security from version stable/2021-07-01 to version 2021-08-01-preview * Updates readme * Updates API version in new specs and examples * First pass at Microsoft.Security/standards for review/preview * Addressing Round-1 feedback, adding missing descriptions and changing scoping to match ProxyResource type * First pass for Microsoft.Security\standardComponents * Changes to fix a typo in an example spec * Added Microsoft.Security/standardAssignments * Fix naming convention issue * Removed types based on Hila's feedback * Changes to data model for both API calls * Fix example errors and typos * Add systemData to Microsoft.Security/standards * Changes to fix systemData linting * un-nest systemData fields in examples * More systemData placement * Naming convention name to assignments from standardAssignment * Additional example added * Changes to address descriptive comments on field types * Add systemData readOnly * prettier-fix against examples * Cleaned types to ref standard v2 common-types entry similar to securityForIoT * Re-added v2 folder? * Missed a change here? * Cleanup of v2 type completely, updated readme.md * Adds suppression to readme * Adds suppression to readme * Change modification to suppress operations linting Co-authored-by: Adam Holliday <[email protected]> * Yifanzhou/api version change (Azure#16191) * remove Catalog * update 2021-05-01-preview * delete 2021-09-01 * Update readme.md * Update purviewcatalog.json * change read-only property (Azure#16209) Co-authored-by: Parv Saxena <[email protected]> * Update account.json - make CollectionReferece type writable (Azure#16208) * Update account.json Updating the CollectionReference to be writeable to unblock the customers. * Update account.json Need the default set in the client SDK * Fixed an event name typo in its description. (Azure#16227) * Introduce new API version for Microsoft.ProviderHub 2021-09-01-preview (Azure#15723) * Introduce new API version for Microsoft.ProviderHub 2021-09-01-preview * Add PrivateResourceProviderConfigurations * Add PrivateRP examples * Fix enum type. * Fix enum again * Prettier * Fix prettier error * Add readme.md + prettier * Fix * Fix avocado error * Modify interface * Fix issue * Make it private preview * Remove private RP changes * Update SKU settings Co-authored-by: REDMOND\lakshv <[email protected]> Co-authored-by: He Huang <[email protected]> * fluidrelay_readme_config (Azure#16192) fluidrelay_readme_config * Add community gallery proxy resource (Azure#16043) * [Hub Generated] Public private branch 'dev-storagecache-Microsoft.StorageCache-2021-09-01' (Azure#16152) * Adds base for updating Microsoft.StorageCache from version stable/2021-05-01 to version 2021-09-01 * Updates readme * Updates API version in new specs and examples * 2021-09-01 API * Fix LintDiff Co-authored-by: rebecca337 <[email protected]> Co-authored-by: Rebecca Dupuis <[email protected]> * Fix S360 bugs for swagger FSPG 2020-02-14-preview, 2021-06-01-preview and 2021-06-01 APIs (Azure#15946) * Changed FSPG RestartParameter.failoverMode from string to enum for API 2021-06-01, 2021-06-01-preview, and 2021-06-15-privatepreview * Change enum first letter to lower case * Change to upper case * Fix S360 bugs for swagger FSPG 2020-02-14-preview, 2021-06-01-preview and 2021-06-01 APIs * Add email reminder when PR makes changes in Synapse (Azure#16116) * add email reminder to synapse * fix for comment * ServiceBus add batch tag (Azure#16229) * add code owner to synapse (Azure#16230) * [Datafactory] Power Query model changes for multiple queries (Azure#16158) * [Synapse] Add release tag for management SDK Oct. release (Azure#16213) * update release tag * fix for avocado Co-authored-by: Tianen <[email protected]> Co-authored-by: Zhenglai Zhang <[email protected]> Co-authored-by: Lei Ni <[email protected]> Co-authored-by: najian <[email protected]> Co-authored-by: Nan Jiang <[email protected]> Co-authored-by: Ray Chen <[email protected]> Co-authored-by: Dongwei Wang <[email protected]> Co-authored-by: Dongwei Wang <[email protected]> Co-authored-by: msyyc <[email protected]> Co-authored-by: AriZavala2 <[email protected]> Co-authored-by: PingZhu2232 <[email protected]> Co-authored-by: Mark Cowlishaw <[email protected]> Co-authored-by: Ping Zhu <[email protected]> Co-authored-by: Mohit Chakraborty <[email protected]> Co-authored-by: Roslyn Lu <[email protected]> Co-authored-by: Roslyn Lu <[email protected]> Co-authored-by: JiahuiPeng <[email protected]> Co-authored-by: edwin-msft <[email protected]> Co-authored-by: Edwin Diaz <[email protected]> Co-authored-by: dochollidayxx <[email protected]> Co-authored-by: Adam Holliday <[email protected]> Co-authored-by: yifan-zhou922 <[email protected]> Co-authored-by: Parv Saxena <[email protected]> Co-authored-by: Parv Saxena <[email protected]> Co-authored-by: hvermis <[email protected]> Co-authored-by: xuepingd <[email protected]> Co-authored-by: laxmankumar12 <[email protected]> Co-authored-by: REDMOND\lakshv <[email protected]> Co-authored-by: He Huang <[email protected]> Co-authored-by: Zed Lei <[email protected]> Co-authored-by: kangsun-ctrl <[email protected]> Co-authored-by: brpanask <[email protected]> Co-authored-by: rebecca337 <[email protected]> Co-authored-by: Rebecca Dupuis <[email protected]> Co-authored-by: xunsun-commits <[email protected]> Co-authored-by: Wan Yang <[email protected]> Co-authored-by: soma-ms <[email protected]> * remove blank line * address comments * address the description comment * update the swagger * remove ManagedInstances_ListOutboundNetworkDependenciesByManagedInstance * Update from microsoft.sql 2021-05-01-preview to base branch (Azure#16184) * Update comment.yml for publish pr (Azure#16166) * Update comment.yml * Update comment.yml * Bump dependencies (Azure#16170) * bump avocado * bump mocha * remove submodule (Azure#16171) * add azure-resource-manager-schema to servicelinker (Azure#16176) Co-authored-by: Nan Jiang <[email protected]> * Update pr assignment config (Azure#16175) * [Synapse] - fix scheme for Artifacts and update readme to include KQL scripts for October release (Azure#16035) * [Synapse] - fix scheme for Notebook, Spark Job definition and SQL script * Update dataset, linked service and pipeline * Update release tag for october release * Correct type of result limit * Add arguments for SynapseSparkJobActivityTypeProperties Co-authored-by: Dongwei Wang <[email protected]> * fix (Azure#16164) Merging the change to have accurate Swagger * update swagger files * update readme.md * add missing json files * update custom-words.txt to address the spelling check * remove 201 define in ServerUpdate.json * remove blank line * address comments * address the description comment * update the swagger * remove ManagedInstances_ListOutboundNetworkDependenciesByManagedInstance Co-authored-by: Tianen <[email protected]> Co-authored-by: Zhenglai Zhang <[email protected]> Co-authored-by: Lei Ni <[email protected]> Co-authored-by: najian <[email protected]> Co-authored-by: Nan Jiang <[email protected]> Co-authored-by: Ray Chen <[email protected]> Co-authored-by: Dongwei Wang <[email protected]> Co-authored-by: Dongwei Wang <[email protected]> Co-authored-by: msyyc <[email protected]> Co-authored-by: Tianen <[email protected]> Co-authored-by: Zhenglai Zhang <[email protected]> Co-authored-by: Lei Ni <[email protected]> Co-authored-by: najian <[email protected]> Co-authored-by: Nan Jiang <[email protected]> Co-authored-by: Ray Chen <[email protected]> Co-authored-by: Dongwei Wang <[email protected]> Co-authored-by: Dongwei Wang <[email protected]> Co-authored-by: msyyc <[email protected]> Co-authored-by: AriZavala2 <[email protected]> Co-authored-by: PingZhu2232 <[email protected]> Co-authored-by: Mark Cowlishaw <[email protected]> Co-authored-by: Ping Zhu <[email protected]> Co-authored-by: Mohit Chakraborty <[email protected]> Co-authored-by: Roslyn Lu <[email protected]> Co-authored-by: Roslyn Lu <[email protected]> Co-authored-by: JiahuiPeng <[email protected]> Co-authored-by: edwin-msft <[email protected]> Co-authored-by: Edwin Diaz <[email protected]> Co-authored-by: dochollidayxx <[email protected]> Co-authored-by: Adam Holliday <[email protected]> Co-authored-by: yifan-zhou922 <[email protected]> Co-authored-by: Parv Saxena <[email protected]> Co-authored-by: Parv Saxena <[email protected]> Co-authored-by: hvermis <[email protected]> Co-authored-by: xuepingd <[email protected]> Co-authored-by: laxmankumar12 <[email protected]> Co-authored-by: REDMOND\lakshv <[email protected]> Co-authored-by: He Huang <[email protected]> Co-authored-by: Zed Lei <[email protected]> Co-authored-by: kangsun-ctrl <[email protected]> Co-authored-by: brpanask <[email protected]> Co-authored-by: rebecca337 <[email protected]> Co-authored-by: Rebecca Dupuis <[email protected]> Co-authored-by: xunsun-commits <[email protected]> Co-authored-by: Wan Yang <[email protected]> Co-authored-by: soma-ms <[email protected]>
Regenerating the protocol layer and adding the generated options classes to our convenience layer APIs
Also removing the local copy of the swagger that we had, now that the digital twins GA swagger is in the rest specs repo