HttpParams.toString() modifies the immutable object #20430

rfuhrer · 2017-11-14T21:05:31Z

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

There's really 2 issues here:

Calling toString() modifies the HttpParams object.
When toString() moves all the 'updates' to the 'map', it doesn't clear the 'updates' list
The combination of these both means that params get duplicated if you ever use toString()

Expected behavior

toString() shouldn't cause modifications to the HttpParams object at all. The documentation reads that This class is immutable - all mutation operations return a new instance. which isn't true for this method. If there's some technical reason why this happens then:
toString() should clear the updates list when it generates the map

Minimal reproduction of the problem with instructions

let test = new HttpParams();
    test = test.append('1', '1'); //map:[], updates:[add 1]
    console.log(test.toString()); //1=1
    test = test.append('2', '2');//map:[1], updates:[add 1, add 2]
    console.log(test.toString()); // 1=1&1=1&2=2

http://plnkr.co/edit/pAWaOvymsJwUpnn51ft8?p=preview

What is the motivation / use case for changing the behavior?

I have a screen where I want to update the Params in the URL as you set search parameters so that if you share it with someone or bookmark the search results it will know all the parameters. The issue is that the set of params I want to save to the URL doesn't include all the params that are used in the service. So I create a HttpParams object and set the params that are in the URL, then I call this.location.replaceState(this.baseUrl, params.toString());, then I send the params off to a service that adds a few more parameters before being fed into a this.http.get<ISearchResult>() call. Doing this means that the request that goes to the server now has duplicates of all the parameters added to start.

Environment


Angular version: 5.0.1


Browser:
- [x] Chrome (desktop) version 62
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: 8.4.0  
- Platform:  Windows

Others:

The text was updated successfully, but these errors were encountered:

DulithaRanatunga · 2017-11-17T06:34:25Z

I just came across this in Angular 4.4.6 as well. :(

DulithaRanatunga · 2017-11-21T04:47:02Z

It seems this bug also affects:

HttpParams.get()
HttpParams.getAll()
HttpParams.keys()
HttpParams.has()

Looking through: https://github.com/angular/angular/blob/5.0.2/packages/common/http/src/params.ts
Its because each of these methods call .init() and init() populates the map without clearing updates.

As a workaround, I create my own params class which contains a proxy HttpParams. But its really not ideal that an ''immutable'' object is modified.
e.g.

  public toString(): string {
    this.applyUpdates();
    return this.proxy.toString();
  }

  private applyUpdates(): void {
    this.proxy = new HttpParams({ fromString: this.proxy.toString() });
  }

worldsayshi · 2018-06-19T09:41:18Z

Here's a reproduction in Angular 6:
https://stackblitz.com/edit/angular-mujmjf

This makes HttpParams near unusable to me. I don't get how this doesn't affect almost everyone using Angular.

rfuhrer · 2018-09-06T22:49:23Z

@alxhub Can I get state: has PR added to this? Will this ever get reviewed? I was kind of excited to push some code to a major project like Angular. That excitement died about 240 days ago 😞

Is there some new-new HttpClient coming out that is the reason we're not fixing bugs in the current "new" one?

philjones88 · 2018-10-24T12:10:39Z

Just hit this too.

We were looping over an objects keys/values but inside the for loop were calling .has() it created duplicate entries.

The solution / hack was to remove any .has or other operations inside the loop and use .set() only (note, weirdly .set() and .append() seem to do the same?)

rfuhrer · 2019-01-09T22:17:47Z

421 days since this bug was reported. If the angular team is never going to look at my pull request (which is 412 days old now) can you at least look at fixing the bug? I just ran into this case again on a table with pagination and sorting and after 3 clicks of a sort you get an absolutely ridiculous URI like this if you don't hack around the bug: https://localhost:44356/RssServicing/GetSearchResults?component=Rss&component=Rss&component=Rss&component=Rss&component=Rss&component=Rss&statusIds=1,2,4,5&statusIds=1,2,4,5&statusIds=1,2,4,5&statusIds=1,2,4,5&statusIds=1,2,4,5&statusIds=1,2,4,5&page=0&pageSize=25&sort=serialNumber&reverse=true

maxtacco · 2019-02-28T23:23:51Z

Wow, spent an hour debugging this. How does this gets into production code considering this is common API and no fixes after more than a year?

Previously, an instance of HttpParams would retain its list of mutations after they have been materialized as a result of a read operation. Not only does this unnecessarily hold onto memory, more importantly does it introduce a bug where branching of off a materialized instance would reconsider the set of mutations that had already been applied, resulting in repeated application of mutations. This commit fixes the bug by clearing the list of pending mutations after they have been materialized, such that they will not be considered once again for branched off instances. Fixes angular#20430

andrei-tatar · 2019-03-29T13:34:57Z

Same problem when using params.delete (running 7.2.11)

const newReq = req.clone({
  ...
  params: req.params.delete(KEY),
});

My newReq has all the other params doubled now...

…9045) Previously, an instance of HttpParams would retain its list of mutations after they have been materialized as a result of a read operation. Not only does this unnecessarily hold onto memory, more importantly does it introduce a bug where branching of off a materialized instance would reconsider the set of mutations that had already been applied, resulting in repeated application of mutations. This commit fixes the bug by clearing the list of pending mutations after they have been materialized, such that they will not be considered once again for branched off instances. Fixes #20430 PR Close #29045

…gular#29045) Previously, an instance of HttpParams would retain its list of mutations after they have been materialized as a result of a read operation. Not only does this unnecessarily hold onto memory, more importantly does it introduce a bug where branching of off a materialized instance would reconsider the set of mutations that had already been applied, resulting in repeated application of mutations. This commit fixes the bug by clearing the list of pending mutations after they have been materialized, such that they will not be considered once again for branched off instances. Fixes angular#20430 PR Close angular#29045

angular-automatic-lock-bot · 2019-09-14T23:53:28Z

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

jasonaden added the area: common/http label Nov 14, 2017

rfuhrer pushed a commit to rfuhrer/angular that referenced this issue Nov 24, 2017

fix(http): rework HttpParams to always be immutable (angular#20430)

e0da761

rfuhrer mentioned this issue Nov 24, 2017

fix(http): rework HttpParams to always be immutable (#20430) #20610

Closed

3 tasks

alxhub added freq2: medium severity3: broken type: bug/fix labels Nov 28, 2017

rfuhrer mentioned this issue Nov 29, 2017

HttpRequest.clone is duplicating query parameters (type 'append') #18812

Closed

8 tasks

ngbot bot added this to the Backlog milestone Jan 23, 2018

ngbot bot modified the milestones: Backlog, needsTriage Feb 26, 2018

trotyl mentioned this issue Apr 18, 2018

HttpParams.toString multiplication after parameter deletion #23432

Closed

rfuhrer pushed a commit to rfuhrer/angular that referenced this issue Jun 19, 2018

fix(http): rework HttpParams to always be immutable (angular#20430)

83eeacd

trotyl mentioned this issue Jan 18, 2019

Hi i found this problem when i use HttpParams.append #28227

Closed

rfuhrer mentioned this issue Jan 23, 2019

ReactiveForms: Changing Validators doesn't trigger recalculation of form/field statuses #28216

Closed

JoostK mentioned this issue Feb 28, 2019

fix(common): prevent repeated application of HttpParams mutations #29045

Closed

14 tasks

benlesh closed this as completed in 8e8e89a Apr 23, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpParams.toString() modifies the immutable object #20430

HttpParams.toString() modifies the immutable object #20430

rfuhrer commented Nov 14, 2017

DulithaRanatunga commented Nov 17, 2017

DulithaRanatunga commented Nov 21, 2017

worldsayshi commented Jun 19, 2018 •

edited

Loading

rfuhrer commented Sep 6, 2018

philjones88 commented Oct 24, 2018

rfuhrer commented Jan 9, 2019

maxtacco commented Feb 28, 2019

andrei-tatar commented Mar 29, 2019 •

edited

Loading

angular-automatic-lock-bot bot commented Sep 14, 2019

HttpParams.toString() modifies the immutable object #20430

HttpParams.toString() modifies the immutable object #20430

Comments

rfuhrer commented Nov 14, 2017

I'm submitting a...

Current behavior

Expected behavior

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

Environment

DulithaRanatunga commented Nov 17, 2017

DulithaRanatunga commented Nov 21, 2017

worldsayshi commented Jun 19, 2018 • edited Loading

rfuhrer commented Sep 6, 2018

philjones88 commented Oct 24, 2018

rfuhrer commented Jan 9, 2019

maxtacco commented Feb 28, 2019

andrei-tatar commented Mar 29, 2019 • edited Loading

angular-automatic-lock-bot bot commented Sep 14, 2019

worldsayshi commented Jun 19, 2018 •

edited

Loading

andrei-tatar commented Mar 29, 2019 •

edited

Loading