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

fix(http): prevent headers from throwing an error when initializing numerical values #49379

Closed

Conversation

danilobassi8
Copy link

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?

Some libraries could use numbers in headers, but Angular is throwing an error when initializing Headers with numerical values

Issue Number: #49353

What is the new behavior?

Angular won't throw an error if there's some number in HTTP Headers. It will just cast those values into string.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla
Copy link

google-cla bot commented Mar 9, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@pullapprove pullapprove bot requested a review from jessicajaniuk March 9, 2023 13:37
@danilobassi8 danilobassi8 force-pushed the fix-headers-assertion-for-ints branch from a09deaa to 6896ca6 Compare March 9, 2023 13:55
@ngbot ngbot bot added this to the Backlog milestone Mar 9, 2023
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for this!

@jessicajaniuk jessicajaniuk added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Mar 13, 2023
packages/common/http/src/headers.ts Outdated Show resolved Hide resolved
packages/common/http/src/headers.ts Outdated Show resolved Hide resolved
@alxhub alxhub added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Mar 14, 2023
@danilobassi8 danilobassi8 force-pushed the fix-headers-assertion-for-ints branch from 6896ca6 to da6a4f4 Compare March 14, 2023 16:49
@danilobassi8 danilobassi8 force-pushed the fix-headers-assertion-for-ints branch 3 times, most recently from cb6c081 to f8e6c39 Compare March 20, 2023 14:51
@danilobassi8 danilobassi8 requested review from JeanMeche and removed request for alxhub March 21, 2023 19:01
@JeanMeche
Copy link
Member

You will need to update the golden files with yarn bazel run //packages/common:common_api.accept to pass the circleci test !

…umerical values

Some libraries could use numbers in headers. this fix prevents Angular from
throwing an error by casting those numerical values into strings.

Fixes angular#49353
@danilobassi8 danilobassi8 force-pushed the fix-headers-assertion-for-ints branch from f8e6c39 to b7f88b1 Compare March 21, 2023 19:10
@danilobassi8
Copy link
Author

You will need to update the golden files with yarn bazel run //packages/common:common_api.accept to pass the circleci test !

@JeanMeche Done! let me know if there's something more to be done. Its my first time opening a PR to this repo!

@danilobassi8 danilobassi8 requested review from alxhub and jessicajaniuk and removed request for JeanMeche and alxhub March 23, 2023 16:11
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@danilobassi8
Copy link
Author

Reviewed-for: public-api

Let me know if there's something more I can do to clear the cleanup tag. I don't really know your usual workflow, so I'm just waiting for instructions

@jessicajaniuk jessicajaniuk removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 13, 2023
@jessicajaniuk jessicajaniuk added the action: merge The PR is ready for merge by the caretaker label Apr 13, 2023
@ngbot
Copy link

ngbot bot commented Apr 13, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "pullapprove" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jessicajaniuk jessicajaniuk added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 13, 2023
@jessicajaniuk
Copy link
Contributor

Caretaker: this is safe to merge

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit ab5e2d9.

jessicajaniuk pushed a commit that referenced this pull request Apr 13, 2023
…umerical values (#49379)

Some libraries could use numbers in headers. this fix prevents Angular from
throwing an error by casting those numerical values into strings.

Fixes #49353

PR Close #49379
jessicajaniuk pushed a commit that referenced this pull request Apr 13, 2023
…umerical values (#49379)

Some libraries could use numbers in headers. this fix prevents Angular from
throwing an error by casting those numerical values into strings.

Fixes #49353

PR Close #49379
@danilobassi8
Copy link
Author

This PR was merged into the repository by commit ab5e2d9.

Great! What is the min version that I should use to see this changes on my project?? (If its already published)

@JeanMeche
Copy link
Member

JeanMeche commented Apr 17, 2023

You can see that the PR was tagged as target: Patch so it will land in the next patch. (Likely this week I think)

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Apr 27, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.2.6/15.2.8) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.2.6/15.2.8) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.2.6/15.2.8) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.2.6/15.2.8) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.2.6/15.2.8) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.2.6/15.2.8) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.2.6/15.2.8) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.2.6/15.2.8) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v15.2.8`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1528-2023-04-19)

[Compare Source](angular/angular@15.2.7...15.2.8)

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [2fff8fadbe](angular/angular@2fff8fa) | fix | handle invalid classes in class array bindings ([#&#8203;49924](angular/angular#49924)) |

##### http

| Commit | Type | Description |
| -- | -- | -- |
| [05a0225deb](angular/angular@05a0225) | fix | prevent headers from throwing an error when initializing numerical values ([#&#8203;49379](angular/angular#49379)) |

##### router

| Commit | Type | Description |
| -- | -- | -- |
| [09a42d988e](angular/angular@09a42d9) | fix | canceledNavigationResolution: 'computed' with redirects to the current URL ([#&#8203;49793](angular/angular#49793)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v15.2.7`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1527-2023-04-12)

[Compare Source](angular/angular@15.2.6...15.2.7)

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [b0c1a90f55](angular/angular@b0c1a90) | fix | Produce diagnositc if directive used in host binding is not exported ([#&#8203;49792](angular/angular#49792)) |

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [a40529af2e](angular/angular@a40529a) | fix | Catch FatalDiagnosticError during template type checking ([#&#8203;49792](angular/angular#49792)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [702ec90110](angular/angular@702ec90) | fix | When using setInput, mark view dirty in same way as `markForCheck` ([#&#8203;49747](angular/angular#49747)) |

#### Special Thanks

Alan Agius, Andrew Kushnir, Andrew Scott, Kristiyan Kostadinov, Matthieu Riegler and Nikola Kološnjaji

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS40MS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNjEuMCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1863
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@angular-automatic-lock-bot
Copy link

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 May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common/http merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants