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

HttpParams.toString() modifies the immutable object #20430

Closed
rfuhrer opened this issue Nov 14, 2017 · 8 comments

Comments

Projects
None yet
8 participants
@rfuhrer
Copy link

commented Nov 14, 2017

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:

  1. Calling toString() modifies the HttpParams object.
  2. 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

  1. 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:
  2. 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:

@DulithaRanatunga

This comment has been minimized.

Copy link

commented Nov 17, 2017

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

@DulithaRanatunga

This comment has been minimized.

Copy link

commented Nov 21, 2017

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

This comment has been minimized.

Copy link

commented Jun 19, 2018

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

This comment has been minimized.

Copy link
Author

commented Sep 6, 2018

@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

This comment has been minimized.

Copy link

commented Oct 24, 2018

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

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

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

This comment has been minimized.

Copy link

commented Feb 28, 2019

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?

JoostK added a commit to JoostK/angular that referenced this issue Feb 28, 2019

fix(common): prevent repeated application of HttpParams mutations
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

This comment has been minimized.

Copy link

commented Mar 29, 2019

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

benlesh added a commit that referenced this issue Apr 23, 2019

fix(common): prevent repeated application of HttpParams mutations (#2…
…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

@benlesh benlesh closed this in 8e8e89a Apr 23, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this issue May 21, 2019

fix(common): prevent repeated application of HttpParams mutations (an…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.