microsoftgraph / msgraph-sdk-java

Microsoft Graph SDK for Java
https://docs.microsoft.com/en-us/graph/sdks/sdks-overview
MIT License
403 stars 134 forks source link

Error Handling #1785

Closed MichalChotvac closed 9 months ago

MichalChotvac commented 9 months ago

Expected behavior

The message of the ApiException will contain a more specific error message based on the OdataError returned from the API.

Documentation

Actual behavior

When trying to get user who does not exist in Graph java.time.format.DateTimeParseException is thrown

Steps to reproduce the behavior

        try {
            User user = graphServiceClient.users()
                    .byUserId("non_existing_user")
                    .get();
        } catch (ApiException e) {
            System.out.println(e.getMessage());
        }

Result:

java.time.format.DateTimeParseException: Text '2024-02-08T12:07:31' could not be parsed at index 19
    at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2046)
    at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1948)
    at java.base/java.time.OffsetDateTime.parse(OffsetDateTime.java:402)
    at java.base/java.time.OffsetDateTime.parse(OffsetDateTime.java:387)
    at com.microsoft.kiota.serialization.JsonParseNode.getOffsetDateTimeValue(JsonParseNode.java:98)
    at com.microsoft.graph.models.odataerrors.InnerError.lambda$getFieldDeserializers$1(InnerError.java:83)
    at com.microsoft.kiota.serialization.JsonParseNode.assignFieldValues(JsonParseNode.java:247)
    at com.microsoft.kiota.serialization.JsonParseNode.getObjectValue(JsonParseNode.java:198)
    at com.microsoft.graph.models.odataerrors.MainError.lambda$getFieldDeserializers$2(MainError.java:83)
    at com.microsoft.kiota.serialization.JsonParseNode.assignFieldValues(JsonParseNode.java:247)
    at com.microsoft.kiota.serialization.JsonParseNode.getObjectValue(JsonParseNode.java:198)
    at com.microsoft.graph.models.odataerrors.ODataError.lambda$getFieldDeserializers$0(ODataError.java:74)
    at com.microsoft.kiota.serialization.JsonParseNode.assignFieldValues(JsonParseNode.java:247)
    at com.microsoft.kiota.serialization.JsonParseNode.getObjectValue(JsonParseNode.java:198)
    at com.microsoft.kiota.http.OkHttpRequestAdapter.lambda$throwIfFailedResponse$0(OkHttpRequestAdapter.java:672)
    at com.microsoft.kiota.ApiExceptionBuilder.<init>(ApiExceptionBuilder.java:26)
    at com.microsoft.kiota.http.OkHttpRequestAdapter.throwIfFailedResponse(OkHttpRequestAdapter.java:671)
    at com.microsoft.kiota.http.OkHttpRequestAdapter.send(OkHttpRequestAdapter.java:279)
    at com.microsoft.graph.users.item.UserItemRequestBuilder.get(UserItemRequestBuilder.java:661)
    at com.microsoft.graph.users.item.UserItemRequestBuilder.get(UserItemRequestBuilder.java:647)

Library:

        <dependency>
            <groupId>com.microsoft.graph</groupId>
            <artifactId>microsoft-graph</artifactId>
            <version>6.1.0</version>
        </dependency>
UXMichael commented 9 months ago

Exactly same exception here when i am trying to download a file:

graphClient.drives().byDriveId("XXXXXXXXXXXXX") .items().byDriveItemId("root:/MediaMeta.xml").content().get()

Ndiritu commented 9 months ago

Failure happens during deserialization of the innerError's date property. The API returns a date time string without the offset e.g. "2024-02-12T19:47:39" but our OpenAPI spec expects a DateTime with an offset which maps to the SDK expecting DateTimeOffset objects. The parsing failure is valid.

.NET's DateTimeOffset implementation seems to handle this case by defaulting to using the UTC timezone. Java's fails.

@baywet @andrueastman might this be an issue during our metadata conversion?

andrueastman commented 9 months ago

Kiota maps the format: date-time to DateTimeOffset which downstream languages change in the refiners. Which looks like the right thing to do.

However, the pattern string/regex suggest that the offset is optional despite the OpenApi spec defining the offset as mandatory for date-time. This is probably because odata ABNF has the the offset as optional.

        date:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          description: Date when the error occured.
          format: date-time
          nullable: true

Ideally,

However, since it would it still end up being to the DateTimeOffset type, I think we probably would end need to end up just updating the deserialization library to allow for the scenario and default to UTC.

Ndiritu commented 9 months ago

Thank for the clarity @andrueastman! I agree that we can add a default UTC offset if none is present in the API response before deserializing to a DateTimeOffset.

baywet commented 9 months ago

Exactly what Andrew said. Just adding a bit of tolerance when parsing things here should be enough. It's interesting that Java is the only language with which we've faced that to date though, but it makes sense since Java "is more of a purist language".

Ndiritu commented 9 months ago

Version 6.2.1 contains this fix.