fix(config): fix race condition in config file (#381)

* fix(config): fix race condition in config file https://coveord.atlassian.net/browse/CDX-509 * fix UT + move to sync method https://coveord.atlassian.net/browse/CDX-509 * remove last asynchronicity (#386) https://coveord.atlassian.net/browse/CDX-509 Co-authored-by: Louis Bompart <lbompart@coveo.com>
coveo · Jul 30, 2021 · a65395d · a65395d
1 parent aec9ba0
commit a65395d
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 53 deletions.
diff --git a/packages/cli/src/commands/org/config/list.spec.ts b/packages/cli/src/commands/org/config/list.spec.ts
@@ -39,9 +39,9 @@ describe('org:config:list', () => {
   mockedConfig.mockImplementation(
     () =>
       ({
-        get: Promise.resolve({
+        get: {
           organization: 'foo',
-        }),
+        },
       } as unknown as Config)
   );
 

diff --git a/packages/cli/src/commands/ui/create/angular.spec.ts b/packages/cli/src/commands/ui/create/angular.spec.ts
@@ -62,7 +62,7 @@ describe('ui:create:angular', () => {
       () =>
         ({
           get: () =>
-            Promise.resolve({
+            ({
               environment: 'dev',
               organization: 'my-org',
               region: 'us-east-1',

diff --git a/packages/cli/src/commands/ui/create/react.spec.ts b/packages/cli/src/commands/ui/create/react.spec.ts
@@ -80,7 +80,7 @@ describe('ui:create:react', () => {
       () =>
         ({
           get: () =>
-            Promise.resolve({
+            ({
               environment: 'dev',
               organization: 'my-org',
               region: 'us-east-1',

diff --git a/packages/cli/src/commands/ui/create/vue.spec.ts b/packages/cli/src/commands/ui/create/vue.spec.ts
@@ -52,7 +52,7 @@ describe('ui:create:vue', () => {
       () =>
         ({
           get: () =>
-            Promise.resolve({
+            ({
               environment: 'dev',
               organization: 'my-org',
               region: 'us-east-1',

diff --git a/packages/cli/src/hooks/analytics/analytics.spec.ts b/packages/cli/src/hooks/analytics/analytics.spec.ts
@@ -63,7 +63,7 @@ describe('analytics hook', () => {
       () =>
         ({
           get: () =>
-            Promise.resolve({
+            ({
               environment: 'dev',
               organization: 'foo',
               region: 'us-east-1',
@@ -211,7 +211,7 @@ describe('analytics hook', () => {
       () =>
         ({
           get: () =>
-            Promise.resolve({
+            ({
               analyticsEnabled: false,
             } as Configuration),
         } as Config)

diff --git a/packages/cli/src/lib/config/config.spec.ts b/packages/cli/src/lib/config/config.spec.ts
@@ -1,28 +1,33 @@
-import {pathExists, createFile, writeJSON, readJSON} from 'fs-extra';
+import {
+  pathExistsSync,
+  createFileSync,
+  writeJSONSync,
+  readJSONSync,
+} from 'fs-extra';
 import {join} from 'path';
 import {mocked} from 'ts-jest/utils';
 import {Config} from './config';
 jest.mock('fs-extra');
-const mockedPathExists = mocked(pathExists);
-const mockedCreateFile = mocked(createFile);
-const mockedWriteJSON = mocked(writeJSON);
-const mockedReadJSON = mocked(readJSON);
+const mockedPathExists = mocked(pathExistsSync);
+const mockedCreateFile = mocked(createFileSync);
+const mockedWriteJSON = mocked(writeJSONSync);
+const mockedReadJSON = mocked(readJSONSync);
 
 describe('config', () => {
   beforeEach(() => {
-    mockedReadJSON.mockImplementation(() => Promise.resolve({}));
+    mockedReadJSON.mockImplementation(() => {});
   });
 
   it('should ensure config file exists', async () => {
-    await new Config('foo/bar').get();
+    new Config('foo/bar').get();
     expect(mockedPathExists).toHaveBeenCalledWith(
       join('foo', 'bar', 'config.json')
     );
   });
 
   it('should create config file when it does not exists', async () => {
-    mockedPathExists.mockImplementationOnce(() => Promise.resolve(false));
-    await new Config('foo/bar').get();
+    mockedPathExists.mockImplementationOnce(() => false);
+    new Config('foo/bar').get();
     expect(mockedCreateFile).toHaveBeenCalledWith(
       join('foo', 'bar', 'config.json')
     );
@@ -33,27 +38,27 @@ describe('config', () => {
   });
 
   it('should not create config file when it does exists', async () => {
-    mockedPathExists.mockImplementationOnce(() => Promise.resolve(true));
-    await new Config('foo/bar').get();
+    mockedPathExists.mockImplementationOnce(() => true);
+    new Config('foo/bar').get();
     expect(mockedCreateFile).not.toHaveBeenCalled();
     expect(mockedWriteJSON).not.toHaveBeenCalled();
   });
 
   describe('when the config file exists', () => {
     beforeEach(() => {
-      mockedPathExists.mockImplementationOnce(() => Promise.resolve(true));
+      mockedPathExists.mockImplementationOnce(() => true);
     });
 
     it('should return the config if no error', async () => {
       const someConfig = {foo: 'bar'};
-      mockedReadJSON.mockImplementationOnce(() => Promise.resolve(someConfig));
-      const cfg = await new Config('foo/bar').get();
+      mockedReadJSON.mockImplementationOnce(() => someConfig);
+      const cfg = new Config('foo/bar').get();
       expect(cfg).toBe(someConfig);
     });
 
     it('should create default config on error', async () => {
-      mockedReadJSON.mockImplementationOnce(() => Promise.reject('oh noes'));
-      await new Config('foo/bar').get();
+      mockedReadJSON.mockReturnValueOnce(new Error('oh noes'));
+      new Config('foo/bar').get();
       expect(mockedWriteJSON).toHaveBeenCalledWith(
         join('foo', 'bar', 'config.json'),
         expect.objectContaining({})
@@ -68,7 +73,7 @@ describe('config', () => {
         analyticsEnabled: true,
         accessToken: 'the-token',
       };
-      await new Config('foo/bar').replace(theNewConfig);
+      new Config('foo/bar').replace(theNewConfig);
       expect(mockedWriteJSON).toHaveBeenCalledWith(
         join('foo', 'bar', 'config.json'),
         theNewConfig
@@ -77,7 +82,7 @@ describe('config', () => {
 
     it('should write config on set', async () => {
       mockedReadJSON.mockImplementationOnce(() => ({hello: 'world'}));
-      await new Config('foo/bar').set('environment', 'dev');
+      new Config('foo/bar').set('environment', 'dev');
       expect(mockedWriteJSON).toHaveBeenCalledWith(
         join('foo', 'bar', 'config.json'),
         expect.objectContaining({

diff --git a/packages/cli/src/lib/config/config.ts b/packages/cli/src/lib/config/config.ts
@@ -1,4 +1,9 @@
-import {readJSON, writeJSON, pathExists, createFile} from 'fs-extra';
+import {
+  pathExistsSync,
+  createFileSync,
+  writeJSONSync,
+  readJSONSync,
+} from 'fs-extra';
 import {join} from 'path';
 import {PlatformEnvironment, PlatformRegion} from '../platform/environment';
 
@@ -25,13 +30,17 @@ export class Config {
     private error = console.error
   ) {}
 
-  public async get(): Promise<Configuration> {
-    await this.ensureExists();
+  public get(): Configuration {
+    this.ensureExists();
     try {
-      return await readJSON(this.configPath);
+      const content = readJSONSync(this.configPath);
+      if (content instanceof Error) {
+        throw content;
+      }
+      return content;
     } catch (e) {
       this.error(`Error while reading configuration at ${this.configPath}`);
-      await this.replace(DefaultConfig);
+      this.replace(DefaultConfig);
       this.error(
         `Configuration has been reset to default value: ${JSON.stringify(
           DefaultConfig
@@ -41,51 +50,51 @@ export class Config {
     }
   }
 
-  public async replace(config: Configuration) {
-    await this.ensureExists();
-    return await writeJSON(this.configPath, config);
+  public replace(config: Configuration) {
+    this.ensureExists();
+    return writeJSONSync(this.configPath, config);
   }
 
-  public async set<K extends keyof Configuration, V extends Configuration[K]>(
+  public set<K extends keyof Configuration, V extends Configuration[K]>(
     key: K,
     value: V
   ) {
-    await this.ensureExists();
-    const config = await this.get();
+    this.ensureExists();
+    const config = this.get();
     config[key] = value;
-    await this.replace(config);
+    this.replace(config);
   }
 
-  public async setAny(key: string, value: unknown) {
-    await this.ensureExists();
-    const config = await this.get();
+  public setAny(key: string, value: unknown) {
+    this.ensureExists();
+    const config = this.get();
     config[key] = value;
-    await this.replace(config);
+    this.replace(config);
   }
 
-  public async delete<K extends keyof Configuration>(key: K) {
-    await this.ensureExists();
-    const config = await this.get();
+  public delete<K extends keyof Configuration>(key: K) {
+    this.ensureExists();
+    const config = this.get();
     delete config[key];
-    await this.replace(config);
+    this.replace(config);
   }
 
-  public async deleteAny(key: string) {
-    await this.ensureExists();
-    const config = await this.get();
+  public deleteAny(key: string) {
+    this.ensureExists();
+    const config = this.get();
     delete config[key];
-    await this.replace(config);
+    this.replace(config);
   }
 
   private get configPath() {
     return join(this.configDir, 'config.json');
   }
 
-  private async ensureExists() {
-    const exists = await pathExists(this.configPath);
+  private ensureExists() {
+    const exists = pathExistsSync(this.configPath);
     if (!exists) {
-      await createFile(this.configPath);
-      await writeJSON(this.configPath, DefaultConfig);
+      createFileSync(this.configPath);
+      writeJSONSync(this.configPath, DefaultConfig);
     }
   }
 }