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

[WIP] feat($httpUrlParams): introduce new service abstracting params serialization #11238

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@pkozlowski-opensource
Member

pkozlowski-opensource commented Mar 4, 2015

@petebacondarwin @caitp here is another stab at having configurable strategies for $http request params serialization. This is still WIP (missing tests and doc updates) but wanted to check this one with you before putting more work into this approach.

Here is the basic idea:

  • we introduce a new service (and the associated provider), $httpUrlParams, that is responsible for serializing $http's request params
  • this new service has essentially one method: serialize = function(params, mode) where mode is a flag / key that people can use to trigger different serialization strategies (ex. per domain)
  • the new provider allows you to configure a default mode (ships with the one named traditional) and register mode handlers (a function)
  • people, while doing $http calls can trigger different modes by passing paramsMode in the config object, ex.:
$http.get(myUrl, {
    params: {...},
    paramsMode: 'jquery'
}).then(...);

It would be totally awesome if we could ship it with 1.4, so any input would be much appreciated.

@googlebot googlebot added the cla: yes label Mar 4, 2015

var HttpUrlParams = {};
HttpUrlParams.serialize = function(params, mode) {
return paramSerializers[lowercase(mode) || provider.defaultMode](params);

This comment has been minimized.

@Narretz

Narretz Mar 4, 2015

Contributor

Should we throw if a mode is not available? I think we should.

@Narretz

Narretz Mar 4, 2015

Contributor

Should we throw if a mode is not available? I think we should.

This comment has been minimized.

@gkalpak

gkalpak Mar 5, 2015

Member

If we decide to throw, it would be better to throw a more meaningful error (instead of undefined is not a function).
I would also consider logging the incident and falling back to the default mode.

BTW, you are using lowercase here, but not when registering. They should be consistent (and probably documented).

@gkalpak

gkalpak Mar 5, 2015

Member

If we decide to throw, it would be better to throw a more meaningful error (instead of undefined is not a function).
I would also consider logging the incident and falling back to the default mode.

BTW, you are using lowercase here, but not when registering. They should be consistent (and probably documented).

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

We should ditch the lowercase altogether. People should be able to specify the correct capitalization.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

We should ditch the lowercase altogether. People should be able to specify the correct capitalization.

if (parts.length > 0) {
url += ((url.indexOf('?') == -1) ? '?' : '&') + parts.join('&');
function buildUrl(url, params, mode) {
params = $httpUrlParams.serialize(params, mode);

This comment has been minimized.

@Narretz

Narretz Mar 4, 2015

Contributor

For maxium flexibility, how about we allow mode to be a function? So you could have an inline params converter in your http config.

@Narretz

Narretz Mar 4, 2015

Contributor

For maxium flexibility, how about we allow mode to be a function? So you could have an inline params converter in your http config.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 4, 2015

Member

This is a great concept to be able to specify this mode in the $http requests.

My first thought is does this really need to be a separate service? It seems like it would quite easily be added to the $httpProvider/$http service. What is the benefit of a new service?

Member

petebacondarwin commented Mar 4, 2015

This is a great concept to be able to specify this mode in the $http requests.

My first thought is does this really need to be a separate service? It seems like it would quite easily be added to the $httpProvider/$http service. What is the benefit of a new service?

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 4, 2015

Member

@petebacondarwin you need a separate service due to #3311

Member

pkozlowski-opensource commented Mar 4, 2015

@petebacondarwin you need a separate service due to #3311

function buildUrl(url, params, mode) {
params = $httpUrlParams.serialize(params, mode);
if (params) {
url += ((url.indexOf('?') == -1) ? '?' : '&') + params;

This comment has been minimized.

@gkalpak

gkalpak Mar 5, 2015

Member

While you are touching this, === would be nice :)

@gkalpak

gkalpak Mar 5, 2015

Member

While you are touching this, === would be nice :)

function serializeParams(params, addArrayMarker) {
var parts = [];
if (!params) return '';

This comment has been minimized.

@gkalpak

gkalpak Mar 5, 2015

Member

Pretty minor, but why not move this to the top (to avoid unused array creation) ?

@gkalpak

gkalpak Mar 5, 2015

Member

Pretty minor, but why not move this to the top (to avoid unused array creation) ?

if (isObject(v)) {
v = isDate(v) ? v.toISOString() : toJson(v);
}
parts.push(encodeUriQuery(key) + (addArrayMarker ? '[]' : '') + '=' + encodeUriQuery(v));

This comment has been minimized.

@gkalpak

gkalpak Mar 5, 2015

Member

I feel like deciding whether to add an array-marker or not, should somehow be more "clever" (e.g. take into account if value was initially an array or something).
With the current implementation, it seems to be a pain to add a marker to certain params.

@gkalpak

gkalpak Mar 5, 2015

Member

I feel like deciding whether to add an array-marker or not, should somehow be more "clever" (e.g. take into account if value was initially an array or something).
With the current implementation, it seems to be a pain to add a marker to certain params.

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

I agree we should only be adding an array marker to params that are actually arrays.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

I agree we should only be adding an array marker to params that are actually arrays.

return serializeParams(params, false);
});
this.registerSerializer('jquery', function(params) {

This comment has been minimized.

@gkalpak

gkalpak Mar 5, 2015

Member

Isn't this the same as the default ?

@gkalpak

gkalpak Mar 5, 2015

Member

Isn't this the same as the default ?

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Should be return serializeParams(params, true)?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Should be return serializeParams(params, true)?

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 5, 2015

Member

@petebacondarwin @Narretz @gkalpak thnx for the detailed review of the code - all the comments are very valid and for sure I'm going to incorporate those into the final version.

But what I need mostly from you now is a OK / KO when it comes to concepts and APIs exposed to the users. Are there any other concerns apart from @petebacondarwin question about having this logic in a separate service? Once again, the need for a separate service was driven by those 2 issues:

If you think that the current approach / APIs are reasonable I would add tests and address all the comments so we can get it into 1.4. But if you've got any concerns regarding the approach it is time to speak up now :-)

Member

pkozlowski-opensource commented Mar 5, 2015

@petebacondarwin @Narretz @gkalpak thnx for the detailed review of the code - all the comments are very valid and for sure I'm going to incorporate those into the final version.

But what I need mostly from you now is a OK / KO when it comes to concepts and APIs exposed to the users. Are there any other concerns apart from @petebacondarwin question about having this logic in a separate service? Once again, the need for a separate service was driven by those 2 issues:

If you think that the current approach / APIs are reasonable I would add tests and address all the comments so we can get it into 1.4. But if you've got any concerns regarding the approach it is time to speak up now :-)

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

@pkozlowski-opensource - OK so here is my take on it. This should by no means be taken as the right way, it is just an opinion...

I would steer away from using strings and lookups like this. It adds an additional opportunity for bugs to be disguised. Instead I would follow a similar design pattern as Http Interceptors, where a paramSerializer is just a service that implements an interface. Anyone is free to create their own version of this that will do the serialization in their own way:

mod.factory('myParamSerializer', function() {
  return function myParamSerializer(params) {
    ...
  };
});

Out of the box we would provide two standard versions: $traditionalParamSerializer and $jqueryParamSerializer.

Then you just pass the version you want to $http. You could do this explicitly in a request:

myMod.controller('MyCtrl', function($scope, $http) {
  $http.get(myUrl, {
      params: {...},
      paramSerializer: '$jqueryParamSerializer'
  }).then(...);
});

or you could set a default in the $httpProvider:

myMod.config(function($httpProvider) {
  $httpProvider.defaultParamSerializer('$jqueryParamSerializer');
});

Following the Http Interceptor design, you could also define the serializer "inline" instead of as a service.

Member

petebacondarwin commented Mar 5, 2015

@pkozlowski-opensource - OK so here is my take on it. This should by no means be taken as the right way, it is just an opinion...

I would steer away from using strings and lookups like this. It adds an additional opportunity for bugs to be disguised. Instead I would follow a similar design pattern as Http Interceptors, where a paramSerializer is just a service that implements an interface. Anyone is free to create their own version of this that will do the serialization in their own way:

mod.factory('myParamSerializer', function() {
  return function myParamSerializer(params) {
    ...
  };
});

Out of the box we would provide two standard versions: $traditionalParamSerializer and $jqueryParamSerializer.

Then you just pass the version you want to $http. You could do this explicitly in a request:

myMod.controller('MyCtrl', function($scope, $http) {
  $http.get(myUrl, {
      params: {...},
      paramSerializer: '$jqueryParamSerializer'
  }).then(...);
});

or you could set a default in the $httpProvider:

myMod.config(function($httpProvider) {
  $httpProvider.defaultParamSerializer('$jqueryParamSerializer');
});

Following the Http Interceptor design, you could also define the serializer "inline" instead of as a service.

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 6, 2015

Member

@petebacondarwin I like what you are saying here and my initial proposal was kind of going in this direction. But I've changed this, mostly based on @caitp input, as I believe that:

  • injecting serializers is cleaner but more troublesome for users (this might be just my non-objective perception)
  • setting defaults on the $httpProvider kills all the testing-related benefits I wanted to have (see #3311 and #7429)

@petebacondarwin @caitp or anyone interested - could we try to have a hangout on this topic somewhere early next week to clarify / agree upon the design? I would really love to ship this thing with 1.4!

Member

pkozlowski-opensource commented Mar 6, 2015

@petebacondarwin I like what you are saying here and my initial proposal was kind of going in this direction. But I've changed this, mostly based on @caitp input, as I believe that:

  • injecting serializers is cleaner but more troublesome for users (this might be just my non-objective perception)
  • setting defaults on the $httpProvider kills all the testing-related benefits I wanted to have (see #3311 and #7429)

@petebacondarwin @caitp or anyone interested - could we try to have a hangout on this topic somewhere early next week to clarify / agree upon the design? I would really love to ship this thing with 1.4!

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Mar 6, 2015

Contributor

i can do a hangout, but my 2c is still that it should be kept as simple as possible. $http(url, { params: ..., legacyParams: true }) or something

Contributor

caitp commented Mar 6, 2015

i can do a hangout, but my 2c is still that it should be kept as simple as possible. $http(url, { params: ..., legacyParams: true }) or something

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 6, 2015

Member

OK just to clarify...

There could be a getter on the $http service that returns you the current default serialised for use in tests.

My idea would allow the name of the serializer to be passed instead of an instance avoiding the need to inject it

Member

petebacondarwin commented Mar 6, 2015

OK just to clarify...

There could be a getter on the $http service that returns you the current default serialised for use in tests.

My idea would allow the name of the serializer to be passed instead of an instance avoiding the need to inject it

@jmendiara

This comment has been minimized.

Show comment
Hide comment
@jmendiara

jmendiara Mar 20, 2015

Contributor

I hope is not too late for debate...
@pkozlowski-opensource and mates, here you have #11386 another approach that solves the same issues and addresses @petebacondarwin concerns, giving more control to userland and keeping core simpler, IMHO.

Contributor

jmendiara commented Mar 20, 2015

I hope is not too late for debate...
@pkozlowski-opensource and mates, here you have #11386 another approach that solves the same issues and addresses @petebacondarwin concerns, giving more control to userland and keeping core simpler, IMHO.

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 24, 2015

Member

OK, so we start to have different proposals from @petebacondarwin, @caitp and @jmendiara now and if we want to land something in 1.4 we need to chose now :-) To facilitate the decision process here is once again a list of problems we are trying to tackle with this change:

  1. support most common serilaization schema (the current one and jQuery's one) - #3740
  2. ability to specify a default serialization schema (per app)
  3. ability to specify a per-request serialization schema
  4. allow people to have completely custom serialization schemas - #7429 and #9224
  5. use serialization functionality in unit tests - #3311

Now, with my proposal here is how one would tackle each use-case.

For (1) and (2):

angular.module('app', [], function($HttpUrlParamsProvider) {
    $HttpUrlParamsProvider.defaultMode = 'jquery';
}).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

would result in http://host.com?foo[]=bar&foo[]=baz for each and every request.

For (3) people could do:

angular.module('app', []).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, paramsMode: 'jquery'})
});

To tackle (4) one would do:

angular.module('app', [], function($HttpUrlParamsProvider) {
    $HttpUrlParamsProvider.registerSerializer('mycustom', function(params) {
        return //whatever crazy serialization people want to have
    });
    $HttpUrlParamsProvider.defaultMode = 'mycustom';
}).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

and finally, one could inject $httpUrlParams into test to express $httpBackend expectations both for the default serialization mode:

$httpBackend.expectGET('http://host.com?' + $httpUrlParams.serialize( {foo: ['bar', 'baz']}));

as well as for the per-request basis:

$httpBackend.expectGET('http://host.com?' + $httpUrlParams.serialize( {foo: ['bar', 'baz']}, 'mycustom'));

Now, what I really dislike here is relaying on string-based names that are really easy to get wrong... At the same time I would appreciate solutions where (4) and (5) are properly handled.

From what I understand @caitp is proposing:

angular.module('app', []).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, serialiseAsJQuery=true})
});

which is easy to use and tackles (1) as well as (2) but doesn't help with the rest, as far as I can see.

@petebacondarwin @jmendiara how the code would look like with your respective proposals?

Member

pkozlowski-opensource commented Mar 24, 2015

OK, so we start to have different proposals from @petebacondarwin, @caitp and @jmendiara now and if we want to land something in 1.4 we need to chose now :-) To facilitate the decision process here is once again a list of problems we are trying to tackle with this change:

  1. support most common serilaization schema (the current one and jQuery's one) - #3740
  2. ability to specify a default serialization schema (per app)
  3. ability to specify a per-request serialization schema
  4. allow people to have completely custom serialization schemas - #7429 and #9224
  5. use serialization functionality in unit tests - #3311

Now, with my proposal here is how one would tackle each use-case.

For (1) and (2):

angular.module('app', [], function($HttpUrlParamsProvider) {
    $HttpUrlParamsProvider.defaultMode = 'jquery';
}).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

would result in http://host.com?foo[]=bar&foo[]=baz for each and every request.

For (3) people could do:

angular.module('app', []).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, paramsMode: 'jquery'})
});

To tackle (4) one would do:

angular.module('app', [], function($HttpUrlParamsProvider) {
    $HttpUrlParamsProvider.registerSerializer('mycustom', function(params) {
        return //whatever crazy serialization people want to have
    });
    $HttpUrlParamsProvider.defaultMode = 'mycustom';
}).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

and finally, one could inject $httpUrlParams into test to express $httpBackend expectations both for the default serialization mode:

$httpBackend.expectGET('http://host.com?' + $httpUrlParams.serialize( {foo: ['bar', 'baz']}));

as well as for the per-request basis:

$httpBackend.expectGET('http://host.com?' + $httpUrlParams.serialize( {foo: ['bar', 'baz']}, 'mycustom'));

Now, what I really dislike here is relaying on string-based names that are really easy to get wrong... At the same time I would appreciate solutions where (4) and (5) are properly handled.

From what I understand @caitp is proposing:

angular.module('app', []).controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, serialiseAsJQuery=true})
});

which is easy to use and tackles (1) as well as (2) but doesn't help with the rest, as far as I can see.

@petebacondarwin @jmendiara how the code would look like with your respective proposals?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 24, 2015

Member

My suggestion is not that much different except it removes the need for this registration service. We would provide two basic serializers (as angular services) out of the box:

  • $legacyParamSerializer (default)
  • $jqueryParamSerializer

The $httpProvider provider would expose a getter/setter $httpProvider.defaultParamSerializer(value)
and the $http service would expose only a getter $http.defaultParamSerializer().

Custom serializers would simply be angular services, see customParamSerializer below.

Below are the demonstrations of how this would work for each use case.

1/2) Specify the serializer at the application level:

angular.module('app', [])

.config(function($httpProvider) {
  $httpProvider.defaultParamSerializer('$jqueryParamSerializer');
})

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}});
});

3) Specify the serializer at the request level:

angular.module('app', [])

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, paramSerializer: '$jqueryParamSerializer'});
});

or more explicitly

.controller('MyCtrl', function($http, $jqueryParamSerializer) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, paramSerializer: $jqueryParamSerializer});
});

4) Provide a completely custom serializer:

angular.module('app', [])

.factory('myCustomParamSerializer', function() {
  return function myCustomParamSerializer(params) {
    return // Whatever crazy serialization people want to have
  };
})

.config(function($httpProvider) {
  $httpProvider.defaultParamSerializer('myCustomParamSerializer');
})

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

5) Enable serialization during testing:

it('should respond to a request with default serialization', inject(function($httpBackend, $http) {
  $httpBackend.expectGET('http://host.com?' + $http.defaultParamSerializer({foo: ['bar', 'baz']}));
}));
it('should respond to a request with a specific serialization', inject(function($httpBackend, myCustomParamSerializer) {
  $httpBackend.expectGET('http://host.com?' + myCustomParamSerializer({foo: ['bar', 'baz']}));
}));
Member

petebacondarwin commented Mar 24, 2015

My suggestion is not that much different except it removes the need for this registration service. We would provide two basic serializers (as angular services) out of the box:

  • $legacyParamSerializer (default)
  • $jqueryParamSerializer

The $httpProvider provider would expose a getter/setter $httpProvider.defaultParamSerializer(value)
and the $http service would expose only a getter $http.defaultParamSerializer().

Custom serializers would simply be angular services, see customParamSerializer below.

Below are the demonstrations of how this would work for each use case.

1/2) Specify the serializer at the application level:

angular.module('app', [])

.config(function($httpProvider) {
  $httpProvider.defaultParamSerializer('$jqueryParamSerializer');
})

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}});
});

3) Specify the serializer at the request level:

angular.module('app', [])

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, paramSerializer: '$jqueryParamSerializer'});
});

or more explicitly

.controller('MyCtrl', function($http, $jqueryParamSerializer) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, paramSerializer: $jqueryParamSerializer});
});

4) Provide a completely custom serializer:

angular.module('app', [])

.factory('myCustomParamSerializer', function() {
  return function myCustomParamSerializer(params) {
    return // Whatever crazy serialization people want to have
  };
})

.config(function($httpProvider) {
  $httpProvider.defaultParamSerializer('myCustomParamSerializer');
})

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

5) Enable serialization during testing:

it('should respond to a request with default serialization', inject(function($httpBackend, $http) {
  $httpBackend.expectGET('http://host.com?' + $http.defaultParamSerializer({foo: ['bar', 'baz']}));
}));
it('should respond to a request with a specific serialization', inject(function($httpBackend, myCustomParamSerializer) {
  $httpBackend.expectGET('http://host.com?' + myCustomParamSerializer({foo: ['bar', 'baz']}));
}));
@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 24, 2015

Member

@petebacondarwin oh, I see, you would expose a custom serializer on the $http itself - OK - I think I like it.

@jmendiara how is your proposal different from what we are discussing here?

Member

pkozlowski-opensource commented Mar 24, 2015

@petebacondarwin oh, I see, you would expose a custom serializer on the $http itself - OK - I think I like it.

@jmendiara how is your proposal different from what we are discussing here?

@jmendiara

This comment has been minimized.

Show comment
Hide comment
@jmendiara

jmendiara Mar 24, 2015

Contributor

@pkozlowski-opensource It's more like @petebacondarwin approach, but clearly separates param serialization from URL Building (which includes requestParameters concatenation with ; or & #9224 and encoding #1388).

My WIP #11386 didn't ship jquery support in order to keep core small and simple (should core ship other fws functionalities? read @caitp comments ) and let users to implement this on their own apps (as an external dependency) or maybe provide an ngJquerySerialize optional module.

My approach registers an optional core provider that helps on urlBuilding, but is not needed at all, just sugar to developers to hack easily into the functionality. Although not in the WIP the jquery support can also be implemented

AssumingTaking @petebacondarwin snippets as starting point...

1/2) Specify the serializer at the application level:

angular.module('app', [])

.config(function($httpProvider) {
  $httpProvider.defaultURLBuilder('$jqueryURLBuilder');
 //OR   $httpProvider.defaults.buildUrl = '$jqueryURLBuilder';
// the last one is just like specifying the default Cache for request, keeping the same paradigm 
})

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}});
});

3) Specify the serializer at the request level:

angular.module('app', [])

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, buildUrl: '$jqueryURLBuilder'});
});

or more explicitly

.controller('MyCtrl', function($http, $jqueryURLBuilder) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, buildUrl: $jqueryURLBuilder});
});

4) Provide a completely custom serializer:
This is where the approached differ most:

//petebacondarwin use case is supported
angular.module('app', [])
.factory('myCustomUrlBuilder', function() {
  return function myCustomUrlBuilder(url, params) {
    return // Whatever crazy serialization people want to have
  };
})
.config(function($httpProvider) {
  $httpProvider.defaultURLBuilder('myCustomUrlBuilder');
})
.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

or using my optional-to-implement-in-the-core $httpUrlBuilderFactory core helper provider to implement url builders. The URL Builder factory creates encoded urls concatenated with & (the current angular default behavior)

angular.module('app')
//Create a function that says how an object should be converted to a `List of {key, value}` strings
.factory('arraySerializer', function() {
  return function arraySerializer(params, addKeyValue) {
    angular.forEach(params, function(value, key) {
      if(angular.isArray(value)) {
        angular.forEach(value, function(arrValue) {
          addKeyValue(key+ '[]', String(arrValue));
        });  
      } else {
         addKeyValue(key, String(value));
      }
    });
  }
})

//We rehuse the common way of building URLs
.factory('arrayStandardBuildUrl', function($httpUrlBuilderFactory, arraySerializer) {
  return $httpUrlBuilderFactory(arraySerializer);
})

//we rehuse the arraySerialization, but making other kind URLs
// semicolonURLs returns a function(url, params) that returns urls concatenated with ;
// see https://github.com/jmendiara/angular.js/blob/http_params_serialization/src/ng/http.js#L1230
.factory('arrayCustomBuildUrl', function(arraySerializer, semicolonURLs) {
  return semicolonURLs(arraySerializer);
})

.controller('MyCtrl', function($http) {
  $http.get('http://host.com', {params: {id: 5, foo: ['bar', 'baz']},
    buildUrl: 'arrayStandardBuildUrl'
  }); // GET http://host.com?id=5&foo[]=bar&foo[]=baz

  $http.get('http://host.com', {params: {id: 5, foo: ['bar', 'baz']},
    buildUrl: 'arrayCustomBuildUrl'
  }); // GET http://host.com?id=5;foo[]=bar;foo[]=baz
})

5) Enable serialization during testing:

it('should respond to a request with default serialization', inject(function($httpBackend, $http) {
  var url = 'http://host.com', params = {foo: ['bar', 'baz']};

  $httpBackend.expectGET($http.defaultURLBuilder(url, params));
  $http.get(url, {params: params});
}));
it('should respond to a request with a specific serialization', inject(function($httpBackend, $http, myCustomUrlBuilder) {
  var url = 'http://host.com', params = {foo: ['bar', 'baz']};

  $httpBackend.expectGET(myCustomUrlBuilder(url, params));
  $http.get(url, {params: params, buildUrl: myCustomUrlBuilder });
}));

As you can see $httpUrlBuilderFactory is optional, but helps developers to implement serializers

Why complete URL building and not only param serialization??
My approach covers the following use cases:

  • As a user, I want to delete $http cache entries: Currently, $http cache has the complete URL as a key. If the URL is generated in core clausures, like your approaches, I cannot be truly confident about the url it was used. Why should a user want to delete cache entries? Some grumpy backends return statusCode === 200 and an error payload {success: false, data: ...} This entry should not be in the cache, but angular core assumes a good response. Users can code a response interceptor, execute config.cache.remove(config.buildUrl(config.url, config.params)) to obtain exactly the cache key and remove from the cache
  • As a user, I want to have a serialized version of my params as jQuery does, without encoding, in order to make some operations with them, like oauth1 signing: Separating URL building (encoding + concatenation) from serialization lead to reuse the serialization and implement the custom url building.

What i dislike most of my approach
The signature function for a serializer, using false callbacks function like addKeyValue

What i like most of my approach
Completely freedom and user power to hack into $http. The important thing is to allow users to buildUrl, not the sugar provided by $httpUrlBuilderFactory, which can be removed

Contributor

jmendiara commented Mar 24, 2015

@pkozlowski-opensource It's more like @petebacondarwin approach, but clearly separates param serialization from URL Building (which includes requestParameters concatenation with ; or & #9224 and encoding #1388).

My WIP #11386 didn't ship jquery support in order to keep core small and simple (should core ship other fws functionalities? read @caitp comments ) and let users to implement this on their own apps (as an external dependency) or maybe provide an ngJquerySerialize optional module.

My approach registers an optional core provider that helps on urlBuilding, but is not needed at all, just sugar to developers to hack easily into the functionality. Although not in the WIP the jquery support can also be implemented

AssumingTaking @petebacondarwin snippets as starting point...

1/2) Specify the serializer at the application level:

angular.module('app', [])

.config(function($httpProvider) {
  $httpProvider.defaultURLBuilder('$jqueryURLBuilder');
 //OR   $httpProvider.defaults.buildUrl = '$jqueryURLBuilder';
// the last one is just like specifying the default Cache for request, keeping the same paradigm 
})

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}});
});

3) Specify the serializer at the request level:

angular.module('app', [])

.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, buildUrl: '$jqueryURLBuilder'});
});

or more explicitly

.controller('MyCtrl', function($http, $jqueryURLBuilder) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}, buildUrl: $jqueryURLBuilder});
});

4) Provide a completely custom serializer:
This is where the approached differ most:

//petebacondarwin use case is supported
angular.module('app', [])
.factory('myCustomUrlBuilder', function() {
  return function myCustomUrlBuilder(url, params) {
    return // Whatever crazy serialization people want to have
  };
})
.config(function($httpProvider) {
  $httpProvider.defaultURLBuilder('myCustomUrlBuilder');
})
.controller('MyCtrl', function($http) {
   $http.get('http://host.com', {params: {foo: ['bar', 'baz']}})
});

or using my optional-to-implement-in-the-core $httpUrlBuilderFactory core helper provider to implement url builders. The URL Builder factory creates encoded urls concatenated with & (the current angular default behavior)

angular.module('app')
//Create a function that says how an object should be converted to a `List of {key, value}` strings
.factory('arraySerializer', function() {
  return function arraySerializer(params, addKeyValue) {
    angular.forEach(params, function(value, key) {
      if(angular.isArray(value)) {
        angular.forEach(value, function(arrValue) {
          addKeyValue(key+ '[]', String(arrValue));
        });  
      } else {
         addKeyValue(key, String(value));
      }
    });
  }
})

//We rehuse the common way of building URLs
.factory('arrayStandardBuildUrl', function($httpUrlBuilderFactory, arraySerializer) {
  return $httpUrlBuilderFactory(arraySerializer);
})

//we rehuse the arraySerialization, but making other kind URLs
// semicolonURLs returns a function(url, params) that returns urls concatenated with ;
// see https://github.com/jmendiara/angular.js/blob/http_params_serialization/src/ng/http.js#L1230
.factory('arrayCustomBuildUrl', function(arraySerializer, semicolonURLs) {
  return semicolonURLs(arraySerializer);
})

.controller('MyCtrl', function($http) {
  $http.get('http://host.com', {params: {id: 5, foo: ['bar', 'baz']},
    buildUrl: 'arrayStandardBuildUrl'
  }); // GET http://host.com?id=5&foo[]=bar&foo[]=baz

  $http.get('http://host.com', {params: {id: 5, foo: ['bar', 'baz']},
    buildUrl: 'arrayCustomBuildUrl'
  }); // GET http://host.com?id=5;foo[]=bar;foo[]=baz
})

5) Enable serialization during testing:

it('should respond to a request with default serialization', inject(function($httpBackend, $http) {
  var url = 'http://host.com', params = {foo: ['bar', 'baz']};

  $httpBackend.expectGET($http.defaultURLBuilder(url, params));
  $http.get(url, {params: params});
}));
it('should respond to a request with a specific serialization', inject(function($httpBackend, $http, myCustomUrlBuilder) {
  var url = 'http://host.com', params = {foo: ['bar', 'baz']};

  $httpBackend.expectGET(myCustomUrlBuilder(url, params));
  $http.get(url, {params: params, buildUrl: myCustomUrlBuilder });
}));

As you can see $httpUrlBuilderFactory is optional, but helps developers to implement serializers

Why complete URL building and not only param serialization??
My approach covers the following use cases:

  • As a user, I want to delete $http cache entries: Currently, $http cache has the complete URL as a key. If the URL is generated in core clausures, like your approaches, I cannot be truly confident about the url it was used. Why should a user want to delete cache entries? Some grumpy backends return statusCode === 200 and an error payload {success: false, data: ...} This entry should not be in the cache, but angular core assumes a good response. Users can code a response interceptor, execute config.cache.remove(config.buildUrl(config.url, config.params)) to obtain exactly the cache key and remove from the cache
  • As a user, I want to have a serialized version of my params as jQuery does, without encoding, in order to make some operations with them, like oauth1 signing: Separating URL building (encoding + concatenation) from serialization lead to reuse the serialization and implement the custom url building.

What i dislike most of my approach
The signature function for a serializer, using false callbacks function like addKeyValue

What i like most of my approach
Completely freedom and user power to hack into $http. The important thing is to allow users to buildUrl, not the sugar provided by $httpUrlBuilderFactory, which can be removed

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 24, 2015

Member

I really like the idea of using the $httpProvider.defaults object instead of my idea of a getter/setter since this is much more in keeping with the current API design.

Member

petebacondarwin commented Mar 24, 2015

I really like the idea of using the $httpProvider.defaults object instead of my idea of a getter/setter since this is much more in keeping with the current API design.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 24, 2015

Member

I can see the benefit in providing a way to plugin in to the whole HTTP url building process but I think that this buildURL API not the best way... I would prefer to see the two processes completely decoupled, rather than the url building "owning" the parameter serialization.

In the meantime, I wonder if we could solve the given use cases without resorting to completely opening up the url building:

  • For the cache clearing, we could easily add the fully generated URL to the request/response objects that are available to the interceptors.
  • For the oauth processing, could a custom serializer not do this as part of processing the params?
Member

petebacondarwin commented Mar 24, 2015

I can see the benefit in providing a way to plugin in to the whole HTTP url building process but I think that this buildURL API not the best way... I would prefer to see the two processes completely decoupled, rather than the url building "owning" the parameter serialization.

In the meantime, I wonder if we could solve the given use cases without resorting to completely opening up the url building:

  • For the cache clearing, we could easily add the fully generated URL to the request/response objects that are available to the interceptors.
  • For the oauth processing, could a custom serializer not do this as part of processing the params?
@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 24, 2015

Member

I would very much like to avoid going into cache-related issues as part of what we are doing here... IMO the current cache implementation for $http leaves much to be desired, especially in terms of what people can control (ex.: when things are cached, under which key etc.).

In short: cache has its own set of issues that we should tackle separately, IMO.

Member

pkozlowski-opensource commented Mar 24, 2015

I would very much like to avoid going into cache-related issues as part of what we are doing here... IMO the current cache implementation for $http leaves much to be desired, especially in terms of what people can control (ex.: when things are cached, under which key etc.).

In short: cache has its own set of issues that we should tackle separately, IMO.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 24, 2015

Member

I agree that it would be best if we could keep this simple.

I believe if we attach the generated URL, as returned from the private function buildUrl(), to the request config object, as config.fullUrl say, then we can use this property throughout the rest of the sendReq function in the $http service instead of the url variable.

This would allow a request interceptor to have access to this URL but also have the opportunity to completely rebuild this URL in any way they want, which is effectively the requirement of @jmendiara.

What do you think?

Member

petebacondarwin commented Mar 24, 2015

I agree that it would be best if we could keep this simple.

I believe if we attach the generated URL, as returned from the private function buildUrl(), to the request config object, as config.fullUrl say, then we can use this property throughout the rest of the sendReq function in the $http service instead of the url variable.

This would allow a request interceptor to have access to this URL but also have the opportunity to completely rebuild this URL in any way they want, which is effectively the requirement of @jmendiara.

What do you think?

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 24, 2015

Member

I don't think I feel comfortable with using config object as a container for temporary variables...

Member

pkozlowski-opensource commented Mar 24, 2015

I don't think I feel comfortable with using config object as a container for temporary variables...

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 25, 2015

Member

That's fine. It doesn't relate directly to this PR anyway and could always be added later if it was wanted. I think the response headers would also already contain this info, right?

Member

petebacondarwin commented Mar 25, 2015

That's fine. It doesn't relate directly to this PR anyway and could always be added later if it was wanted. I think the response headers would also already contain this info, right?

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 25, 2015

Member

Probably. In any case I don't want to go into cache-related problems just when we should be shipping this one :-)

For now I'm leaning towards @petebacondarwin proposal - if there are no objections I can re-submit my PR with this idea implemented.

Member

pkozlowski-opensource commented Mar 25, 2015

Probably. In any case I don't want to go into cache-related problems just when we should be shipping this one :-)

For now I'm leaning towards @petebacondarwin proposal - if there are no objections I can re-submit my PR with this idea implemented.

@Narretz

This comment has been minimized.

Show comment
Hide comment
@Narretz

Narretz Mar 25, 2015

Contributor

I think Pete's proposal is the best with regards to consistency with the rest of Angular.
One thing, will this new serializer also work with $resource?

Contributor

Narretz commented Mar 25, 2015

I think Pete's proposal is the best with regards to consistency with the rest of Angular.
One thing, will this new serializer also work with $resource?

@jmendiara

This comment has been minimized.

Show comment
Hide comment
@jmendiara

jmendiara Mar 25, 2015

Contributor

@petebacondarwin

For the oauth processing, could a custom serializer not do this as part of processing the params?

oauth1 signature needs params + url + method

If we have transformRequest and transformResponse functions, introducing a function buildUrl(config) function/service is very well aligned with $http() and $httpProvider.defaults API

I'm talking now about introducing the ability to buildUrl per request and in defaults, not my proposed $httpUrlBuilderFactory, as it is only sugar

Small code change, no new services/providers in core, fully customization by user and more use cases covered.

BTW, @pkozlowski-opensource whichever decission you take, tell me if you need help with anything

Contributor

jmendiara commented Mar 25, 2015

@petebacondarwin

For the oauth processing, could a custom serializer not do this as part of processing the params?

oauth1 signature needs params + url + method

If we have transformRequest and transformResponse functions, introducing a function buildUrl(config) function/service is very well aligned with $http() and $httpProvider.defaults API

I'm talking now about introducing the ability to buildUrl per request and in defaults, not my proposed $httpUrlBuilderFactory, as it is only sugar

Small code change, no new services/providers in core, fully customization by user and more use cases covered.

BTW, @pkozlowski-opensource whichever decission you take, tell me if you need help with anything

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource
Member

pkozlowski-opensource commented Mar 30, 2015

Suppressed by #11461

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 30, 2015

Member

@jmendiara if you got a minute to review #11461, this would be much appreciated!

Member

pkozlowski-opensource commented Mar 30, 2015

@jmendiara if you got a minute to review #11461, this would be much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment