Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpClient sends the wrong Accept header #28915

Open
the-avid-engineer opened this issue Feb 22, 2019 · 15 comments
Open

HttpClient sends the wrong Accept header #28915

the-avid-engineer opened this issue Feb 22, 2019 · 15 comments

Comments

@the-avid-engineer
Copy link

🐞 bug report

Affected Package

@angular/common/http

Is this a regression?

Not sure

Description

The get<T> method on an HttpClient will send the following Accept header with a request if you do not override it:

Accept: application/json, text/plain, */*

This suggests that it is OK if the server responds with text/plain (i.e., not JSON).

However, if the server responds with something similar to this (truncated), you will get an error.

Content-Type: text/plain; charset=utf-8

59fb693e0302a519e0974379

Clearly text/plain is not a valid response type, and the default Accept header should be:

Accept: application/json

🔬 Minimal Reproduction

I will add this later if deemed necessary.

🔥 Exception or Error

"error": {
   "name": "SyntaxError",
   "message": "Unexpected token f in JSON at position 2"
  }

🌍 Your Environment

Angular Version:

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/


Angular CLI: 7.3.2
Node: 10.14.1
OS: win32 x64
Angular: 7.2.5
... animations, common, compiler, compiler-cli, core, forms
... http, language-service, platform-browser
... platform-browser-dynamic, platform-server, router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.13.2
@angular-devkit/build-angular     0.13.2
@angular-devkit/build-optimizer   0.13.2
@angular-devkit/build-webpack     0.13.2
@angular-devkit/core              7.3.2
@angular-devkit/schematics        7.3.2
@angular/cdk                      7.3.2
@angular/cli                      7.3.2
@angular/material                 7.3.2
@ngtools/webpack                  7.3.2
@schematics/angular               7.3.2
@schematics/update                0.13.2
rxjs                              6.4.0
typescript                        3.2.4
webpack                           4.29.5
@m-henderson
Copy link

@the-avid-engineer could you please provide a minimal reproduction?

@the-avid-engineer
Copy link
Author

Yes I will do that

@m-henderson
Copy link

@the-avid-engineer awesome! when you have it just let me know and I will check it out.

@daleclements
Copy link

@m-henderson Well I've found this issue 5 months after the last comment and figured I'd make the repro myself: https://stackblitz.com/edit/angular-issue-repro2-fjrew5 here you go

Just open dev tools and you can see that the GET has accept: application/json, text/plain, */*

Which is a lie. It will not, in fact, accept anything other than application/json. If HttpClient will attempt to parse the response as JSON by default, then it should not send anything other than application/json in the accept header by default. Right?

@the-avid-engineer
Copy link
Author

@daleclements thanks for making a repro

@the-avid-engineer
Copy link
Author

the-avid-engineer commented Feb 11, 2020

I've been a bit busy, and haven't had any time to reproduce it in the scenario I described... but here's a bit more info.

I have several endpoints running on .NET Core / MVC which return strings.

Given an endpoint that returns a string, and a request with two acceptable content types (application/json or text/plain) with the same q-factor, MVC prefers to return text/plain with a string value (e.g., abcd) in the body as opposed to application/json with a string literal (e.g., "abcd") in the body.

I've overridden my calls to always only send application/json as a workaround.

@alxhub
Copy link
Member

alxhub commented May 29, 2020

Apparently I spoke too soon - while HttpRequest doesn't add one, HttpXhrBackend apparently does. Likely this behavior was copied for compatibility with @angular/http.

We should at a minimum evaluate whether this is necessary or at all desirable. Technically though, it would be a breaking change, so it would have to happen in Angular v11.

@hungnguyenmanh82
Copy link

hungnguyenmanh82 commented Aug 24, 2020

this bug hasn't been fixed. I am working on Angular v10.0.7.
I prefer httpClient.get(url) or httpClient.get<HttpResponse>(url) than httpClient.get(url, option).
Actually, httpClient.get<HttpResponse>(url) doesn't work now.
if users wan't more 'Accept" type like /, etc , they should add it via headers

dont work here:

   this.httpClient
      .get<HttpResponse<string>>('http://localhost:8080/GET-json', {
        headers: headers,
        // observe: 'response' 
        params: params,
        // responseType: 'text' 
      })
      .subscribe({
        next: (res) => {
          this.response = res;   // it doesn't work here  => I prefer it work the same at the below code
        },
        error: (err) => {
          console.log('error:' + err);
        },
      });

work well here:

    this.httpClient
      .get('http://localhost:8080/GET-json', {
        headers: headers,
        observe: 'response'             /* I don't like this way*/
        params: params,
        responseType: 'text'            /* I don't like this way */,
      })**
      .subscribe({
        next: (res) => {
          this.response = res;    // it works well but "Accept: application/json, text/plain, */*" => expectation: "Accept: text/plain"*
        },
        error: (err) => {
          console.log('error:' + err);
        },
      });

httpClient.get<HttpResponse>(url) and httpClient.get<HttpResponse>(url) should be implemented in the future

@mlc-mlapis
Copy link
Contributor

@hungnguyenmanh82 It's not a bug. It works as designed and was many times explained why this way has been chosen. Btw, httpClient.get<HttpResponse>(url) is just typing - it doesn't affect the run-time code in any way.

@gkalpak
Copy link
Member

gkalpak commented Aug 24, 2020

BTW, adding this header also (currently) breaks HTTP2 preloading. See #34899 (comment) for details.

@jelbourn jelbourn added P4 A relatively minor issue that is not relevant to core functions and removed severity2: inconvenient labels Oct 1, 2020
@the-avid-engineer
Copy link
Author

IMO there should be at least a way to opt-out of this behavior

@IgorMinar
Copy link
Contributor

The text/plain, */* was added to application/json primarily to support legacy backends, especially those serving static .json files and not recognizing them and serving them with a proper mime type.

It might be safe these days to just default to application/json and provide an api to turn on legacy header support (or just document how to set the header for legacy servers).

However, that would still not solve the problem with preloading captured in #34899 (comment). Defaulting to nothing at all might be too breaky though... so maybe the best path forward is to:

  • update the default to application/json,
  • document how to adjust the Accept header for non-standard backends, and
  • provide a way to opt out of setting the Accept header at all

Thoughts?

@gkalpak
Copy link
Member

gkalpak commented Apr 2, 2021

Not sure how breaky it would be to change the default to application/json (both in popular prod servers and dev servers), so I don't have an opinion on that.

But providing a way to opt out of setting the header or (even better) having a way to set the default/fallback headers would be a non-breaking and quite flexible, so +1 from me.
(FWIW, I like how AngularJS' $http handles setting default/fallback headers 🤔)

@petebacondarwin
Copy link
Member

I think a breaking change for v12 to change the default would be fine, if we can make it easy to update the default without having to change all requests.

@thw0rted
Copy link

Hello from the future! Any plans to change this behavior? Also, is #48505 a duplicate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests