microsoftgraph / msgraph-sdk-javascript

Microsoft Graph client library for JavaScript
https://graph.microsoft.com
MIT License
746 stars 225 forks source link

Multiple headers only take the last one #1048

Open RinkAttendant6 opened 1 year ago

RinkAttendant6 commented 1 year ago

Bug Report

Prerequisites

For more information, see the CONTRIBUTING guide.

Description

Using the client's .header() method multiple times will only send the last header if several of them with the same name.

Console Errors:

No

Screenshots: [If applicable, add screenshots to help explain your problem]

Steps to Reproduce

const client = Client.init({...});

const response = await client
    .api(`/users/${upn}/mailFolders/Inbox/messages`)
    .header('Prefer', 'outlook.body-content-type="html"')
    .header('Prefer', 'IdType="ImmutableId"')
    .get();

console.log(response);

Expected behavior:

Actual behavior:

Flipping the order of the method calls will return messages in HTML format but not with immutable IDs.

Additional Context

The method only takes a string so two values cannot be passed. For example this is not permitted:

    .header('Prefer', ['outlook.body-content-type="html"', 'IdType="ImmutableId"'])

Usage Information

Request ID - Value of the requestId field if you are receiving a Graph API error response

SDK Version - 3.0.4

Node Version - v18.12

Browser Name - [The name of Browser that you are using for SDK]

Version - [The version of the browser you are using]

RinkAttendant6 commented 1 year ago

This behaviour is also apparently with the Graph Explorer.

Is it possible to have multiple Prefer headers in general? If so, what is the proper way to do this?

nikithauc commented 1 year ago

@RinkAttendant6

Can you try the following and let me know if that works?

image

The values need to be comma-separated.

@peombwa Thank you for your help with this!

RinkAttendant6 commented 1 year ago

Yes this seems to work! I suppose it would be beneficial to have it documented somewhere

sebastienlevert commented 1 year ago

@RinkAttendant6, I think your point is valid, though. @nikithauc what would be the impact to support a string array and a simple string as parameters and us do the work to separate them with commas? I feel this should be the SDK role to help devs and not the devs to understand the intricacies of Graph and headers...

So basically have this

interface KeyValuePairObjectStringNumber {
    [key: string]: string | number | string[];
}

versus

interface KeyValuePairObjectStringNumber {
    [key: string]: string | number;
}

Thoughts?

nikithauc commented 1 year ago

@RinkAttendant6 Thank you for responding!

@sebastienlevert I think this should be documented in the Graph docs as this information is relevant to any Graph API user.

Headers in most JS/TS libraries are Record<string,string>. If passing an array of strings in a popular use case in the Graph, then we should support that.

nikithauc commented 1 year ago

@jasonjoh Can you please guide on how to document passing of multiple values for a specific header key in Graph?

nikithauc commented 1 year ago

@sebastienlevert I am assigning this issue to you. I think the following actions are needed on this issue:

  1. Documentation on how to pass multiple values to a specific key.
  2. Check for the popularity of the use case to offer something like headers: Record<string, string|string[]>.
jasonjoh commented 1 year ago

So in the case of Prefer header, it's perfectly valid to have multiple instances. Graph accepts both of these:

GET /me/messages
Prefer: IdType="ImmutableId"
Prefer: outlook.body-content-type="text"
GET /me/messages
Prefer: IdType="ImmutableId",outlook.body-content-type="text"
RinkAttendant6 commented 1 year ago

@jasonjoh The first form is what I expected the SDK and Graph Explorer to do when multiple are specified, but the behaviour appears to overwrite rather than append. This isn't surprising in most languages as some sort of Map<K, V> structure containing unique keys is most likely used for the headers.

Doing a bit more research on this, this behaviour of not generating multiple headers of the same name is actually very spec-compliant. Section 3.2.2 of RFC 7230 states:

A sender MUST NOT generate multiple header fields with the same field name in a message unless either the entire field value for that header field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).

(the only "well-known exception" noted is Set-Cookie)

As @sebastienlevert mentioned, the SDK should help devs. I would not expect developers using the SDK (or Graph Explorer) to necessarily know this (even I did not know this until I looked it up). Documentation would certainly be helpful. Even better would be for the SDK and Graph Explorer to handle such cases (either in a spec-compliant manner of joining all the values with commas, or in the non-compliant manner of sending the request multiple headers).

jasonjoh commented 1 year ago

@RinkAttendant6 The Prefer header is an exception. RFC 7240 states:

A client MAY use multiple instances of the Prefer header field in a single message, or it MAY use a single Prefer header field with multiple comma-separated preference tokens. If multiple Prefer header fields are used, it is equivalent to a single Prefer header field with the comma-separated concatenation of all of the tokens.

In .NET, the HttpRequestMessage allows you to add Prefer multiple times without overwriting, for example:

using System.Net.Http;

var request = new HttpRequestMessage();
request.Headers.Add("Prefer", "foo");
request.Headers.Add("Prefer", "bar");
ddyett commented 1 year ago

@darrelmiller @baywet indicates you are working on the design preference on this. we may need to fix this in a number of sdks.

darrelmiller commented 1 year ago

We have two choices. We either use a data structure that allows duplicate header names, or we implement "combined field values" as described by the specification https://www.rfc-editor.org/rfc/rfc9110.html#section-5.2

In my opinion we should investigate implementing combined field values so that we can keep the customer interface to behave like a simple dictionary.

darrelmiller commented 1 year ago

@RinkAttendant6 The 7230 wording is confusing. 9110 does a better job

a sender MUST NOT generate multiple field lines with the same name in a message (whether in the headers or trailers) or append a field line when a field line of the same name already exists in the message, unless that field's definition allows multiple field line values to be recombined as a comma-separated list (i.e., at least one alternative of the field's definition allows a comma-separated list, such as an ABNF rule of #(values) defined in Section 5.6.1).

If a field supports combining with commas, then it is fine to send multiple headers with the same name, or send the field value combined.

baywet commented 1 year ago

right now, for all kiota based SDKs, we're using a dictionary and exposing it publicly. https://github.com/microsoft/kiota-abstractions-dotnet/blob/9290e8943c0df50aa8ede648a3107b3a2a5b2cd4/src/RequestInformation.cs#L116

making it private and switching to add/remove/list/get methods would also allow us to fix casing normalization (not all languages support passing a comparer to the "dictionary") of headers. My only concern if we go the folded way, is how do we know which headers support folding and which don't? I'm guessing we'll have to maintain a hash somewhere, but that feels wrong.

darrelmiller commented 1 year ago

Other than set-cookie, if the headers support multiple headers, then they will support combining values with a comma. We can blindly fold on the client side and the server will 400 because the client application is asking for something that HTTP doesn't support. We can't have an "allow list" because we need to support custom headers that can be combined.

baywet commented 1 year ago

Created this issue to track that for Kiota-generation languages. https://github.com/microsoft/kiota/issues/2032

ddyett commented 1 year ago

moving to engineering to validate with kiota.

andrueastman commented 1 year ago

I believe this was resolved via https://github.com/microsoft/kiota-typescript/pull/460