search

microsoftgraph / msgraph-beta-sdk-go

Microsoft Graph Beta Go SDK
https://learn.microsoft.com/graph/sdks/use-beta?tabs=Go
MIT License
18 stars 5 forks source link

AuditLogQueries Unknown AuditLogRecordType #410

Open L4r1k opened 4 months ago

L4r1k commented 4 months ago

Hi @baywet,

Sorry to report again, but having another similar metadata issue to https://github.com/microsoftgraph/msgraph-beta-sdk-go/issues/391 and https://github.com/microsoftgraph/msgraph-sdk-go/issues/617 where I'm getting the following error when retrieving audit log query records from the new beta endpoint https://learn.microsoft.com/en-us/graph/api/security-auditlogquery-list-records?view=graph-rest-beta&tabs=http .

Unknown AuditLogRecordType value: CopilotInteraction

Occurs when the following value appears in the auditLogRecordType field for an audit log query record returned by the API:

...
"auditLogRecordType":"CopilotInteraction"
...
andrueastman commented 4 months ago

Out of curiosity @L4r1k, does the error block/prevent the parsing of the entire response?

baywet commented 4 months ago

@andrueastman yes, it's by design. @L4r1k thanks for reporting, created this ICM, hopefully the service team is a bit more responsive than the other one. https://portal.microsofticm.com/imp/v3/incidents/incident/499922748/summary

andrueastman commented 4 months ago

@baywet The behavior seems inconsistent with that of other languages. (Looks to be highlighted at https://github.com/microsoft/kiota-abstractions-dotnet/issues/277). I think we should probably look into changing the default to be nil for an enum rather than preventing the parsing of the entire payload.

baywet commented 4 months ago

I'm fine either way, we've received feedback from some people that they prefer a hard failure rather than a silent one. This implementation drift has an even split between languages, I'd like Darrel to weight in before we make any changes either way.

rkodev commented 4 months ago

I would agree with fail first rather than serializing it to a nil. Of concern, it will be almost impossible for developers to know if the enum value was not set i.e this is a null value from the server, or if its a serialization error that was ignored.

andrueastman commented 4 months ago

I'm unfortunately biased to the setting the value to nil in this scenario and not totally failing here.

From a graph perspective, I don't think we want the state of things to be "the API was updated, so your code will break till the metadata is updated" but rather "the API was updated, so you won't access the new member till the metadata is updated".

I think at a certain state/version of the package/library, only certain members of the enum exist from the client's perspective, so the returning of new value defaulting to nil could arguably be saying "nothing you expected came back. Update if you want to receive something new". This is not the same as defaulting to a certain member in the enumeration (e.g. the first value) which would be misleading as nil is not really a member in enumeration. If strictness is what we're going for, nil really shouldn't be an option really.

baywet commented 4 months ago

I think we could shoehorn that change on the parsing side without causing a breaking change since: the parsing methods already return a pointer the parse node interface returns any, which can be nil

L4r1k commented 4 months ago

I would also make the argument that just dropping the entirety of the data on that iteration is not what we should be doing. Particular when it comes to forensics, dropping data is just unacceptable. I've had to spend months making painstaking workarounds for error handling, picking up iteration after the failure, etc. and have had to submit a number of tickets where the metadata has drifted from the SDK and caused this issue. ( I actually just ran into another over the weekend I need to post ).

I would much rather we fill the new field with nil or even return an interface map frankly than drop data because one field was new.

L4r1k commented 4 months ago

To add to the above: It's so severe I'm seriously considering pivoting away from the SDK and just coding up the raw queries myself (something I'd really rather not do as I do want to support the SDK)

andrueastman commented 4 months ago

We'll keep this issue open for the purposes of tracking the metadata issue here.

I think we could shoehorn that change on the parsing side without causing a breaking change since: the parsing methods already return a pointer the parse node interface returns any, which can be nil

Created https://github.com/microsoft/kiota/issues/4621 for the generation change described here.

L4r1k commented 4 months ago

Any update on the ICM for this one @baywet ? Thanks!

baywet commented 4 months ago

They mentioned they are working on a fix as we speak.

L4r1k commented 4 months ago

Any chance it'll be in the next build? 🤞

baywet commented 3 months ago

A pull request is awaiting review and merge on the schema side. Unlikely the fix will be in this week on the SDKs side. However https://github.com/microsoft/kiota/pull/4646 has been merged, so at least the client should stop blowing up with this latest generation.

baywet commented 3 months ago

Update: the service team has made the metadata change about 10 days ago. We're waiting for the central service team to roll out the deployment.

baywet commented 1 month ago

Update: the deployment was never completed for some reason, I have opened another ICM on the service team to understand what's going on. https://portal.microsofticm.com/imp/v3/incidents/incident/528480824/summary

baywet commented 1 week ago

update: the deployment has resumed and the metadata should be updated next week