fix: Move check for serviceUrl to createRequest (#47)

IBM · Oct 3, 2019 · 6f04739 · 6f04739
1 parent c5556de
commit 6f04739
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 37 deletions.
diff --git a/lib/base_service.ts b/lib/base_service.ts
@@ -129,7 +129,7 @@ export class BaseService {
    *
    * @param {Object} parameters - service request options passed in by user.
    * @param {string} parameters.options.method - the http method.
-   * @param {string} parameters.options.url - the URL of the service.
+   * @param {string} parameters.options.url - the path portion of the URL to be appended to the serviceUrl
    * @param {string} parameters.options.path - the path to be appended to the service URL.
    * @param {string} parameters.options.qs - the querystring to be included in the URL.
    * @param {string} parameters.options.body - the data to be sent as the request body.
@@ -140,11 +140,19 @@ export class BaseService {
    * - object: the value is converted to a JSON string before insertion into the form body
    * - NodeJS.ReadableStream|Buffer|FileWithMetadata: sent as a file, with any associated metadata
    * - array: each element of the array is sent as a separate form part using any special processing as described above
-   * @param {HeaderOptions} parameters.options.headers - additional headers to be passed on the request.
+   * @param {Object} parameters.defaultOptions
+   * @param {string} parameters.defaultOptions.serviceUrl - the base URL of the service
+   * @param {HeaderOptions} parameters.defaultOptions.headers - additional headers to be passed on the request.
    * @param {Function} callback - callback function to pass the response back to
    * @returns {ReadableStream|undefined}
    */
   protected createRequest(parameters, callback) {
+    // validate serviceUrl parameter has been set
+    const serviceUrl = parameters.defaultOptions && parameters.defaultOptions.serviceUrl;
+    if (!serviceUrl || typeof serviceUrl !== 'string') {
+      return callback(new Error('The service URL is required'), null);
+    }
+
     this.authenticator.authenticate(parameters.defaultOptions, err => {
       err ? callback(err) : this.requestWrapperInstance.sendRequest(parameters, callback);
     });

diff --git a/lib/requestwrapper.ts b/lib/requestwrapper.ts
@@ -125,14 +125,6 @@ export class RequestWrapper {
     const { path, body, form, formData, qs, method, serviceUrl } = options;
     let { headers, url } = options;
 
-    // validate service url parameter has been set
-    if (!serviceUrl || typeof serviceUrl !== 'string') {
-      return _callback(new Error('The service URL is required'), null);
-    }
-
-    // use default service url if `url` parameter is not specified in generated method
-    url = url || serviceUrl;
-
     const multipartForm = new FormData();
 
     // Form params

diff --git a/test/unit/base-service.test.js b/test/unit/base-service.test.js
@@ -262,6 +262,33 @@ describe('Base Service', () => {
     expect(testService.requestWrapperInstance.sendRequest).toBe(sendRequestMock); // verify it is calling the instance
   });
 
+  it('should call callback with an error if `serviceUrl` is not set', done => {
+    const testService = new TestService({
+      authenticator: AUTHENTICATOR,
+    });
+    testService.setServiceUrl(undefined);
+
+    const parameters = {
+      defaultOptions: {
+        body: 'post=body',
+        formData: '',
+        qs: {},
+        method: 'POST',
+        headers: {
+          'test-header': 'test-header-value',
+        },
+        responseType: 'buffer',
+      },
+    };
+
+    testService.createRequest(parameters, (err, res) => {
+      // assert results
+      expect(err).toBeInstanceOf(Error);
+      expect(res).toBeNull();
+      done();
+    });
+  });
+
   it('should send error back to user on authenticate() failure', done => {
     const testService = new TestService({
       authenticator: AUTHENTICATOR,
@@ -272,7 +299,13 @@ describe('Base Service', () => {
     const fakeError = new Error('token request failed');
     authenticateMock.mockImplementation((_, callback) => callback(fakeError));
 
-    testService.createRequest(EMPTY_OBJECT, err => {
+    const parameters = {
+      defaultOptions: {
+        serviceUrl: 'https://foo.bar.baz/api',
+      },
+    };
+
+    testService.createRequest(parameters, err => {
       expect(err).toBe(fakeError);
       expect(authenticateMock).toHaveBeenCalled();
       done();

diff --git a/test/unit/requestWrapper.test.js b/test/unit/requestWrapper.test.js
@@ -59,7 +59,7 @@ describe('sendRequest', () => {
         formData: '',
         qs: {},
         method: 'POST',
-        serviceUrl:
+        url:
           'https://example.ibm.com/v1/environments/environment-id/configurations/configuration-id',
         headers: {
           'test-header': 'test-header-value',
@@ -326,31 +326,6 @@ describe('sendRequest', () => {
     });
   });
 
-  it('should call callback with an error if `serviceUrl` is not set', done => {
-    const parameters = {
-      defaultOptions: {
-        body: 'post=body',
-        formData: '',
-        qs: {},
-        method: 'POST',
-        headers: {
-          'test-header': 'test-header-value',
-        },
-        responseType: 'buffer',
-      },
-    };
-
-    mockAxiosInstance.mockResolvedValue(axiosResolveValue);
-
-    requestWrapperInstance.sendRequest(parameters, (err, res) => {
-      // assert results
-      expect(err).toBeInstanceOf(Error);
-      expect(res).toBeNull();
-      expect(mockAxiosInstance).not.toHaveBeenCalled();
-      done();
-    });
-  });
-
   // Need to rewrite this to test instantiation with userOptions
 
   //   it('should keep parameters in options that are not explicitly set in requestwrapper', done => {