search

microsoftgraph / msgraph-sdk-go

Microsoft Graph SDK for Go
https://docs.microsoft.com/en-us/graph/sdks/sdks-overview
MIT License
245 stars 38 forks source link

Panic when parsing Photo facet #685

Closed bartek closed 4 months ago

bartek commented 7 months ago

Hello!

Ran into an issue during indexing of drive items where, when the photo facet is present, the Go SDK panics. Here's an example facet from the MS Graph API (v1.0):

{
  "photo": {
    "cameraMake": "Apple",
    "cameraModel": "iPhone 15",
    "exposureDenominator": 742,
    "exposureNumerator": 1,
    "fNumber": 1.6,
    "focalLength": 5.96,
    "iso": 50,
    "orientation": "6",
    "takenDateTime": "2024-04-02T17:21:10.37Z"
  }
}

The panic with (relevant) stack trace is as follows:

panic: interface conversion: interface {} is *string, not *float64

goroutine 118 [running]:
runtime/debug.Stack()
    /usr/local/go/src/runtime/debug/stack.go:24 +0x64
github.com/atolio/lumen/pkg/sdk/runner.(*Runner).executeWithRecover.func1()
    /go/src/pkg/sdk/runner/runner.go:371 +0x114
panic({0x31c9740?, 0x4001814cc0?})
    /usr/local/go/src/runtime/panic.go:914 +0x218
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetFloat64Value(...)
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_parse_node.go:406
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetInt32Value(0x400144ce40?)
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_parse_node.go:411 +0xa0
github.com/microsoftgraph/msgraph-sdk-go/models.(*Photo).GetFieldDeserializers.func9({0x4471050?, 0x400144ce40?})
    /go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.25.0/models/photo.go:170 +0x34
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetObjectValue(0x400144ce80, 0x400143de40?)
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_parse_node.go:225 +0x368
github.com/microsoftgraph/msgraph-sdk-go/models.(*DriveItem).GetFieldDeserializers.func18({0x4471050?, 0x400144ce80?})
    /go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.25.0/models/drive_item.go:287 +0x3c
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetObjectValue(0x400144cf00, 0x0?)
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_parse_node.go:225 +0x368
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetCollectionOfObjectValues(0x4001517e80?, 0x41bfd78)
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_parse_node.go:251 +0x16c
github.com/microsoftgraph/msgraph-sdk-go/drives.(*ItemItemsItemDeltaResponse).GetFieldDeserializers.(*ItemItemsItemDeltaGetResponse).GetFieldDeserializers.func1({0x4471050?, 0x4001517e40?})
    /go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.25.0/drives/item_items_item_delta_get_response.go:27 +0x3c
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetObjectValue(0x4001517e60, 0x44627e0?)
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_parse_node.go:225 +0x368
github.com/microsoft/kiota-http-go.(*NetHttpRequestAdapter).Send(0x4000523360, {0x4462818?, 0x400035d4a0?}, 0x40010803c0, 0x40010a4060?, 0x4000fe11e0?)
    /go/pkg/mod/github.com/microsoft/kiota-http-go@v1.1.0/nethttp_request_adapter.go:359 +0x290
github.com/microsoftgraph/msgraph-sdk-go/drives.(*ItemItemsItemDeltaRequestBuilder).Get(0x4000a10f58, {0x4462818, 0x400035d4a0}, 0xf?)
    /go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.25.0/drives/item_items_item_delta_request_builder.go:63 +0xec

As per generated Photo, it seems that an int is expected but the MS Graph API is returning a string, resulting in this panic:

// models/photo.go
res["orientation"] = func (n i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode) error {
    val, err := n.GetInt32Value()
    if err != nil {
        return err
    }
    if val != nil {
        m.SetOrientation(val)
    }
    return nil
}

In browsing other APIs which deal with orientation[1], it seems an int to represent orientation is more common.

Questions:

  1. Can the msgraph-sdk-go project be re-generated to solve the discrepancy between API/client? I am happy to help with this, but I could not find where the relevant spec(s) are managed.
  2. Should this be resolved in the API itself? I understand this may result in breaking issues for existing clients (which may be working around this issue). Curious what the typical flow is here

Thank you!

  1. https://developer.apple.com/documentation/uikit/uiimage/orientation
gil-laminar commented 7 months ago

+1, we are also encountering this issue

gil-laminar commented 7 months ago

Looking through some of the other languages' SDKs, it looks like they all agree on orientation being an Int32, while the API documentation says it's an Int16, and the server sends a string. My guess is that the server's wrong.

bartek commented 7 months ago

FWIW, I worked around this by using $select to ensure I don't receive the photos facet. This is satisfactory for my use case as I don't need this facet for what I am doing, but of course not ideal in general.

gil-laminar commented 7 months ago

Thanks, this is what we're planning to do as well.

baywet commented 7 months ago

Hi everyone, Thank you for using the Go SDK and for reporting this. You are correct the metadata (used to generate the SDK) and the documentation are not aligned with what the service sends. I've opened an incident on the relevant team (you won't be able to open that) https://portal.microsofticm.com/imp/v3/incidents/incident/494219554/summary

If some of you could share client request ids and timestamps for the requests, that'd be helpful as well. (even from Graph Explorer) Thanks!

abezzub commented 6 months ago

This brought down our integration, any ETA on resolution?

coderavels commented 6 months ago

@bartek I tried using select query and even then facing the issue. Here is the code we are using:

var (
    fieldsForGetDelta = []string{"id", "name", "webUrl", "lastModifiedDateTime", "createdDateTime", "createdBy", "lastModifiedBy", "file", "size", "parentReference", "deleted"}
)
...
...
...
var deltaResp drives.ItemItemsItemDeltaGetResponseable
    if url == "" {
        deltaResp, err = client.Drives().ByDriveId(driveID).Items().ByDriveItemId(itemID).Delta().GetAsDeltaGetResponse(ctx, &drives.ItemItemsItemDeltaRequestBuilderGetRequestConfiguration{
            QueryParameters: &drives.ItemItemsItemDeltaRequestBuilderGetQueryParameters{
                Select: fieldsForGetDelta,
            },
        })
        if err != nil {
            errResp := common.FormatODataError(err)
            return resp, core.TraceError(ctx, "error while getting onedrive drive item first delta", errResp)
        }
    } else {
        deltaResp, err = client.Drives().ByDriveId(driveID).Items().ByDriveItemId(itemID).Delta().WithUrl(url).GetAsDeltaGetResponse(ctx, &drives.ItemItemsItemDeltaRequestBuilderGetRequestConfiguration{
            QueryParameters: &drives.ItemItemsItemDeltaRequestBuilderGetQueryParameters{
                Select: fieldsForGetDelta,
            },
        })
        if err != nil {
            errResp := common.FormatODataError(err)
            return resp, core.TraceError(ctx, "error while getting onedrive drive item delta with url", errResp)
        }
    }

Here is the stack trace

github.com/watchtowerai/m365dlp/internal/m365/onedrive/client.azureGraphServiceImpl.GetDriveItemDelta.func2
    /projects/m365dlp/internal/m365/onedrive/client/azure_graph.go:242
runtime.gopanic
    /usr/local/go/src/runtime/panic.go:770
runtime.panicdottypeE
    /usr/local/go/src/runtime/iface.go:262
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetFloat64Value
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.7/json_parse_node.go:463
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetInt32Value
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.7/json_parse_node.go:468
github.com/microsoftgraph/msgraph-sdk-go/models.(*Photo).GetFieldDeserializers.func9
    /go/pkg/mod/github.com/watchtowerai/msgraph-sdk-go@v0.0.0-20240109082659-a38109d4eb90/models/photo.go:170
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetObjectValue
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.7/json_parse_node.go:282
github.com/microsoftgraph/msgraph-sdk-go/models.(*DriveItem).GetFieldDeserializers.func18
    /go/pkg/mod/github.com/watchtowerai/msgraph-sdk-go@v0.0.0-20240109082659-a38109d4eb90/models/drive_item.go:287
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetObjectValue
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.7/json_parse_node.go:282
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetCollectionOfObjectValues
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.7/json_parse_node.go:308
github.com/microsoftgraph/msgraph-sdk-go/drives.(*ItemItemsItemDeltaGetResponse).GetFieldDeserializers.func1
    /go/pkg/mod/github.com/watchtowerai/msgraph-sdk-go@v0.0.0-20240109082659-a38109d4eb90/drives/item_items_item_delta_get_response.go:27
github.com/microsoft/kiota-serialization-json-go.(*JsonParseNode).GetObjectValue
    /go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.7/json_parse_node.go:282
github.com/microsoft/kiota-http-go.(*NetHttpRequestAdapter).Send
    /go/pkg/mod/github.com/microsoft/kiota-http-go@v1.3.1/nethttp_request_adapter.go:359
github.com/microsoftgraph/msgraph-sdk-go/drives.(*ItemItemsItemDeltaRequestBuilder).GetAsDeltaGetResponse
    /go/pkg/mod/github.com/watchtowerai/msgraph-sdk-go@v0.0.0-20240109082659-a38109d4eb90/drives/item_items_item_delta_request_builder.go:82

Could you help with what different you did to use the select query and prevent this issue? Also which version of graph-sdk are you using?

bartek commented 6 months ago

Hey @coderavels. Not sure, I'd recommend logging the response to understand what may be getting parsed. We are accessing the data from the root, like so:

root, err := c.graphClient.Drives().ByDriveId(driveId).Root().Get(ctx, nil)
if err != nil {
    return nil, err
}

if root.GetId() == nil {
    return nil, fmt.Errorf("drive root ID is nil")
}
rootId := *root.GetId()    

deltaReqBldr := c.graphClient.Drives().ByDriveId(driveId).Items().ByDriveItemId(rootId).Delta()
if deltaLink != "" {
    deltaReqBldr = deltaReqBldr.WithUrl(deltaLink)
}

result, err := deltaReqBldr.GetAsDeltaGetResponse(ctx, &drives.ItemItemsItemDeltaRequestBuilderGetRequestConfiguration{
QueryParameters: &drives.ItemItemsItemDeltaRequestBuilderGetQueryParameters{
    Select: []string{
        "id", "lastModifiedDateTime", "name", "webUrl", "size",
        "createdBy", "lastModifiedBy", "parentReference", "file",
        "fileSystemInfo",
    },
},
})
if err != nil {
    return nil, err
}
baywet commented 6 months ago

Thanks everyone for the nudge, I've nudged the concerned service team to get an update.

baywet commented 4 months ago

Hi everyone, Thank you for your patience. While the service team never came back to me on the topic, I took the time to retry the call today and the API now returns integers. Closing.

kyong0612 commented 3 months ago

@bartek Did this bug is fix?

While the service team never came back to me on the topic, I took the time to retry the call today and the API now returns integers.

If the response is an integer, does a similar panic occur during type assertion to a float64?