Validation refactoring per code review

mikechu-optimizely · mikechu-optimizely · commit fed5e9b11f94 · 2022-09-19T09:40:36.000-04:00
diff --git a/packages/optimizely-sdk/lib/plugins/odp/graphql_manager.ts b/packages/optimizely-sdk/lib/plugins/odp/graphql_manager.ts
@@ -74,29 +74,23 @@ export class GraphqlManager implements IGraphQLManager {
    * @param segmentsToCheck Audience segments to check for experiment inclusion
    */
   public async fetchSegments(apiKey: string, apiEndpoint: string, userKey: string, userValue: string, segmentsToCheck: string[]): Promise<string[] | null> {
-    if (!apiEndpoint || !apiKey) {
-      this._logger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to fetch segments');
-      return null;
+    if (segmentsToCheck?.length === 0) {
+      return EMPTY_SEGMENTS_COLLECTION;
     }
 
-    const parameters = new QuerySegmentsParameters({
+    const parameters = new QuerySegmentsParameters(
       apiKey,
       apiEndpoint,
       userKey,
       userValue,
       segmentsToCheck,
-    });
-
+    );
     const segmentsResponse = await this._odpClient.querySegments(parameters);
     if (!segmentsResponse) {
       this._logger.log(LogLevel.ERROR, 'Audience segments fetch failed (network error)');
       return null;
     }
 
-    if (segmentsToCheck?.length === 0) {
-      return EMPTY_SEGMENTS_COLLECTION;
-    }
-
     const parsedSegments = this.parseSegmentsResponseJson(segmentsResponse);
     if (!parsedSegments) {
       this._logger.log(LogLevel.ERROR, 'Audience segments fetch failed (decode error)');
@@ -131,6 +125,8 @@ export class GraphqlManager implements IGraphQLManager {
 
     try {
       jsonObject = JSON.parse(jsonResponse);
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
     } catch (error: any) {
       this._errorHandler.handleError(error);
diff --git a/packages/optimizely-sdk/lib/plugins/odp/odp_client.ts b/packages/optimizely-sdk/lib/plugins/odp/odp_client.ts
@@ -67,21 +67,7 @@ export class OdpClient implements IOdpClient {
    * @returns JSON response string from ODP or null
    */
   public async querySegments(parameters: QuerySegmentsParameters): Promise<string | null> {
-    const { apiEndpoint, apiKey, httpVerb, userKey, userValue, segmentsToCheck } = parameters;
-
-    if (!apiEndpoint || !apiKey) {
-      this._logger.log(LogLevel.ERROR, 'No ApiHost or ApiKey set before querying segments');
-      return null;
-    }
-
-    if (!userKey || !userValue) {
-      this._logger.log(LogLevel.ERROR, 'No UserKey or UserValue set before querying segments');
-      return null;
-    }
-
-    if (segmentsToCheck?.length === 0) {
-      return '';
-    }
+    const { apiEndpoint, apiKey, httpVerb } = parameters;
 
     const method = httpVerb;
     const url = apiEndpoint;
@@ -95,6 +81,8 @@ export class OdpClient implements IOdpClient {
     try {
       const request = this._requestHandler.makeRequest(url, headers, method, data);
       response = await request.responsePromise;
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
     } catch (error: any) {
       this._errorHandler.handleError(error);
@@ -118,17 +106,12 @@ export class OdpClient implements IOdpClient {
   public async sendEvents(parameters: SendEventsParameters): Promise<number | null> {
     const { apiEndpoint, apiKey, httpVerb, events } = parameters;
 
-    if (!apiEndpoint || !apiKey) {
-      this._logger.log(LogLevel.ERROR, 'No ApiEndpoint or ApiKey set before attempting to send ODP events');
-      return null;
-    }
-
     if (events?.length === 0) {
       return null;
     }
 
     const method = httpVerb;
-    const url = apiEndpoint ?? '';
+    const url = apiEndpoint;
     const headers = {
       'Content-Type': 'application/json',
       'x-api-key': apiKey,
@@ -139,6 +122,8 @@ export class OdpClient implements IOdpClient {
     try {
       const request = this._requestHandler.makeRequest(url, headers, method, data);
       response = await request.responsePromise;
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
     } catch (error: any) {
       this._errorHandler.handleError(error);
diff --git a/packages/optimizely-sdk/lib/plugins/odp/odp_request_parameters.ts b/packages/optimizely-sdk/lib/plugins/odp/odp_request_parameters.ts
@@ -15,24 +15,34 @@
  */
 
 export interface IOdpRequestParameters {
-  apiKey?: string;
-  apiEndpoint?: string;
+  apiKey: string;
+  apiEndpoint: string;
   httpVerb: string;
 }
 
 export abstract class OdpRequestParameters implements IOdpRequestParameters {
   /**
    * Optimizely Data Platform API key
    */
-  public apiKey?: string;
+  public readonly apiKey: string;
 
   /**
    * Fully-qualified URL to ODP events endpoint
    */
-  public apiEndpoint?: string;
+  public readonly apiEndpoint: string;
 
   /**
    * HTTP Verb used to send request
    */
-  public abstract readonly httpVerb: string;
+  public readonly httpVerb: string;
+
+  protected constructor(apiKey: string, apiEndpoint: string, httpVerb: string) {
+    if (!apiEndpoint || !apiKey) {
+      throw new Error('Parameters apiKey and apiEndpoint are required');
+    }
+
+    this.apiKey = apiKey;
+    this.apiEndpoint = apiEndpoint;
+    this.httpVerb = httpVerb ?? 'POST';
+  }
 }
diff --git a/packages/optimizely-sdk/lib/plugins/odp/query_segments_parameters.ts b/packages/optimizely-sdk/lib/plugins/odp/query_segments_parameters.ts
@@ -23,26 +23,32 @@ export class QuerySegmentsParameters extends OdpRequestParameters {
   /**
    * 'vuid' or 'fs_user_id' (client device id or fullstack id)
    */
-  public userKey?: string;
+  public readonly userKey: string;
 
   /**
    * Value for the user key
    */
-  public userValue?: string;
+  public readonly userValue: string;
 
   /**
    * Audience segments to check for inclusion in the experiment
    */
-  public segmentsToCheck?: string[];
+  public readonly segmentsToCheck: string[];
 
-  /**
-   * HTTP Verb used to send request
-   */
-  public readonly httpVerb = 'POST';
+  constructor(apiKey: string, apiEndpoint: string, userKey: string, userValue: string, segmentsToCheck: string[]) {
+    super(apiKey, apiEndpoint, 'POST');
+
+    if (!userKey || !userValue) {
+      throw new Error('Parameters userKey or userValue are required');
+    }
+
+    if (segmentsToCheck.length < 1) {
+      throw new Error('Parameter segmentsToCheck must have elements');
+    }
 
-  constructor(parameters: { apiKey: string, apiEndpoint: string, userKey: string, userValue: string, segmentsToCheck: string[] }) {
-    super();
-    Object.assign(this, parameters);
+    this.userKey = userKey;
+    this.userValue = userValue;
+    this.segmentsToCheck = segmentsToCheck;
   }
 
   /**
diff --git a/packages/optimizely-sdk/lib/plugins/odp/send_events_parameters.ts b/packages/optimizely-sdk/lib/plugins/odp/send_events_parameters.ts
@@ -24,16 +24,16 @@ export class SendEventsParameters extends OdpRequestParameters {
   /**
    * Collection of ODP events to transmit
    */
-  public events?: OdpEvent[];
+  public readonly events: OdpEvent[];
 
-  /**
-   * HTTP Verb used to send request
-   */
-  public httpVerb = 'POST';
+  constructor(apiKey: string, apiEndpoint: string, events: OdpEvent[]) {
+    super(apiKey, apiEndpoint, 'POST');
+
+    if (OdpEvent.length < 1) {
+      throw new Error('Parameter events must have elements');
+    }
 
-  constructor(parameters: { apiKey: string, apiEndpoint: string, events: OdpEvent[] }) {
-    super();
-    Object.assign(this, parameters);
+    this.events = events;
   }
 
   /**
diff --git a/packages/optimizely-sdk/tests/odpQuerySegments.spec.ts b/packages/optimizely-sdk/tests/odpQuerySegments.spec.ts
@@ -23,17 +23,17 @@ import { QuerySegmentsParameters } from '../lib/plugins/odp/query_segments_param
 import { BrowserRequestHandler } from '../lib/utils/http_request_handler/browser_request_handler';
 import { NodeRequestHandler } from '../lib/utils/http_request_handler/node_request_handler';
 
-const MOCK_QUERY_PARAMETERS = new QuerySegmentsParameters({
-  apiKey: 'not-real-api-key',
-  apiEndpoint: 'https://api.example.com/v3/graphql',
-  userKey: 'fs_user_id',
-  userValue: 'mock-user-id',
-  segmentsToCheck: [
+const MOCK_QUERY_PARAMETERS = new QuerySegmentsParameters(
+  'not-real-api-key',
+  'https://api.example.com/v3/graphql',
+  'fs_user_id',
+  'mock-user-id',
+  [
     'has_email',
     'has_email_opted_in',
     'push_on_sale',
   ],
-});
+);
 const VALID_RESPONSE_JSON = {
   'data': {
     'customer': {
@@ -78,85 +78,64 @@ describe('OdpClient Query Segments', () => {
     resetCalls(mockNodeRequestHandler);
   });
 
-  it('should handle missing API Endpoint', async () => {
-    const missingApiEndpoint = new QuerySegmentsParameters({
-      apiKey: 'apiKey',
-      apiEndpoint: '',
-      userKey: 'userKey',
-      userValue: 'userValue',
-      segmentsToCheck: ['segmentToCheck'],
-    });
-    const client = new OdpClient(instance(mockErrorHandler), instance(mockLogger), instance(mockNodeRequestHandler));
-
-    await client.querySegments(missingApiEndpoint);
-
-    verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(LogLevel.ERROR, 'No ApiHost or ApiKey set before querying segments')).once();
+  it('should handle missing API Endpoint', () => {
+    expect(() => {
+      new QuerySegmentsParameters(
+        'apiKey',
+        '',
+        'userKey',
+        'userValue',
+        ['segmentToCheck'],
+      );
+    }).toThrow('Parameters apiKey and apiEndpoint are required');
   });
 
-  it('should handle missing API Key', async () => {
-    const missingApiHost = new QuerySegmentsParameters({
-      apiKey: '',
-      apiEndpoint: 'apiEndpoint',
-      userKey: 'userKey',
-      userValue: 'userValue',
-      segmentsToCheck: ['segmentToCheck'],
-    });
-    const client = new OdpClient(instance(mockErrorHandler), instance(mockLogger), instance(mockNodeRequestHandler));
-
-    await client.querySegments(missingApiHost);
-
-    verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(LogLevel.ERROR, 'No ApiHost or ApiKey set before querying segments')).once();
+  it('should handle missing API Key', () => {
+    expect(() => {
+      new QuerySegmentsParameters(
+        '',
+        'apiEndpoint',
+        'userKey',
+        'userValue',
+        ['segmentToCheck'],
+      );
+    }).toThrow('Parameters apiKey and apiEndpoint are required');
   });
 
-  it('should handle missing User Key', async () => {
-    const missingApiEndpoint = new QuerySegmentsParameters({
-      apiKey: 'apiKey',
-      apiEndpoint: 'apiEndpoint',
-      userKey: '',
-      userValue: 'userValue',
-      segmentsToCheck: ['segmentToCheck'],
-    });
-    const client = new OdpClient(instance(mockErrorHandler), instance(mockLogger), instance(mockNodeRequestHandler));
-
-    await client.querySegments(missingApiEndpoint);
-
-    verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(LogLevel.ERROR, 'No UserKey or UserValue set before querying segments')).once();
+  it('should handle missing User Key', () => {
+    expect(() => {
+      new QuerySegmentsParameters(
+        'apiKey',
+        'apiEndpoint',
+        '',
+        'userValue',
+        ['segmentToCheck'],
+      );
+    }).toThrow('Parameters userKey or userValue are required');
   });
 
-  it('should handle missing User Value', async () => {
-    const missingApiHost = new QuerySegmentsParameters({
-      apiKey: 'apiKey',
-      apiEndpoint: 'apiEndpoint',
-      userKey: 'userKey',
-      userValue: '',
-      segmentsToCheck: ['segmentToCheck'],
-    });
-    const client = new OdpClient(instance(mockErrorHandler), instance(mockLogger), instance(mockNodeRequestHandler));
-
-    await client.querySegments(missingApiHost);
-
-    verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(LogLevel.ERROR, 'No UserKey or UserValue set before querying segments')).once();
+  it('should handle missing User Value', () => {
+    expect(() => {
+      new QuerySegmentsParameters(
+        'apiKey',
+        'apiEndpoint',
+        'userKey',
+        '',
+        ['segmentToCheck'],
+      );
+    }).toThrow('Parameters userKey or userValue are required');
   });
 
-  it('should handle no segments being requested', async () => {
-    const missingApiHost = new QuerySegmentsParameters({
-      apiKey: 'apiKey',
-      apiEndpoint: 'apiEndpoint',
-      userKey: 'userKey',
-      userValue: 'userValue',
-      segmentsToCheck: [],
-    });
-    const client = new OdpClient(instance(mockErrorHandler), instance(mockLogger), instance(mockNodeRequestHandler));
-
-    const json = await client.querySegments(missingApiHost);
-
-    verify(mockErrorHandler.handleError(anything())).never();
-    verify(mockLogger.log(anything(), anyString())).never();
-    expect(json).toBe('');
+  it('should handle no segments being requested', () => {
+    expect(() => {
+      new QuerySegmentsParameters(
+        'apiKey',
+        'apiEndpoint',
+        'userKey',
+        'userValue',
+        [],
+      );
+    }).toThrow('Parameter segmentsToCheck must have elements');
   });
 
   it('Browser: should get mocked segments successfully', async () => {
diff --git a/packages/optimizely-sdk/tests/odpSendEvents.spec.ts b/packages/optimizely-sdk/tests/odpSendEvents.spec.ts
