Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Fix #6128 and #8150 #8154

Closed
wants to merge 2 commits into from
Closed

Conversation

danbarua
Copy link
Contributor

Request Type: bug

How to reproduce: it('should not double quote dates', function() {
$httpBackend.expect('GET', '/url?date=2014-07-15T17:30:00.000Z').respond('');
$http({url: '/url', params: {date:new Date('2014-07-15T17:30:00.000Z')}, method: 'GET'});
})

Currently, datetimes passed into $http.get params are wrapped in double quotes and then escaped giving urls like this: /url?date=%222014-07-15T17:30:00.000Z%22

Component(s): $http

Impact: large

Complexity: small

This issue is related to:

Detailed Description:

Other Comments:

See comments on #6128, #8150

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8154)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@danbarua danbarua changed the title fix($http): fix double-quoted date issue when encoding params Fix #6128 and #8150 Jul 11, 2014
$http was wrapping dates in double quotes leading to query strings like this:
  ?date=%222014-07-07T23:00:00.000Z%22
Instead of calling JSON.stringify, this fix checks to see if a param object
has a toJSON() function and uses that for encoding, falling back to
JSON.stringify as per the previous behaviour.

Closes angular#8150 and angular#6128
@danbarua danbarua added cla: yes and removed cla: no labels Jul 11, 2014
@@ -990,7 +990,12 @@ function $HttpProvider() {

forEach(value, function(v) {
if (isObject(v)) {
v = toJson(v);
if (isDefined(v.toJSON)){
v = v.toJSON();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not right. toJSON doesn't have to return a string value, but we always need a string value, that's why we are doing this serialization.

how about we special case date instead? instead of your isDefined check on line 933 do isDate.

Are there other scenarios besides dates where toJSON doesn't do the right thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @IgorMinar , thanks for taking the time to review this PR.
As per my comments in issue #8150, I had initially taken the approach of special casing date as per below:

forEach(value, function(v) {
              if (v instanceof Date){
                  v = v.toISOString(); //toISOString() only supported in IE8 and above
              }
              else if (isObject(v)) {
                v = toJson(v);
              }
              parts.push(encodeUriQuery(key) + '=' +
                         encodeUriQuery(v));
            });

I then came across a similar older issue #6128 and I then thought that perhaps the .toJSON method would be a more robust approach.

Are there other scenarios besides dates where toJSON doesn't do the right thing?

I don't actually know of any scenarios, so I'd be happy to revert to the isDate approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://stackoverflow.com/questions/20734894/difference-between-tojson-and-json-stringify

That said, the main difference is that toJSON produces a value (a number, boolean, object, ...) that gets converted to a JSON string whereas JSON.stringify always produces a string.

It looks like special casing isDate is the correct approach - I'll submit a new commit.

@IgorMinar IgorMinar self-assigned this Jul 15, 2014
@IgorMinar IgorMinar added this to the 1.3.0-beta.16 milestone Jul 15, 2014
Special case date handling rather than calling toJSON as we always need
a string representation of the object.
@danbarua
Copy link
Contributor Author

@IgorMinar It looks like special casing isDate is the correct approach - PR updated.

IgorMinar pushed a commit that referenced this pull request Jul 16, 2014
This commit special cases date handling rather than calling toJSON as we always need
a string representation of the object.

$http was wrapping dates in double quotes leading to query strings like this:
  ?date=%222014-07-07T23:00:00.000Z%22

Closes #8150
Closes #6128
Closes #8154
@IgorMinar
Copy link
Contributor

landed. thanks

@IgorMinar IgorMinar closed this Jul 16, 2014
IgorMinar pushed a commit that referenced this pull request Jul 16, 2014
This commit special cases date handling rather than calling toJSON as we always need
a string representation of the object.

$http was wrapping dates in double quotes leading to query strings like this:
  ?date=%222014-07-07T23:00:00.000Z%22

Closes #8150
Closes #6128
Closes #8154
ckknight pushed a commit to ckknight/angular.js that referenced this pull request Jul 16, 2014
This commit special cases date handling rather than calling toJSON as we always need
a string representation of the object.

$http was wrapping dates in double quotes leading to query strings like this:
  ?date=%222014-07-07T23:00:00.000Z%22

Closes angular#8150
Closes angular#6128
Closes angular#8154
@danbarua danbarua deleted the fix-url-encoding branch July 21, 2014 09:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants