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.

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

Security Center DataExportSettingProperties not unmarshalled properly #12724

Closed
beandrad opened this issue Oct 12, 2020 · 6 comments · Fixed by #14015
Closed

Security Center DataExportSettingProperties not unmarshalled properly #12724

beandrad opened this issue Oct 12, 2020 · 6 comments · Fixed by #14015
Labels
Security Service Attention Workflow: This issue is responsible by Azure service team.

Comments

@beandrad
Copy link

Bug Report

  • import path of package in question: github.com/Azure/azure-sdk-for-go/services/preview/security/mgmt/v3.0/security
  • SDK version: 46.4.0

Regarding the Azure API specs, Settinghttps://github.com/Azure/azure-rest-api-specs/blob/27cc07ddd294d98e05cb301e07a72378df9f87e8/specification/security/resource-manager/Microsoft.Security/stable/2019-01-01/settings.json#L175 is of type SettingResource has a field (kind) defined as discriminator, which makes SettingResource polymorphic.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 12, 2020
@ArcturusZhang ArcturusZhang added Security Service Attention Workflow: This issue is responsible by Azure service team. labels Oct 13, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 13, 2020
@ghost
Copy link

ghost commented Oct 13, 2020

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @chlahav.

@ArcturusZhang
Copy link
Member

HI @beandrad thanks for this issue!

Yeah, now I see this API is problematic, if the resource itself is polymorphic, the returned model should have a discriminator. But in this case, the returned model Setting does not have a discriminator, but it directly inherit from SettingResource which has a discriminator and does not introduce more fields. This makes Settings and SettingResource effectively equivalent, but in this way the code generator is not able to detect that discriminator and gives us the wrong code.

I will possibly have a look at this issue in detail. But I assume to actually fix this, we will have to fix the problematic swagger definition to remove that redundant inheritances.

@ArcturusZhang
Copy link
Member

I just had a quick test, the returned JSON from the service is

{"id":"/subscriptions/REDACTED/providers/Microsoft.Security/settings/MCAS","name":"MCAS","type":"Microsoft.Security/settings","kind":"DataExportSettings","properties":{"enabled":true}}

As you can see, this is clearly a DataExportSettings struct, but due to the reasons I explained above, we cannot actually cast this struct (its type is Settings) to DataExportSettings.

And unfortunately we do not have any workaround within the framework of the SDK. I will try to fix this by fixing the swagger, please stay tuned.

@ArcturusZhang
Copy link
Member

ArcturusZhang commented Oct 13, 2020

Hi @beandrad I made some experiments refining the inheritance structure in the swaggers and generating new SDK, the following test code:

	client := security.NewSettingsClient(subsID, "westus2")
	client.Authorizer = authorizer
	resp, err := client.Get(ctx, "MCAS")
	if err != nil {
		return err
	}
	fmt.Println(reflect.TypeOf(resp.Value))

And it gives you this as expected:

security.DataExportSettings

Since we already have an upstream issue tracking this, I will then close this issue. Please stay tuned and I will release a new version as soon as the swagger is fixed. Thanks!

@ArcturusZhang
Copy link
Member

Well, I decide to reopen this and close this from the new release PR.

@beandrad
Copy link
Author

Thanks for looking into it @ArcturusZhang! :)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Security Service Attention Workflow: This issue is responsible by Azure service team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants