Skip to content

[Synapse] - Fixed deserialization error when create Pipeline/Dataset/Trigger through DefinitionFile #13721

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

Merged

Conversation

idear1203
Copy link
Contributor

@idear1203 idear1203 commented Dec 11, 2020

Description

<TL;DR>
This PR aims to fix deserialization issue for New-*Pipeline/Dataset/Trigger cmdlets.
TL:DR

Synapse has integrated some Azure Data Factory's fundamental features, for example pipeline, dataset, triggers, etc. We refer these resources as artifacts. One common use scenario for New-*Artifact is, users provide a definition file that contains request body, the PowerShell command reads the definition file, deserialize its content to some SDK models and call methods to send a PUT request to create some artifact resource.

Previously, these related PowerShell cmdlets suffer from a deserialization issue. In short, this is because, The PowerShell consumes Azure.Analytics.Synapse.Artifacts C# SDK. The SDK is track 2 data plane SDK. Its serializer/deserializer (methods to serialize/deserialize SDK models) are not exposed. For example, see PipelineResource.Serialization.cs - methods in this file are not public.

Before this change, our deserialization approach in PowerShell is as below: we writes some wrapper classes, let's say PSArtifact.cs. First of call, we leverage Newtonsoft.Json library for serialization to an instance of PSArtifact, then we write and call ToSdkObject method inside PSArtifact to transform PSArtifact to some SDK model SdkArtifact. Then call SDK method with SdkArtifact. This approach is buggy and we have seen several customer reported deserialization failure issues recently. At code level, this is because we have to write lots of ToSdkObject methods by hand and provide parameterless constructor for many files (Otherwise the Newtonsoft.Json will complain that it cannot find proper ctor or call some unexpected ctor).

In this PR, we are trying to fix the deserialization issue by calling SDK's original serialization/de-serialization methods, rather than leverage Newtonsoft.Json library. Due to the invisibility of those SDK serialization methods, we cannot call them directly. So we turn to use reflection instead. The mainly related files are:

Besides the major change that is mentioned above, the PR also does the following items:

  • Upgrade Azure.Analytics.Synapse.Artifacts to the latest version and update code in Set-AzSynapseNotebook to solve an payload validation issue due to a breaking change at backend side.
  • Remove 200+ unused model wrapper classes

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@idear1203
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

{
internal static T DeserializeObject<T>(string rawJsonContent)
{
var document = JsonDocument.Parse(rawJsonContent, default);
Copy link
Contributor

Choose a reason for hiding this comment

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

Feature default literal is not available in C# 7.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Fixed.

@idear1203
Copy link
Contributor Author

@wyunchi-ms could you please help review again?

@wyunchi-ms
Copy link
Contributor

Could you please add these modification into Changelog.md.

@idear1203
Copy link
Contributor Author

idear1203 commented Dec 15, 2020

@wyunchi-ms there is already an entry in the Upcoming Release section:

Fixed deserialization error when create Pipeline/Dataset/Trigger through DefinitionFile

@wyunchi-ms wyunchi-ms merged commit 83d920a into Azure:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants