fix(carto): Resolve conflicts between base URL and parameters (#8925)

modules/carto/src/api/common.ts

@@ -1,16 +1,4 @@
-import {VERSION} from '@deck.gl/core';
-
 export const DEFAULT_API_BASE_URL = 'https://gcp-us-east1.api.carto.com';
 export const DEFAULT_CLIENT = 'deck-gl-carto';
 export const V3_MINOR_VERSION = '3.4';
 export const MAX_GET_LENGTH = 8192;
-
-export const DEFAULT_PARAMETERS = {
-  v: V3_MINOR_VERSION,
-  deckglVersion: VERSION
-};
-
-export const DEFAULT_HEADERS = {
-  Accept: 'application/json',
-  'Content-Type': 'application/json'
-};

modules/carto/src/api/request-with-parameters.ts

@@ -1,37 +1,45 @@
+import {VERSION} from '@deck.gl/core';
 import {isPureObject} from '../utils';
 import {CartoAPIError} from './carto-api-error';
-import {DEFAULT_HEADERS, DEFAULT_PARAMETERS, MAX_GET_LENGTH} from './common';
+import {MAX_GET_LENGTH, V3_MINOR_VERSION} from './common';
 import type {APIErrorContext} from './types';
 
 /**
- * Simple encode parameter
+ * Parameters added to all requests issued with `requestWithParameters()`.
+ * These parameters override parameters already in the base URL, but not
+ * user-provided parameters.
  */
-function encodeParameter(name: string, value: unknown): string {
-  if (isPureObject(value) || Array.isArray(value)) {
-    return `${name}=${encodeURIComponent(JSON.stringify(value))}`;
-  }
-  return `${name}=${encodeURIComponent(value as string | boolean | number)}`;
-}
+const DEFAULT_PARAMETERS = {
+  v: V3_MINOR_VERSION,
+  deckglVersion: VERSION
+};
+
+const DEFAULT_HEADERS = {
+  Accept: 'application/json',
+  'Content-Type': 'application/json'
+};
 
 const REQUEST_CACHE = new Map<string, Promise<unknown>>();
+
 export async function requestWithParameters<T = any>({
   baseUrl,
-  parameters,
-  headers: customHeaders,
+  parameters = {},
+  headers: customHeaders = {},
   errorContext
 }: {
   baseUrl: string;
   parameters?: Record<string, unknown>;
-  headers: Record<string, string>;
+  headers?: Record<string, string>;
   errorContext: APIErrorContext;
 }): Promise<T> {
   parameters = {...DEFAULT_PARAMETERS, ...parameters};
-  const key = createCacheKey(baseUrl, parameters || {}, customHeaders || {});
+  baseUrl = excludeURLParameters(baseUrl, Object.keys(parameters));
+  const key = createCacheKey(baseUrl, parameters, customHeaders);
   if (REQUEST_CACHE.has(key)) {
     return REQUEST_CACHE.get(key) as Promise<T>;
   }
 
-  const url = parameters ? createURLWithParameters(baseUrl, parameters) : baseUrl;
+  const url = createURLWithParameters(baseUrl, parameters);
   const headers = {...DEFAULT_HEADERS, ...customHeaders};
 
   /* global fetch */
@@ -73,9 +81,35 @@ function createCacheKey(
   return JSON.stringify({baseUrl, parameters: parameterEntries, headers: headerEntries});
 }
 
-function createURLWithParameters(baseUrl: string, parameters: Record<string, unknown>): string {
-  const encodedParameters = Object.entries(parameters).map(([key, value]) =>
-    encodeParameter(key, value)
-  );
-  return `${baseUrl}?${encodedParameters.join('&')}`;
+/**
+ * Appends query string parameters to a URL. Existing URL parameters are kept,
+ * unless there is a conflict, in which case the new parameters override
+ * those already in the URL.
+ */
+function createURLWithParameters(
+  baseUrlString: string,
+  parameters: Record<string, unknown>
+): string {
+  const baseUrl = new URL(baseUrlString);
+  for (const [key, value] of Object.entries(parameters)) {
+    if (isPureObject(value) || Array.isArray(value)) {
+      baseUrl.searchParams.set(key, JSON.stringify(value));
+    } else {
+      baseUrl.searchParams.set(key, (value as string | boolean | number).toString());
+    }
+  }
+  return baseUrl.toString();
+}
+
+/**
+ * Deletes query string parameters from a URL.
+ */
+function excludeURLParameters(baseUrlString: string, parameters: string[]) {
+  const baseUrl = new URL(baseUrlString);
+  for (const param of parameters) {
+    if (baseUrl.searchParams.has(param)) {
+      baseUrl.searchParams.delete(param);
+    }
+  }
+  return baseUrl.toString();
 }

test/modules/carto/api/query.spec.ts

@@ -16,7 +16,7 @@ test('query', async t => {
     const [queryCall] = calls;
 
     t.match(queryCall.url, /v3\/sql\/carto_dw\/query/, 'connection');
-    t.match(queryCall.url, /q=SELECT%20\*%20FROM%20a\.b\.h3_table/, 'query');
+    t.match(queryCall.url, /q=SELECT\+\*\+FROM\+a\.b\.h3_table/, 'query');
     t.match(queryCall.url, /client\=CUSTOM_CLIENT/, 'clientId');
 
     t.ok(response, 'returns response');

test/modules/carto/api/request-with-parameters.spec.ts

@@ -1,5 +1,6 @@
 import test from 'tape-catch';
 import {requestWithParameters} from '@deck.gl/carto/api/request-with-parameters';
+import {V3_MINOR_VERSION} from '@deck.gl/carto/api/common';
 import {withMockFetchMapsV3} from '../mock-fetch';
 import {CartoAPIError} from '@deck.gl/carto';
 
@@ -158,3 +159,36 @@ test('requestWithParameters#method', async t => {
   });
   t.end();
 });
+
+test('requestWithParameters#precedence', async t => {
+  await withMockFetchMapsV3(async calls => {
+    t.equals(calls.length, 0, '0 initial calls');
+
+    await Promise.all([
+      requestWithParameters({
+        baseUrl: 'https://example.com/v1/params?test=1',
+        headers: {},
+        parameters: {}
+      }),
+      requestWithParameters({
+        baseUrl: `https://example.com/v1/params?test=2&v=3.0`,
+        headers: {},
+        parameters: {}
+      }),
+      requestWithParameters({
+        baseUrl: `https://example.com/v1/params?test=3&v=3.0`,
+        headers: {},
+        parameters: {v: '3.2'}
+      })
+    ]);
+
+    t.equals(calls.length, 3, '3 requests');
+
+    const [url1, url2, url3] = calls.map(call => new URL(call.url));
+
+    t.equals(url1.searchParams.get('v'), V3_MINOR_VERSION, 'unset');
+    t.equals(url2.searchParams.get('v'), V3_MINOR_VERSION, 'default overrides url');
+    t.equals(url3.searchParams.get('v'), '3.2', 'param overrides default');
+  });
+  t.end();
+});

test/modules/carto/sources/boundary-query-source.spec.ts

@@ -22,12 +22,12 @@ test('boundaryQuerySource', async t => {
     t.match(initCall.url, /matchingColumn=geoid/, 'matchingColumn');
     t.match(
       initCall.url,
-      /propertiesSqlQuery=select%20\*%20from%20%60a.b.properties_table%60/,
+      /propertiesSqlQuery=select\+\*\+from\+%60a.b.properties_table%60/,
       'propertiesSqlQuery'
     );
     t.match(initCall.url, /columns=column1%2Ccolumn2/, 'columns');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns source');
     t.deepEqual(tilejson.tiles, ['https://xyz.com/{z}/{x}/{y}?formatTiles=binary'], 'source.tiles');

test/modules/carto/sources/boundary-table-source.spec.ts

@@ -23,7 +23,7 @@ test('boundaryTableSource', async t => {
     t.match(initCall.url, /propertiesTableName=a.b.properties_table/, 'propertiesTableName');
     t.match(initCall.url, /columns=column1%2Ccolumn2/, 'columns');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns source');
     t.deepEqual(tilejson.tiles, ['https://xyz.com/{z}/{x}/{y}?formatTiles=binary'], 'source.tiles');

test/modules/carto/sources/h3-query-source.spec.ts

@@ -17,13 +17,13 @@ test('h3QuerySource', async t => {
     const [initCall, tilesetCall] = calls;
 
     t.match(initCall.url, /v3\/maps\/carto_dw\/query/, 'connection');
-    t.match(initCall.url, /aggregationExp=SUM\(population\)%20as%20pop/, 'aggregationExp');
+    t.match(initCall.url, /aggregationExp=SUM%28population%29\+as\+pop/, 'aggregationExp');
     t.match(initCall.url, /spatialDataColumn=h3/, 'spatialDataColumn');
     t.match(initCall.url, /spatialDataType=h3/, 'spatialDataType');
-    t.match(initCall.url, /q=SELECT%20\*%20FROM%20a\.b\.h3_table/, 'query');
+    t.match(initCall.url, /q=SELECT\+\*\+FROM\+a\.b\.h3_table/, 'query');
     t.match(initCall.url, /client\=CUSTOM_CLIENT/, 'clientId');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/h3-table-source.spec.ts

@@ -16,12 +16,12 @@ test('h3TableSource', async t => {
     const [initCall, tilesetCall] = calls;
 
     t.match(initCall.url, /v3\/maps\/carto_dw\/table/, 'connection');
-    t.match(initCall.url, /aggregationExp=SUM\(population\)%20as%20pop/, 'aggregationExp');
+    t.match(initCall.url, /aggregationExp=SUM%28population%29\+as\+pop/, 'aggregationExp');
     t.match(initCall.url, /spatialDataColumn=h3/, 'spatialDataColumn');
     t.match(initCall.url, /spatialDataType=h3/, 'spatialDataType');
     t.match(initCall.url, /name=a.b.h3_table/, 'table');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/h3-tileset-source.spec.ts

@@ -17,7 +17,7 @@ test('h3TilesetSource', async t => {
     t.match(initCall.url, /v3\/maps\/carto_dw\/tileset/, 'connection');
     t.match(initCall.url, /name=a.b.h3_tileset/, 'tileset');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/quadbin-query-source.spec.ts

@@ -16,12 +16,12 @@ test('quadbinQuerySource', async t => {
     const [initCall, tilesetCall] = calls;
 
     t.match(initCall.url, /v3\/maps\/carto_dw\/query/, 'connection');
-    t.match(initCall.url, /aggregationExp=SUM\(population\)%20as%20pop/, 'aggregationExp');
+    t.match(initCall.url, /aggregationExp=SUM%28population%29\+as\+pop/, 'aggregationExp');
     t.match(initCall.url, /spatialDataColumn=quadbin/, 'spatialDataColumn');
     t.match(initCall.url, /spatialDataType=quadbin/, 'spatialDataType');
-    t.match(initCall.url, /q=SELECT%20\*%20FROM%20a\.b\.quadbin_table/, 'query');
+    t.match(initCall.url, /q=SELECT\+\*\+FROM\+a\.b\.quadbin_table/, 'query');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/quadbin-table-source.spec.ts

@@ -16,12 +16,12 @@ test('quadbinTableSource', async t => {
     const [initCall, tilesetCall] = calls;
 
     t.match(initCall.url, /v3\/maps\/carto_dw\/table/, 'connection');
-    t.match(initCall.url, /aggregationExp=SUM\(population\)%20as%20pop/, 'aggregationExp');
+    t.match(initCall.url, /aggregationExp=SUM%28population%29\+as\+pop/, 'aggregationExp');
     t.match(initCall.url, /spatialDataColumn=quadbin/, 'spatialDataColumn');
     t.match(initCall.url, /spatialDataType=quadbin/, 'spatialDataType');
     t.match(initCall.url, /name=a.b.quadbin_table/, 'table');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/quadbin-tileset-source.spec.ts

@@ -17,7 +17,7 @@ test('quadbinTilesetSource', async t => {
     t.match(initCall.url, /v3\/maps\/carto_dw\/tileset/, 'connection');
     t.match(initCall.url, /name=a.b.quadbin_tileset/, 'tileset');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/raster-source.spec.ts

@@ -17,7 +17,7 @@ test('rasterSource', async t => {
     t.match(initCall.url, /v3\/maps\/carto_dw\/raster/, 'connection');
     t.match(initCall.url, /name=a\.b\.raster_table/, 'table');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/vector-query-source.spec.ts

@@ -18,7 +18,7 @@ test('vectorQuerySource', async t => {
     const [initCall, tilesetCall] = calls;
 
     t.match(initCall.url, /v3\/maps\/carto_dw\/query/, 'connection');
-    t.match(initCall.url, /q=SELECT%20\*%20FROM%20a\.b\.vector_table/, 'query');
+    t.match(initCall.url, /q=SELECT\+\*\+FROM\+a\.b\.vector_table/, 'query');
     t.match(initCall.url, /columns=a%2Cb/, 'columns');
     t.match(initCall.url, /spatialDataColumn=mygeom/, 'spatialDataColumn');
     t.match(initCall.url, /spatialDataType=geo/, 'spatialDataType');
@@ -28,7 +28,7 @@ test('vectorQuerySource', async t => {
       'queryParameters'
     );
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/vector-table-source.spec.ts

@@ -22,7 +22,7 @@ test('vectorTableSource', async t => {
     t.match(initCall.url, /spatialDataColumn=mygeom/, 'spatialDataColumn');
     t.match(initCall.url, /spatialDataType=geo/, 'spatialDataType');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(

test/modules/carto/sources/vector-tileset-source.spec.ts

@@ -17,7 +17,7 @@ test('vectorTilesetSource', async t => {
     t.match(initCall.url, /v3\/maps\/carto_dw\/tileset/, 'connection');
     t.match(initCall.url, /name=a\.b\.vector_tileset/, 'table');
 
-    t.match(tilesetCall.url, /^https:\/\/xyz\.com\?format\=tilejson\&cache\=/, 'tileset URL');
+    t.match(tilesetCall.url, /^https:\/\/xyz\.com\/\?format\=tilejson\&cache\=/, 'tileset URL');
 
     t.ok(tilejson, 'returns tilejson');
     t.deepEqual(