fix #1727, validate all objects before saving the first one to avoid …

…a situation when a component object is saved and its Version object is invalid

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [unreleased]
 
+- [#1727](https://github.com/teambit/bit/issues/1727) prevent saving objects that link to invalid objects
 - [#1856](https://github.com/teambit/bit/issues/1856) fix links deletion from `node_modules` after installing packages by a compiler on a capsule
 - [#1710](https://github.com/teambit/bit/issues/1710) improve performance of importing an entire collection
 

e2e/e2e-helper.js

@@ -337,6 +337,9 @@ export default class Helper {
       .sync(path.normalize(`**/${ext}`), { cwd: this.localScopePath, dot: includeDot })
       .map(x => path.normalize(x));
   }
+  getObjectFiles() {
+    return glob.sync(path.normalize('*/*'), { cwd: path.join(this.localScopePath, '.bit/objects') });
+  }
   createFile(folder: string, name: string, impl?: string = fixtures.fooFixture) {
     const filePath = path.join(this.localScopePath, folder, name);
     fs.outputFileSync(filePath, impl);

e2e/functionalities/envs.e2e.3.js

@@ -1525,3 +1525,49 @@ describe('envs with relative paths', function () {
     });
   });
 });
+
+describe('add an env with an invalid env name', function () {
+  this.timeout(0);
+  const helper = new Helper();
+  let numOfObjectsBeforeTagging;
+  before(() => {
+    helper.reInitLocalScope();
+    helper.importDummyCompiler();
+    const bitJson = helper.readBitJson();
+    bitJson.env = {
+      compiler: {
+        dummy: {
+          // an invalid name. doesn't have a scope name.
+          options: {
+            file: path.join(
+              helper.localScopePath,
+              `.bit/components/compilers/dummy/${helper.envScope}/0.0.1/compiler.js`
+            )
+          }
+        }
+      }
+    };
+    helper.writeBitJson(bitJson);
+    const objectFiles = helper.getObjectFiles();
+    numOfObjectsBeforeTagging = objectFiles.length;
+    helper.createComponentBarFoo();
+    helper.addComponentBarFoo();
+  });
+  after(() => {
+    helper.destroyEnv();
+  });
+  describe('tagging the component', () => {
+    let output;
+    before(() => {
+      output = helper.runWithTryCatch('bit tag -a');
+    });
+    it('should throw an error saying BitId is invalid', () => {
+      expect(output).to.have.string('the env.name has an invalid Bit id');
+    });
+    it('should not save anything into the objects dir', () => {
+      // see https://github.com/teambit/bit/issues/1727 for a previous bug about it
+      const numOfObjectsAfterTagging = helper.getObjectFiles().length;
+      expect(numOfObjectsAfterTagging).to.equal(numOfObjectsBeforeTagging);
+    });
+  });
+});

src/scope/objects/repository.js

@@ -264,10 +264,35 @@ export default class Repository {
   async persist(validate: boolean = true): Promise<void> {
     logger.debug(`Repository.persist, validate = ${validate.toString()}`);
     await this._deleteMany();
-    if (!validate) Object.keys(this.objects).map(hash => (this.objects[hash].validateBeforePersist = false));
+    this._validateObjects(validate);
     await this._writeMany();
   }
 
+  /**
+   * normally, the validation step takes place just before the acutal writing of the file.
+   * however, this can be an issue where a component has an invalid version. the component could
+   * be saved before validating the version (see #1727). that's why we validate here before writing
+   * anything to the filesystem.
+   * the open question here is whether should we validate again before the actual writing or it
+   * should be enough to validate here?
+   * for now, it does validate again before saving, only to be 100% sure nothing happens in a few
+   * lines of code until the actual writing. however, if the performance penalty is noticeable, we
+   * can easily revert it by changing `bitObject.validateBeforePersist = false` line run regardless
+   * the `validate` argument.
+   */
+  _validateObjects(validate: boolean) {
+    Object.keys(this.objects).forEach((hash) => {
+      const bitObject = this.objects[hash];
+      // $FlowFixMe some BitObject classes have validate() method
+      if (validate && bitObject.validate) {
+        bitObject.validate();
+      }
+      if (!validate) {
+        bitObject.validateBeforePersist = false;
+      }
+    });
+  }
+
   async _deleteMany(): Promise<void> {
     if (!this.objectsToRemove.length) return;
     const uniqRefs = uniqBy(this.objectsToRemove, 'hash');