microsoftgraph / msgraph-sdk-go

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

Upload app logo resulted in Error #745

Open ritu-rubrik opened 1 month ago

ritu-rubrik commented 1 month ago

Using the below code to upload an Logo for a given App:

headers := abstractions.NewRequestHeaders()
    headers.Add("Content-Type", "image/jpg")
    configuration := &applications.
        ItemLogoRequestBuilderPutRequestConfiguration{
        Headers: headers,
    }
    appLogo, err := os.ReadFile(appLogoFile)
    resp, err := client.Applications().ByApplicationId(appObjectID).Logo().Put(
        ctx,
        appLogo,
        configuration,
    )

However this results in the below error: content type text/html does not have a factory registered to be parsed On debugging further it was found that the APi call results in a a Bad Request 400 erro and the same is getting set in response body resulting in above error. Looks like Content-tye is set to 2 values image/jpeg and application/octet-stream . Not sure if this is causeing the request to the MS graph API to fail

rkodev commented 1 month ago

Hi @ritu-rubrik , Thanks for trying the Graph Go SDK. Could you retry the code without setting the content header i.e remove the line explicitly setting content header headers.Add("Content-Type", "image/jpg")

headers := abstractions.NewRequestHeaders()
-   headers.Add("Content-Type", "image/jpg")
    configuration := &applications.
        ItemLogoRequestBuilderPutRequestConfiguration{
        Headers: headers,
    }
ritu-rubrik commented 1 month ago

Hi @rkodev I had initially tried it without setting the Content-Type

headers := abstractions.NewRequestHeaders()
    configuration := &applications.
        ItemLogoRequestBuilderPutRequestConfiguration{
        Headers: headers,
    }
    appLogo, err := os.ReadFile(appLogoFile)

    resp, err := client.Applications().ByApplicationId(appObjectID).Logo().Put(
        ctx,
        appLogo,
        configuration,
    )

However I ended up getting the below error which resulted in me adding the Content-Type: Content-Type was application/octet-stream. Image Content-Type header should be one of the following: [image/bmp, image/emf, image/wmf, image/jpg, image/jpeg, image/gif, image/ico, image/png, image/exif, image/tif, image/tiff, image/*, images/*].

ritu-rubrik commented 1 month ago

Also one another question I have is if the response is Bad Request , why is it not getting set in OData Error as it is a 4XX status code, instead it is getting set in the response body which is leading to the initial error.content type text/html does not have a factory registered to be parsed

rkodev commented 1 month ago

Thanks for pointing this out,

  1. I will be working on a solution on allowing users to override Content-Type which would be useful to in this case. As you correctly pointed out, whatever Content-Type header have right now, the sdk will override and set it to octet-stream. We will not be able to set the content type on the sdk at this moment as this SDK is "generated".

  2. Also I've noted the issue with the issue with error management. Issue seems to be the underlying service accepts a payload with content type 'application/json' but reports the error as 'text/html'. We will be working on that as well

ritu-rubrik commented 1 month ago

@rkodev Thanks for acknowledging the issue. Is there a timeline for this that I can expect for this change to be available?

rkodev commented 1 month ago

@ritu-rubrik The fix should be ready within the next 7 days

baywet commented 1 month ago

@rkodev Can you open a corresponding issue in the metadata repo please? Fundamentally, the API description should not describe a media type for the request body if the service does not support it. Addressing the issue in the metadata emission phase would fix the issue for all generated clients, not just go.