Fixed save for case when a job has a shape and a track with the same id (#693)

* cvat-core fixed save for two object with the same id
* Added API test
* Fixed production code
* Dinamic shape types instead of hardcoded

cvat-core/src/annotations-saver.js

@@ -18,25 +18,35 @@
             this.id = session.id;
             this.version = version;
             this.collection = collection;
-            this.initialObjects = [];
+            this.initialObjects = {};
             this.hash = this._getHash();
 
             // We need use data from export instead of initialData
             // Otherwise we have differ keys order and JSON comparison code incorrect
             const exported = this.collection.export();
+
+            this._resetState();
             for (const shape of exported.shapes) {
-                this.initialObjects[shape.id] = shape;
+                this.initialObjects.shapes[shape.id] = shape;
             }
 
             for (const track of exported.tracks) {
-                this.initialObjects[track.id] = track;
+                this.initialObjects.tracks[track.id] = track;
             }
 
             for (const tag of exported.tags) {
-                this.initialObjects[tag.id] = tag;
+                this.initialObjects.tags[tag.id] = tag;
             }
         }
 
+        _resetState() {
+            this.initialObjects = {
+                shapes: {},
+                tracks: {},
+                tags: {},
+            };
+        }
+
         _getHash() {
             const exported = this.collection.export();
             return JSON.stringify(exported);
@@ -95,9 +105,9 @@
             // Find created and updated objects
             for (const type of Object.keys(exported)) {
                 for (const object of exported[type]) {
-                    if (object.id in this.initialObjects) {
+                    if (object.id in this.initialObjects[type]) {
                         const exportedHash = JSON.stringify(object);
-                        const initialHash = JSON.stringify(this.initialObjects[object.id]);
+                        const initialHash = JSON.stringify(this.initialObjects[type][object.id]);
                         if (exportedHash !== initialHash) {
                             splitted.updated[type].push(object);
                         }
@@ -113,24 +123,22 @@
             }
 
             // Now find deleted objects
-            const indexes = exported.tracks.concat(exported.shapes)
+            const indexes = {
-                .concat(exported.tags).map(object => object.id);
+                shapes: exported.shapes.map((object) => +object.id),
-
+                tracks: exported.tracks.map((object) => +object.id),
-            for (const id of Object.keys(this.initialObjects)) {
+                tags: exported.tags.map((object) => +object.id),
-                if (!indexes.includes(+id)) {
+            };
-                    const object = this.initialObjects[id];
+
-                    let type = null;
+            for (const type of Object.keys(this.initialObjects)) {
-                    if ('shapes' in object) {
+                for (const id of Object.keys(this.initialObjects[type])) {
-                        type = 'tracks';
+                    if (!indexes[type].includes(+id)) {
-                    } else if ('points' in object) {
+                        const object = this.initialObjects[type][id];
-                        type = 'shapes';
+                        splitted.deleted[type].push(object);
-                    } else {
-                        type = 'tags';
                     }
-                    splitted.deleted[type].push(object);
                 }
             }
 
+
             return splitted;
         }
 
@@ -164,9 +172,9 @@
         _receiveIndexes(exported) {
             // Receive client indexes before saving
             const indexes = {
-                tracks: exported.tracks.map(track => track.clientID),
+                tracks: exported.tracks.map((track) => track.clientID),
-                shapes: exported.shapes.map(shape => shape.clientID),
+                shapes: exported.shapes.map((shape) => shape.clientID),
-                tags: exported.tags.map(tag => tag.clientID),
+                tags: exported.tags.map((tag) => tag.clientID),
             };
 
             // Remove them from the request body
@@ -192,9 +200,7 @@
                 if (flush) {
                     onUpdate('New objects are being saved..');
                     const indexes = this._receiveIndexes(exported);
-                    const savedData = await this._put(Object.assign({}, exported, {
+                    const savedData = await this._put({ ...exported, version: this.version });
-                        version: this.version,
-                    }));
                     this.version = savedData.version;
                     this.collection.flush = false;
 
@@ -202,9 +208,12 @@
                     this._updateCreatedObjects(savedData, indexes);
 
                     onUpdate('Initial state is being updated');
-                    for (const object of savedData.shapes
+
-                        .concat(savedData.tracks).concat(savedData.tags)) {
+                    this._resetState();
-                        this.initialObjects[object.id] = object;
+                    for (const type of Object.keys(this.initialObjects)) {
+                        for (const object of savedData[type]) {
+                            this.initialObjects[type][object.id] = object;
+                        }
                     }
                 } else {
                     const {
@@ -215,44 +224,41 @@
 
                     onUpdate('New objects are being saved..');
                     const indexes = this._receiveIndexes(created);
-                    const createdData = await this._create(Object.assign({}, created, {
+                    const createdData = await this._create({ ...created, version: this.version });
-                        version: this.version,
-                    }));
                     this.version = createdData.version;
 
                     onUpdate('Saved objects are being updated in the client');
                     this._updateCreatedObjects(createdData, indexes);
 
                     onUpdate('Initial state is being updated');
-                    for (const object of createdData.shapes
+                    for (const type of Object.keys(this.initialObjects)) {
-                        .concat(createdData.tracks).concat(createdData.tags)) {
+                        for (const object of createdData[type]) {
-                        this.initialObjects[object.id] = object;
+                            this.initialObjects[type][object.id] = object;
+                        }
                     }
 
                     onUpdate('Changed objects are being saved..');
                     this._receiveIndexes(updated);
-                    const updatedData = await this._update(Object.assign({}, updated, {
+                    const updatedData = await this._update({ ...updated, version: this.version });
-                        version: this.version,
+                    this.version = updatedData.version;
-                    }));
-                    this.version = createdData.version;
 
                     onUpdate('Initial state is being updated');
-                    for (const object of updatedData.shapes
+                    for (const type of Object.keys(this.initialObjects)) {
-                        .concat(updatedData.tracks).concat(updatedData.tags)) {
+                        for (const object of updatedData[type]) {
-                        this.initialObjects[object.id] = object;
+                            this.initialObjects[type][object.id] = object;
+                        }
                     }
 
                     onUpdate('Changed objects are being saved..');
                     this._receiveIndexes(deleted);
-                    const deletedData = await this._delete(Object.assign({}, deleted, {
+                    const deletedData = await this._delete({ ...deleted, version: this.version });
-                        version: this.version,
-                    }));
                     this._version = deletedData.version;
 
                     onUpdate('Initial state is being updated');
-                    for (const object of deletedData.shapes
+                    for (const type of Object.keys(this.initialObjects)) {
-                        .concat(deletedData.tracks).concat(deletedData.tags)) {
+                        for (const object of deletedData[type]) {
-                        delete this.initialObjects[object.id];
+                            delete this.initialObjects[type][object.id];
+                        }
                     }
                 }
 

cvat-core/tests/api/annotations.js

@@ -17,6 +17,7 @@ jest.mock('../../src/server-proxy', () => {
 
 // Initialize api
 window.cvat = require('../../src/api');
+const serverProxy = require('../../src/server-proxy');
 
 // Test cases
 describe('Feature: get annotations', () => {
@@ -373,6 +374,30 @@ describe('Feature: save annotations', () => {
         await job.annotations.save();
         expect(await job.annotations.hasUnsavedChanges()).toBe(false);
     });
+
+    test('delete & save annotations for a job when there are a track and a shape with the same id', async () => {
+        const job = (await window.cvat.jobs.get({ jobID: 112 }))[0];
+        const annotations = await job.annotations.get(0);
+        let okay = false;
+
+        // Temporary override this method because we need to know what data
+        // have been sent to a server
+        const oldImplementation = serverProxy.annotations.updateAnnotations;
+        serverProxy.annotations.updateAnnotations = async (session, id, data, action) => {
+            const result = await oldImplementation
+                .call(serverProxy.annotations, session, id, data, action);
+            if (action === 'delete') {
+                okay = okay || (action === 'delete' && !!(data.shapes.length || data.tracks.length));
+            }
+            return result;
+        };
+
+        await annotations[0].delete();
+        await job.annotations.save();
+
+        serverProxy.annotations.updateAnnotations = oldImplementation;
+        expect(okay).toBe(true);
+    });
 });
 
 describe('Feature: merge annotations', () => {
@@ -383,9 +408,9 @@ describe('Feature: merge annotations', () => {
         const states = [annotations0[0], annotations1[0]];
         await task.annotations.merge(states);
         const merged0 = (await task.annotations.get(0))
-            .filter(state => state.objectType === window.cvat.enums.ObjectType.TRACK);
+            .filter((state) => state.objectType === window.cvat.enums.ObjectType.TRACK);
         const merged1 = (await task.annotations.get(1))
-            .filter(state => state.objectType === window.cvat.enums.ObjectType.TRACK);
+            .filter((state) => state.objectType === window.cvat.enums.ObjectType.TRACK);
         expect(merged0).toHaveLength(1);
         expect(merged1).toHaveLength(1);
 
@@ -400,9 +425,9 @@ describe('Feature: merge annotations', () => {
         const states = [annotations0[0], annotations1[0]];
         await job.annotations.merge(states);
         const merged0 = (await job.annotations.get(0))
-            .filter(state => state.objectType === window.cvat.enums.ObjectType.TRACK);
+            .filter((state) => state.objectType === window.cvat.enums.ObjectType.TRACK);
         const merged1 = (await job.annotations.get(1))
-            .filter(state => state.objectType === window.cvat.enums.ObjectType.TRACK);
+            .filter((state) => state.objectType === window.cvat.enums.ObjectType.TRACK);
         expect(merged0).toHaveLength(1);
         expect(merged1).toHaveLength(1);
 
@@ -456,7 +481,7 @@ describe('Feature: merge annotations', () => {
         const task = (await window.cvat.tasks.get({ id: 100 }))[0];
         const annotations0 = await task.annotations.get(0);
         const annotations1 = (await task.annotations.get(1))
-            .filter(state => state.shapeType === window.cvat.enums.ObjectShape.POLYGON);
+            .filter((state) => state.shapeType === window.cvat.enums.ObjectShape.POLYGON);
         const states = [annotations0[0], annotations1[0]];
 
         expect(task.annotations.merge(states))
@@ -488,7 +513,7 @@ describe('Feature: split annotations', () => {
         expect(annotations4[0].clientID).toBe(annotations5[0].clientID);
         await task.annotations.split(annotations5[0], 5);
         const splitted4 = await task.annotations.get(4);
-        const splitted5 = (await task.annotations.get(5)).filter(state => !state.outside);
+        const splitted5 = (await task.annotations.get(5)).filter((state) => !state.outside);
         expect(splitted4[0].clientID).not.toBe(splitted5[0].clientID);
     });
 
@@ -500,7 +525,7 @@ describe('Feature: split annotations', () => {
         expect(annotations4[0].clientID).toBe(annotations5[0].clientID);
         await job.annotations.split(annotations5[0], 5);
         const splitted4 = await job.annotations.get(4);
-        const splitted5 = (await job.annotations.get(5)).filter(state => !state.outside);
+        const splitted5 = (await job.annotations.get(5)).filter((state) => !state.outside);
         expect(splitted4[0].clientID).not.toBe(splitted5[0].clientID);
     });
 

cvat-core/tests/api/tasks.js

@@ -26,7 +26,7 @@ describe('Feature: get a list of tasks', () => {
     test('get all tasks', async () => {
         const result = await window.cvat.tasks.get();
         expect(Array.isArray(result)).toBeTruthy();
-        expect(result).toHaveLength(5);
+        expect(result).toHaveLength(6);
         for (const el of result) {
             expect(el).toBeInstanceOf(Task);
         }
@@ -137,7 +137,7 @@ describe('Feature: save a task', () => {
         });
 
         expect(result[0].labels).toHaveLength(labelsLength + 1);
-        const appendedLabel = result[0].labels.filter(el => el.name === 'My boss\'s car');
+        const appendedLabel = result[0].labels.filter((el) => el.name === 'My boss\'s car');
         expect(appendedLabel).toHaveLength(1);
         expect(appendedLabel[0].attributes).toHaveLength(1);
         expect(appendedLabel[0].attributes[0].name).toBe('parked');

cvat-core/tests/mocks/dummy-data.mock.js

@@ -185,12 +185,46 @@ const shareDummyData = [
 ]
 
 const tasksDummyData = {
-    "count": 4,
+    "count": 5,
     "next": null,
     "previous": null,
     "results": [
         {
-            "url": "http://localhost:7000/api/v1/tasks/1",
+            "url": "http://localhost:7000/api/v1/tasks/102",
+            "id": 102,
+            "name": "Test",
+            "size": 1,
+            "mode": "annotation",
+            "owner": 1,
+            "assignee": null,
+            "bug_tracker": "",
+            "created_date": "2019-09-05T11:59:22.987942Z",
+            "updated_date": "2019-09-05T14:04:07.569344Z",
+            "overlap": 0,
+            "segment_size": 0,
+            "z_order": false,
+            "status": "annotation",
+            "labels": [{
+                "id": 5,
+                "name": "car",
+                "attributes": []
+            }],
+            "segments": [{
+                "start_frame": 0,
+                "stop_frame": 0,
+                "jobs": [{
+                    "url":"http://localhost:7000/api/v1/jobs/112",
+                    "id": 112,
+                    "assignee":null,
+                    "status":"annotation"
+                }]
+            }],
+            "image_quality": 50,
+            "start_frame": 0,
+            "stop_frame": 0,
+            "frame_filter": ""
+        }, {
+            "url": "http://localhost:7000/api/v1/tasks/100",
             "id": 100,
             "name": "Image Task",
             "size": 9,
@@ -226,7 +260,7 @@ const tasksDummyData = {
                   "stop_frame": 8,
                   "jobs": [
                      {
-                        "url": "http://localhost:7000/api/v1/jobs/1",
+                        "url": "http://localhost:7000/api/v1/jobs/100",
                         "id": 100,
                         "assignee": null,
                         "status": "annotation"
@@ -1387,6 +1421,48 @@ const tasksDummyData = {
 }
 
 const taskAnnotationsDummyData = {
+    '112': {
+        "version":21,
+        "tags": [],
+        "shapes": [{
+            "type": "rectangle",
+            "occluded": false,
+            "z_order": 1,
+            "points": [
+                557.7890625,
+                276.2216796875,
+                907.1888732910156,
+                695.5014038085938
+            ],
+            "id": 15,
+            "frame": 0,
+            "label_id": 5,
+            "group": 0,
+            "attributes": []
+        }],
+        "tracks": [{
+            "id": 15,
+            "frame": 0,
+            "label_id": 5,
+            "group": 0,
+            "shapes": [{
+                "type": "rectangle",
+                "occluded": false,
+                "z_order": 13,
+                "points": [
+                    792.787109375,
+                    16.5234375,
+                    1171.1027526855469,
+                    521.3458862304688
+                ],
+                "id": 22,
+                "frame": 0,
+                "outside": false,
+                "attributes": []
+            }],
+            "attributes": []
+        }]
+    },
     '101': {
         "version":21,
         "tags":[],
@@ -2514,6 +2590,10 @@ const frameMetaDummyData = {
           "width": 1888,
           "height": 1408
     }],
+    102: [{
+        "width":1920,
+        "height":1080
+    }],
 }
 
 module.exports = {

cvat-core/tests/mocks/server-proxy.mock.js

@@ -287,11 +287,12 @@ class ServerProxy {
             },
 
             annotations: {
-                value: Object.freeze({
+                value: {
                     updateAnnotations,
                     getAnnotations,
-                }),
+                },
-                writable: false,
+                // To implement on of important tests
+                writable: true,
             },
         }));
     }

cvat/apps/engine/static/engine/js/annotationSaver.js

@@ -20,22 +20,30 @@ class AnnotationSaverModel extends Listener {
 
         this._version = initialData.version;
         this._shapeCollection = shapeCollection;
-        this._initialObjects = [];
+        this._initialObjects = {};
 
+        this._resetState();
         this.update();
 
         // We need use data from export instead of initialData
         // Otherwise we have differ keys order and JSON comparison code incorrect
         const data = this._shapeCollection.export()[0];
         for (const shape of data.shapes) {
-            this._initialObjects[shape.id] = shape;
+            this._initialObjects.shapes[shape.id] = shape;
         }
 
         for (const track of data.tracks) {
-            this._initialObjects[track.id] = track;
+            this._initialObjects.tracks[track.id] = track;
         }
     }
 
+    _resetState() {
+        this._initialObjects = {
+            shapes: {},
+            tracks: {},
+        };
+    }
+
     update() {
         this._hash = this._getHash();
     }
@@ -122,9 +130,6 @@ class AnnotationSaverModel extends Listener {
     }
 
     _split(exported) {
-        const exportedIDs = Array.from(exported.shapes, shape => +shape.id)
-            .concat(Array.from(exported.tracks, track => +track.id));
-
         const created = {
             version: this._version,
             shapes: [],
@@ -148,30 +153,36 @@ class AnnotationSaverModel extends Listener {
 
         // Compare initial state objects and export state objects
         // in order to get updated and created objects
-        for (const obj of exported.shapes.concat(exported.tracks)) {
+        for (const type of Object.keys(this._initialObjects)) {
-            if (obj.id in this._initialObjects) {
+            for (const obj of exported[type]) {
-                const exportedHash = JSON.stringify(obj);
+                if (obj.id in this._initialObjects[type]) {
-                const initialSash = JSON.stringify(this._initialObjects[obj.id]);
+                    const exportedHash = JSON.stringify(obj);
-                if (exportedHash !== initialSash) {
+                    const initialSash = JSON.stringify(this._initialObjects[type][obj.id]);
-                    const target = 'shapes' in obj ? updated.tracks : updated.shapes;
+                    if (exportedHash !== initialSash) {
-                    target.push(obj);
+                        updated[type].push(obj);
+                    }
+                } else if (typeof obj.id === 'undefined') {
+                    created[type].push(obj);
+                } else {
+                    throw Error(`Bad object ID found: ${obj.id}. `
+                        + 'It is not contained in initial state and have server ID');
                 }
-            } else if (typeof obj.id === 'undefined') {
-                const target = 'shapes' in obj ? created.tracks : created.shapes;
-                target.push(obj);
-            } else {
-                throw Error(`Bad object ID found: ${obj.id}. `
-                    + 'It is not contained in initial state and have server ID');
             }
         }
 
+        const indexes = {
+            shapes: exported.shapes.map((object) => +object.id),
+            tracks: exported.tracks.map((object) => +object.id),
+        };
+
         // Compare initial state indexes and export state indexes
         // in order to get removed objects
-        for (const shapeID in this._initialObjects) {
+        for (const type of Object.keys(this._initialObjects)) {
-            if (!exportedIDs.includes(+shapeID)) {
+            for (const shapeID in this._initialObjects[type]) {
-                const initialShape = this._initialObjects[shapeID];
+                if (!indexes[type].includes(+shapeID)) {
-                const target = 'shapes' in initialShape ? deleted.tracks : deleted.shapes;
+                    const object = this._initialObjects[type][shapeID];
-                target.push(initialShape);
+                    deleted[type].push(object);
+                }
             }
         }
 
@@ -229,10 +240,12 @@ class AnnotationSaverModel extends Listener {
                 this._updateCreatedObjects(exported, savedObjects, mapping);
                 this._shapeCollection.flush = false;
                 this._version = savedObjects.version;
-                for (const object of savedObjects.shapes.concat(savedObjects.tracks)) {
+                this._resetState();
-                    this._initialObjects[object.id] = object;
+                for (const type of Object.keys(this._initialObjects)) {
+                    for (const object of savedObjects[type]) {
+                        this._initialObjects[type][object.id] = object;
+                    }
                 }
-
                 this._version = savedObjects.version;
             } else {
                 const [created, updated, deleted] = this._split(exported);
@@ -240,25 +253,27 @@ class AnnotationSaverModel extends Listener {
                 const savedCreated = await this._create(created);
                 this._updateCreatedObjects(created, savedCreated, mapping);
                 this._version = savedCreated.version;
-                for (const object of created.shapes.concat(created.tracks)) {
+                for (const type of Object.keys(this._initialObjects)) {
-                    this._initialObjects[object.id] = object;
+                    for (const object of savedCreated[type]) {
+                        this._initialObjects[type][object.id] = object;
+                    }
                 }
 
                 this.notify('saveUpdated');
                 const savedUpdated = await this._update(updated);
                 this._version = savedUpdated.version;
-                for (const object of updated.shapes.concat(updated.tracks)) {
+                for (const type of Object.keys(this._initialObjects)) {
-                    if (object.id in this._initialObjects) {
+                    for (const object of savedUpdated[type]) {
-                        this._initialObjects[object.id] = object;
+                        this._initialObjects[type][object.id] = object;
                     }
                 }
 
                 this.notify('saveDeleted');
                 const savedDeleted = await this._delete(deleted);
                 this._version = savedDeleted.version;
-                for (const object of savedDeleted.shapes.concat(savedDeleted.tracks)) {
+                for (const type of Object.keys(this._initialObjects)) {
-                    if (object.id in this._initialObjects) {
+                    for (const object of savedDeleted[type]) {
-                        delete this._initialObjects[object.id];
+                        delete this._initialObjects[type][object.id];
                     }
                 }