angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.81k stars 27.5k forks source link

angular-mocks $httpBackend always return the same object instance when $http cache is enabled #13762

Open mjeanroy opened 8 years ago

mjeanroy commented 8 years ago

Suppose this function using $httpBackend :

$scope.find = function() {
  $http.get('/resource', { cache: true }).then(function(response) {
    var data = response.data;
    var parts = data.date.split('-').map(Number);
    data.date = Date.UTC(parts[0], parts[1] - 1, parts[0]);

    $scope.result = data;
  });
}

Note that:

Suppose this simple test :

it('should compute UTC date', function() {
    $httpBackend.whenGET('/resource').respond({
      date: '2016-01-01'
    });

    $scope.find();
    $httpBackend.flush();
    expect(angular.isNumber($scope.result.date)).toBe(true);

    // make a second call, it will fail
    $scope.find();
    $httpBackend.flush();
});

Here is the reason :

I can fix this in my test by using angular.toJson to specify that the response is not an object but a json string, but the problem could be solved in angular-mocks :

+ function _toJson(data) {
+   return angular.isObject(data) ? angular.toJson(data) : data;
+ }

function createResponse(status, data, headers, statusText) {
    if (angular.isFunction(status)) return status;

    return function() {
      return angular.isNumber(status)
-          ? [status, data, headers, statusText]
-          : [200, status, data, headers];
+          ? [status, _toJson(data), headers, statusText]
+          : [200, _toJson(status), data, headers];
    };

What do you think ? Should I fix this in my test or a PR is welcome ?

Note that this is a problem with angular-mocks, not with $http since the object stored in the $http cache is the server response (which is a string in a browser, deserialized in the defaultHttpResponseTransform function).

Narretz commented 8 years ago

I see the problem, but I believe that the cause is not on angular-mocks. In fact, your patch doesn'T solve the problem. The issue is that the httpBackend isn't hit, when a cached request is made: the cached request is returned by $http itself. The problem here is that you are directly manipulating the data object and replacing the date string with a number; by that you are actually modifying the cached data object as well. So if a change should be made, than that the $http cache copies the response object before it is put into the cache. You can see this when the issue is fixed: the second $httpBacked.flush will throw, because no request has been made.

Quick fix is for you to make a copy of the data before modifying it in your success handler.

Another solution is for you to use a transformResponse function on your request, which removes the logic from your controller completely, and also doesn't run into that problem.

mjeanroy commented 8 years ago

@Narretz Thanks for your response.

I am not sure that the fact that the httpBackend isn't hit is a real problem:

  1. When a cached request is made, the server response (a json string) is stored in the cache, then it is deserialized as a javascript object.
  2. The next call use the cache (the json string), then deserialized again the response producing a new instance for the success handler.

With angular-mocks, it is a bit different:

  1. The first call store the response specified with $httpBackend in the cache (this is a javascript object, not a json string) and the response (the object in the cache) is given to the success handler.
  2. The next call use the cache (still the same object), and the same object is given to the succes handler.

Note that:

it('should compute UTC date', function() {
    $httpBackend.whenGET('/resource').respond(angular.toJson({
      date: '2016-01-01'
    }));

    $scope.find();
    $httpBackend.flush();
    expect(angular.isNumber($scope.result.date)).toBe(true);

    // Now, because I used angular.toJson, it works
    $scope.find();
    $httpBackend.flush();
});
Narretz commented 8 years ago

Strange, I wasn't able to fix the problem with the patch you provided. If you can show that this fixes it in a PR, I'll take a look.

Narretz commented 8 years ago

Ah, I see what you mean. A real http request is cached as a string, but because angular mocks returns an object, you modify the object in your success handler. And your fix didn't work because I probably applied _toJson to data in the second line, which is wrong, because status = data at that point. Damn that code is ugly (not your patch)! ;)

So, yeah, if you want to knock up a PR, I'm happy to look at it. If you could clean up the part where status is sometimes data, too, that would be great.

I wonder though if this might be a breaking change for some people.

mjeanroy commented 8 years ago

@Narretz It might break changes if object contains some special properties deleted by angular.toJson (key prefixed with$$ is an example) but we can handle it by using JSON.stringify instead of angular.toJson.