msgraph-sdk-javascript icon indicating copy to clipboard operation
msgraph-sdk-javascript copied to clipboard

Multiple headers only take the last one

Open RinkAttendant6 opened this issue 3 years ago • 18 comments

Bug Report

Prerequisites

  • [x] Can you reproduce the problem?
  • [x] Are you running the latest version?
  • [x] Are you reporting to the correct repository?
  • [x] Did you perform a cursory search?

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:

  • Message body in HTML format
  • Immutable IDs

Actual behavior:

  • Message body in text format (default if unspecified)
  • Immutable IDs

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

  • [x] Node (Check, if using Node version of SDK)

Node Version - v18.12

  • [ ] Browser (Check, if using Browser version of SDK)

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

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

RinkAttendant6 avatar Nov 13 '22 04:11 RinkAttendant6

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?

RinkAttendant6 avatar Nov 13 '22 05:11 RinkAttendant6

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

nikithauc avatar Nov 16 '22 19:11 nikithauc

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

RinkAttendant6 avatar Nov 17 '22 01:11 RinkAttendant6

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

sebastienlevert avatar Nov 17 '22 16:11 sebastienlevert

@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 avatar Nov 17 '22 19:11 nikithauc

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

nikithauc avatar Nov 17 '22 19:11 nikithauc

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

nikithauc avatar Nov 17 '22 19:11 nikithauc

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"

jasonjoh avatar Nov 17 '22 20:11 jasonjoh

@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).

RinkAttendant6 avatar Nov 18 '22 02:11 RinkAttendant6

@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");

jasonjoh avatar Nov 18 '22 17:11 jasonjoh

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

ddyett avatar Nov 30 '22 19:11 ddyett

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 avatar Nov 30 '22 22:11 darrelmiller

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

darrelmiller avatar Nov 30 '22 22:11 darrelmiller

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.

baywet avatar Dec 01 '22 13:12 baywet

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.

darrelmiller avatar Dec 01 '22 15:12 darrelmiller

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

baywet avatar Dec 06 '22 19:12 baywet

moving to engineering to validate with kiota.

ddyett avatar Jul 25 '23 04:07 ddyett

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

andrueastman avatar Jul 25 '23 06:07 andrueastman