AuditLogQueries Unknown AuditLogRecordType

[L4r1k]

Hi @baywet,

Sorry to report again, but having another similar metadata issue to #391 and microsoftgraph/msgraph-sdk-go#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]

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

[baywet]

@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]

@baywet The behavior seems inconsistent with that of other languages. (Looks to be highlighted at microsoft/kiota-dotnet#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]

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]

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]

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]

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]

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]

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)