Skip to content

fix(common): add right ContentType for boolean values with HttpClient request body(#38924)#41885

Closed
gopal-jayaraman wants to merge 1 commit intoangular:masterfrom
gopal-jayaraman:gopj-content-type
Closed

fix(common): add right ContentType for boolean values with HttpClient request body(#38924)#41885
gopal-jayaraman wants to merge 1 commit intoangular:masterfrom
gopal-jayaraman:gopj-content-type

Conversation

@gopal-jayaraman
Copy link
Copy Markdown
Contributor

@gopal-jayaraman gopal-jayaraman commented Apr 29, 2021

We are adding application/json as ContentType for boolean as the body, currently it is seen as text/plain, where it should be seen as application/json, since it is valid JSON, like numbers.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

currently a boolean as the body is seen as text/plain, where it should be seen as application/json, since it is valid JSON, like numbers.

Issue Number: #38924

What is the new behavior?

currently a boolean as the body is seen as text/plain, where it should be seen as application/json, since it is valid JSON, like numbers.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Apr 29, 2021
petebacondarwin
petebacondarwin approved these changes Apr 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this shouldn't be a breaking change, but I wonder if it would actually break some people who have relied on booleans being marked as text...

@AndrewKushnir AndrewKushnir mentioned this pull request Apr 29, 2021
Closed
3 tasks
@gopal-jayaraman gopal-jayaraman force-pushed the gopj-content-type branch 2 times, most recently from f76484a to d1bee36 Compare April 30, 2021 00:17
alfaproject
alfaproject reviewed Apr 30, 2021
View reviewed changes
packages/common/http/src/request.ts Outdated
Copy link
Copy Markdown
Contributor

@alfaproject alfaproject Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.isArray is redundant because this.body is an object in that case

Copy link
Copy Markdown
Contributor

@alfaproject alfaproject Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S.: Update the comment in line 325

Copy link
Copy Markdown
Contributor Author

@gopal-jayaraman gopal-jayaraman Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alfaproject - I have updated the review comments, Please check.

@gopal-jayaraman
fix(common): add right ContentType for boolean values with HttpClient…
1cb2bdb 
… request body(angular#38924)

currently a boolean as body is seen as text/plain, where is should be seen as application/json, since it is valid JSON, like numbers.
@petebacondarwin petebacondarwin added action: presubmit The PR is in need of a google3 presubmit area: common/http Issues related to HTTP and HTTP Client target: patch This PR is targeted for the next patch release labels Apr 30, 2021
@ngbot ngbot bot modified the milestone: Backlog Apr 30, 2021
@petebacondarwin petebacondarwin removed the request for review from jessicajaniuk April 30, 2021 16:34
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Apr 30, 2021

Presubmit.

@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented May 1, 2021

@petebacondarwin FYI presubmits are successful for the changes in this PR.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label May 1, 2021
@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label May 1, 2021
mhevery pushed a commit that referenced this pull request May 3, 2021
@gopal-jayaraman @mhevery
fix(common): add right ContentType for boolean values with HttpClient…
ae0fa08 
… request body(#38924) (#41885)

currently a boolean as body is seen as text/plain, where is should be seen as application/json, since it is valid JSON, like numbers.

PR Close #41885
mhevery pushed a commit that referenced this pull request May 3, 2021
@gopal-jayaraman @mhevery
fix(common): add right ContentType for boolean values with HttpClient…
922a602 
… request body(#38924) (#41885)

currently a boolean as body is seen as text/plain, where is should be seen as application/json, since it is valid JSON, like numbers.

PR Close #41885
@mhevery mhevery closed this in 6e11feb May 3, 2021
@JoostK JoostK mentioned this pull request May 7, 2021
Closed
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Jun 3, 2021

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@alfaproject alfaproject alfaproject left review comments

@petebacondarwin petebacondarwin petebacondarwin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: common/http Issues related to HTTP and HTTP Client cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@gopal-jayaraman @AndrewKushnir @petebacondarwin @alfaproject