Skip to content
This repository was 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

Fix #6128 and #8150 #8154

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3366b35
fix($http): fix double-quoted date issue when encoding params
danbarua Jul 11, 2014
17931a5
fix($http): fix double-quoted date issue when encoding params
danbarua Jul 15, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
'use strict';
'use strict';

/**
* Parse headers into key value object
Expand Down Expand Up @@ -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.

}
else {
v = toJson(v);
}
}
parts.push(encodeUriQuery(key) + '=' +
encodeUriQuery(v));
Expand Down
5 changes: 5 additions & 0 deletions test/ng/httpSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,11 @@ describe('$http', function() {
$httpBackend.expect('GET', '/url').respond('');
$http({url: '/url', params: {}, method: 'GET'});
});

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'});
});
});


Expand Down