azure-sdk-for-net icon indicating copy to clipboard operation
azure-sdk-for-net copied to clipboard

[FEATURE REQ] Make Deserialize*** methods public instead of internal

Open DBojsen opened this issue 3 years ago • 9 comments

Library name

Azure.Analytics.Synapse.Artifacts

Please describe the feature.

Please change accessibility level of Deserialize methods such as DeserializeLinkedServiceResource & DeserializeLinkedService to public. I find it unnecessary restricting to have these as internal methods.

I need to be able to take the json representations in my repo and deserialize to the corresponding objects - this is currently blocking me from doing so. This is a big change from Data Factory to Synapse Pipelines, as I can easily deserialize ADF objects from json.

DBojsen avatar Dec 05 '21 21:12 DBojsen

Thank you for your feedback. Tagging and routing to the team member best able to assist.

jsquire avatar Dec 06 '21 13:12 jsquire

For now, I have cloned the repo - changed the access level of the functions I need and built it into a dll. Will be using that instead of the nuget - but I find it unfortunate, that I need to go thru this just to be able to use this functionality

DBojsen avatar Dec 08 '21 08:12 DBojsen

@DBojsen, Can you help me understand your application flow here? I'd like to understand why you're holding these objects as JSON in your repo. Thanks!

annelo-msft avatar Dec 08 '21 18:12 annelo-msft

Hi Anne

When using the git integration, all Synapse Pipeline components are saved (by Synapse) as json documents.

Venlig Hilsen

David Bojsen +45 9181 9050


From: Anne Thompson @.> Sent: Wednesday, December 8, 2021 7:30 PM To: Azure/azure-sdk-for-net Cc: David Bojsen; Mention Subject: Re: [Azure/azure-sdk-for-net] [FEATURE REQ] Make Deserialize methods public instead of internal (Issue #25699)

@DBojsenhttps://github.com/DBojsen, Can you help me understand your application flow here? I'd like to understand why you're holding these objects as JSON in your repo. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Azure/azure-sdk-for-net/issues/25699#issuecomment-989075262, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFJVCUB6WOOAD2N4ZTXUEX3UP6P27ANCNFSM5JNHBIUA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

DBojsen avatar Dec 08 '21 18:12 DBojsen

Hi @DBojsen, thanks for this additional information! I'm looking into what precedents we have around making deserialization methods public in the Azure SDK (it isn't common practice), and working with the service team to understand this scenario better. I'll need more time to understand the options here, but I will reply to you on this issue with a plan once I have it. Thanks!

annelo-msft avatar Dec 08 '21 23:12 annelo-msft

Hi Anne

Thank you for your help. While I do not know about the normal practice in the Azure SDK, I do know that this is supported by the Data Factory nugets, which Synapse Pipelines is the logical successor for.

It would be a shame to let the community either build their own methods for this or require them to ‘hot-wire’ your code.

Venlig Hilsen

David Bojsen +45 9181 9050


From: Anne Thompson @.> Sent: Thursday, December 9, 2021 12:55:11 AM To: Azure/azure-sdk-for-net @.> Cc: David Bojsen @.>; Mention @.> Subject: Re: [Azure/azure-sdk-for-net] [FEATURE REQ] Make Deserialize*** methods public instead of internal (Issue #25699)

Hi @DBojsenhttps://github.com/DBojsen, thanks for this additional information! I'm looking into what precedents we have around making deserialization methods public in the Azure SDK (it isn't common practice), and working with the service team to understand this scenario better. I'll need more time to understand the options here, but I will reply to you on this issue with a plan once I have it. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Azure/azure-sdk-for-net/issues/25699#issuecomment-989322183, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFJVCUGSKJBXDYMS3LRFP5DUP7V57ANCNFSM5JNHBIUA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

DBojsen avatar Dec 09 '21 04:12 DBojsen

@annelo-msft, see the issue I just referenced for another need for cross-library JSON deserialization. ResourceGraph SDK can be used to fetch JSON representing Azure resources, and the recommended way to use them is something like this:

ArmClient client = new(credential);
TenantCollection tenants = client.GetTenants();
await foreach (TenantResource tenant in tenants)
{
    QueryContent query = new(@"
Resources
| where type == ""microsoft.keyvault/vaults""
| order by name asc
");
    QueryResponse response = await tenant.ResourcesAsync(query);
    IList<IDictionary<string, JsonElement>> vaultDatas = response.Data.ToObjectFromJson<IList<IDictionary<string, JsonElement>>>();
}

It's super lousy to interact with IDictionary<string, JsonElement> instances though, compared to a proper type like KeyVaultData. I'm hacking around this by using a reflection-aware JsonConverter implementation (see referenced issue), but it'd be great if the Azure SDKs would just stop trying to hide their JSON serializer/deserializer methods. 😄

AtOMiCNebula avatar Dec 22 '22 23:12 AtOMiCNebula

Hi @AtOMiCNebula -

Thanks for your inquiry! @m-nash can address questions regarding management plane libraries such as Azure.ResourceManager.KeyVault.

annelo-msft avatar Jan 04 '23 23:01 annelo-msft

@annelo-msft I don't see any reason to have a separate answer here for management plane. Consistency across all azure sdks would be the priority. Once we have the results of your investigation from above lets make sure its compatible.

m-nash avatar Jan 05 '23 19:01 m-nash

Hi @DBojsen, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

github-actions[bot] avatar Mar 04 '24 18:03 github-actions[bot]

Hi @DBojsen, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

github-actions[bot] avatar Apr 03 '24 18:04 github-actions[bot]