Change from manual URL construction to using a lib

Makes the code cleaner to read and more resilient to changes

Fixes https://jira.coreos.com/browse/QUAY-940

oauth/base.py

@@ -26,13 +26,6 @@ class OAuthEndpoint(object):
     params_copy.update(parameters)
     return OAuthEndpoint(self.base_url, params_copy)
     
-  def to_url_prefix(self):
-    prefix = self.to_url()
-    if self.params:
-      return prefix + '&'
-    else:
-      return prefix + '?'
-
   def to_url(self):
     (scheme, netloc, path, _, fragment) = urlparse.urlsplit(self.base_url)
     updated_query = urllib.urlencode(self.params)

oauth/services/github.py

@@ -113,7 +113,7 @@ class GithubOAuthService(OAuthLoginService):
   def get_public_config(self):
     return  {
       'CLIENT_ID': self.client_id(),
-      'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url_prefix(),
+      'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url(),
       'GITHUB_ENDPOINT': self._endpoint(),
       'ORG_RESTRICT': self.config.get('ORG_RESTRICT', False)
     }

oauth/services/gitlab.py

@@ -55,6 +55,6 @@ class GitLabOAuthService(OAuthService):
   def get_public_config(self):
     return {
       'CLIENT_ID': self.client_id(),
-      'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url_prefix(),
+      'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url(),
       'GITLAB_ENDPOINT': self._endpoint(),
     }

oauth/services/google.py

@@ -61,7 +61,7 @@ class GoogleOAuthService(OAuthLoginService):
   def get_public_config(self):
     return  {
       'CLIENT_ID': self.client_id(),
-      'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url_prefix()
+      'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url()
     }
 
   def get_login_service_id(self, user_info):

oauth/services/test/test_github.py

@@ -29,7 +29,6 @@ def test_basic_enterprise_config(trigger_config, domain, api_endpoint, is_enterp
   assert github_trigger.is_enterprise() == is_enterprise
 
   assert github_trigger.authorize_endpoint().to_url() == '%s/login/oauth/authorize' % domain
-  assert github_trigger.authorize_endpoint().to_url_prefix() == '%s/login/oauth/authorize?' % domain
 
   assert github_trigger.token_endpoint().to_url() == '%s/login/oauth/access_token' % domain
 

package.json

@@ -43,6 +43,7 @@
     "showdown": "^1.6.4",
     "underscore": "^1.5.2",
     "urijs": "^1.18.10",
+    "url-parse": "^1.4.0",
     "zeroclipboard": "^2.3.0"
   },
   "devDependencies": {

static/js/directives/ui/entity-search.js

@@ -84,7 +84,7 @@ angular.module('quay').directive('entitySearch', function () {
       $scope.checkLazyLoad = function() {
         if (!$scope.namespace || !$scope.thisUser || !$scope.requiresLazyLoading ||
              $scope.isLazyLoading || !$scope.userRequestedLazyLoading) {
-              return;
+            return;
         }
 
         $scope.isLazyLoading = true;
@@ -229,15 +229,20 @@ angular.module('quay').directive('entitySearch', function () {
           name: 'entities' + $rootScope.__entity_search_counter,
           remote: {
             url: '/api/v1/entities/%QUERY',
-            replace: function (url, uriEncodedQuery) {
+            replace: function (query_url, uriEncodedQuery) {
+              $scope.lazyLoad();
+
               var namespace = $scope.namespace || '';
-              url = url.replace('%QUERY', uriEncodedQuery);
+
-              url += '?namespace=' + encodeURIComponent(namespace);
+              var url = UtilService.getRestUrl(query_url.replace('%QUERY', uriEncodedQuery));
+              url.setQueryParameter('namespace', namespace);
+
               if ($scope.isOrganization && isSupported('team')) {
-                url += '&includeTeams=true'
+                url.setQueryParameter('includeTeams', true);
               }
+
               if (isSupported('org')) {
-                url += '&includeOrgs=true'
+                url.setQueryParameter('includeOrgs', true);
               }
               return url;
             },

static/js/directives/ui/logs-view.js

@@ -1,5 +1,5 @@
 import { LogUsageChart } from '../../graphing';
-
+import { parse } from 'path';
 
 /**
  * Element which displays usage logs for the given entity.
@@ -383,8 +383,8 @@ angular.module('quay').directive('logsView', function () {
           url = UtilService.getRestUrl('superuser', suffix)
         }
 
-        url += '?starttime=' + encodeURIComponent(getDateString($scope.options.logStartDate));
+        url.setQueryParameter('starttime', getDateString($scope.options.logStartDate));
-        url += '&endtime=' + encodeURIComponent(getDateString($scope.options.logEndDate));
+        url.setQueryParameter('endtime', getDateString($scope.options.logEndDate));
         return url;
       };
 
@@ -405,7 +405,7 @@ angular.module('quay').directive('logsView', function () {
 
         $scope.chartLoading = true;
 
-        var aggregateUrl = getUrl('aggregatelogs')
+        var aggregateUrl = getUrl('aggregatelogs').toString();
         var loadAggregate = Restangular.one(aggregateUrl);
         loadAggregate.customGET().then(function(resp) {
           $scope.chart = new LogUsageChart(logKinds);
@@ -430,12 +430,10 @@ angular.module('quay').directive('logsView', function () {
 
         $scope.loading = true;
 
-        var logsUrl = getUrl('logs');
+        var url = getUrl('logs');
-        if ($scope.nextPageToken) {
+        url.setQueryParameter('next_page', $scope.nextPageToken);
-          logsUrl = logsUrl + '&next_page=' + encodeURIComponent($scope.nextPageToken);
-        }
 
-        var loadLogs = Restangular.one(logsUrl);
+        var loadLogs = Restangular.one(url.toString());
         loadLogs.customGET().then(function(resp) {
           resp.logs.forEach(function(log) {
             $scope.logs.push(log);

static/js/services/api-service.js

@@ -1,3 +1,5 @@
+var urlParseURL = require('url-parse');
+
 /**
  * Service which exposes the server-defined API as a nice set of helper methods and automatic
  * callbacks. Any method defined on the server is exposed here as an equivalent method. Also
@@ -44,7 +46,7 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService'
 
     // Build the path, adjusted with the inline parameters.
     var used = {};
-    var url = '';
+    var urlPath = '';
     for (var i = 0; i < path.length; ++i) {
       var c = path[i];
       if (c == '{') {
@@ -56,29 +58,29 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService'
         }
 
         used[varName] = true;
-        url += parameters[varName];
+        urlPath += encodeURI(parameters[varName]);
         i = end;
         continue;
       }
 
-      url += c;
+      urlPath += c;
     }
 
     // Append any query parameters.
-    var isFirst = true;
+    var url = new urlParseURL(urlPath, '/');
+    url.query = {};
+
     for (var paramName in parameters) {
       if (!parameters.hasOwnProperty(paramName)) { continue; }
       if (used[paramName]) { continue; }
 
       var value = parameters[paramName];
       if (value != null) {
-        url += isFirst ? '?' : '&';
+        url.query[paramName] = value
-        url += paramName + '=' + encodeURIComponent(value)
-        isFirst = false;
       }
     }
 
-    return url;
+    return url.toString();
   };
 
   var getGenericOperationName = function(userOperationName) {

static/js/services/roles-service.js

@@ -29,7 +29,7 @@ angular.module('quay').factory('RolesService', ['UtilService', 'Restangular', 'A
     var namespace = repository.namespace;
     var name = repository.name;
     var url = UtilService.getRestUrl('repository', namespace, name, 'permissions', entityKind, entityName);
-    return Restangular.one(url);
+    return Restangular.one(url.toString());
   };
 
   roleService.deleteRepositoryRole = function(repository, entityKind, entityName, callback) {

static/js/services/trigger-service.js

@@ -23,11 +23,13 @@ angular.module('quay').factory('TriggerService', ['UtilService', '$sanitize', 'K
         var redirect_uri = KeyService['githubRedirectUri'] + '/trigger/' +
           namespace + '/' + repository;
 
-        var authorize_url = KeyService['githubTriggerAuthorizeUrl'];
         var client_id = KeyService['githubTriggerClientId'];
 
-        return authorize_url + 'client_id=' + client_id +
+        var authorize_url = new UtilService.UrlBuilder(KeyService['githubTriggerAuthorizeUrl']);
-          '&scope=repo,user:email&redirect_uri=' + redirect_uri;
+        authorize_url.setQueryParameter('client_id', client_id);
+        authorize_url.setQueryParameter('scope', 'repo,user:email');
+        authorize_url.setQueryParameter('redirect_uri', redirect_uri);
+        return authorize_url.toString();
       },
       'is_external': true,
       'is_enabled': function() {
@@ -81,10 +83,14 @@ angular.module('quay').factory('TriggerService', ['UtilService', '$sanitize', 'K
       'run_parameters': [branch_tag],
       'get_redirect_url': function(namespace, repository) {
         var redirect_uri = KeyService['gitlabRedirectUri'] + '/trigger';
-        var authorize_url = KeyService['gitlabTriggerAuthorizeUrl'];
         var client_id = KeyService['gitlabTriggerClientId'];
 
-        return authorize_url + 'client_id=' + client_id + '&redirect_uri=' + redirect_uri + '&response_type=code&state=repo:' + namespace + '/' + repository;
+        var authorize_url = new UtilService.UrlBuilder(KeyService['gitlabTriggerAuthorizeUrl']);
+        authorize_url.setQueryParameter('client_id', client_id);
+        authorize_url.setQueryParameter('state', 'repo:' + namespace + '/' + repository);
+        authorize_url.setQueryParameter('redirect_uri', redirect_uri);
+        authorize_url.setQueryParameter('response_type', 'code');
+        return authorize_url.toString();
       },
       'is_external': false,
       'is_enabled': function() {

static/js/services/util-service.js

@@ -1,3 +1,23 @@
+var urlParseURL = require('url-parse');
+
+var UrlBuilder = function(initial_url) {  
+  this.url = urlParseURL(initial_url || '', '/');
+};
+
+UrlBuilder.prototype.setQueryParameter = function(paramName, paramValue) {
+  if (paramValue == null) {
+    return;
+  }
+
+  this.url.query = this.url.query || {};
+  this.url.query[paramName] = paramValue;
+};
+
+UrlBuilder.prototype.toString = function() {
+  return this.url.toString();
+};
+
+
 /**
  * Service which exposes various utility methods.
  */
@@ -98,19 +118,23 @@ angular.module('quay').factory('UtilService', ['$sanitize', 'markdownConverter',
     };
 
     utilService.getRestUrl = function(args) {
-      var url = '';
+      var path = '';
+
       for (var i = 0; i < arguments.length; ++i) {
         if (i > 0) {
-           url += '/';
+          path += '/';
         }
-        url += encodeURI(arguments[i])
+        path += encodeURI(arguments[i])
       }
-      return url;
+    
+      return new UrlBuilder(path);
     };
 
     utilService.textToSafeHtml = function(text) {
       return $sanitize(utilService.escapeHtmlString(text));
     };
 
+    utilService.UrlBuilder = UrlBuilder;
+
     return utilService;
 }]);

yarn.lock

@@ -48,16 +48,6 @@
   version "0.0.32"
   resolved "https://registry.yarnpkg.com/@types/q/-/q-0.0.32.tgz#bd284e57c84f1325da702babfc82a5328190c0c5"
 
-"@types/react-dom@0.14.17":
-  version "0.14.17"
-  resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-0.14.17.tgz#d8b0dec27e873c218d9075856c6ca1c5db956d5d"
-  dependencies:
-    "@types/react" "*"
-
-"@types/react@*", "@types/react@0.14.39":
-  version "0.14.39"
-  resolved "https://registry.yarnpkg.com/@types/react/-/react-0.14.39.tgz#11cb715768da5f7605aa2030a5dc63e77a137eb5"
-
 "@types/selenium-webdriver@^2.53.35", "@types/selenium-webdriver@~2.53.39":
   version "2.53.42"
   resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-2.53.42.tgz#74cb77fb6052edaff2a8984ddafd88d419f25cac"
@@ -3044,6 +3034,10 @@ querystring@0.2.0:
   version "0.2.0"
   resolved "https://registry.yarnpkg.com/querystring/-/querystring-0.2.0.tgz#b209849203bb25df820da756e747005878521620"
 
+querystringify@^2.0.0:
+  version "2.0.0"
+  resolved "https://registry.yarnpkg.com/querystringify/-/querystringify-2.0.0.tgz#fa3ed6e68eb15159457c89b37bc6472833195755"
+
 randomatic@^1.1.3:
   version "1.1.6"
   resolved "https://registry.yarnpkg.com/randomatic/-/randomatic-1.1.6.tgz#110dcabff397e9dcff7c0789ccc0a49adf1ec5bb"
@@ -3257,7 +3251,7 @@ require-main-filename@^1.0.1:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/require-main-filename/-/require-main-filename-1.0.1.tgz#97f717b69d48784f5f526a6c5aa8ffdda055a4d1"
 
-requires-port@1.x.x:
+requires-port@1.x.x, requires-port@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/requires-port/-/requires-port-1.0.0.tgz#925d2601d39ac485e091cf0da5c6e694dc3dcaff"
 
@@ -3909,6 +3903,13 @@ urijs@^1.18.10:
   version "1.18.10"
   resolved "https://registry.yarnpkg.com/urijs/-/urijs-1.18.10.tgz#b94463eaba59a1a796036a467bb633c667f221ab"
 
+url-parse@^1.4.0:
+  version "1.4.0"
+  resolved "https://registry.yarnpkg.com/url-parse/-/url-parse-1.4.0.tgz#6bfdaad60098c7fe06f623e42b22de62de0d3d75"
+  dependencies:
+    querystringify "^2.0.0"
+    requires-port "^1.0.0"
+
 url@^0.11.0:
   version "0.11.0"
   resolved "https://registry.yarnpkg.com/url/-/url-0.11.0.tgz#3838e97cfc60521eb73c525a8e55bfdd9e2e28f1"