From 30840a4551f095db1fec0246e805f88f3c74100b Mon Sep 17 00:00:00 2001 From: Yonatan Date: Sun, 10 May 2020 00:08:55 +0300 Subject: [PATCH 1/8] Prevent error when name is null or undefined (fixes #8804) --- Source/DataSources/Entity.js | 6 +++++- Specs/DataSources/EntitySpec.js | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/Source/DataSources/Entity.js b/Source/DataSources/Entity.js index 119711736bce..f043d64816e1 100644 --- a/Source/DataSources/Entity.js +++ b/Source/DataSources/Entity.js @@ -109,7 +109,11 @@ function Entity(options) { this._availability = undefined; this._id = id; this._definitionChanged = new Event(); - this._name = options.name; + if (options.name !== undefined && options.name !== null) { + this._name = options.name; + } else { + delete options.name; + } this._show = defaultValue(options.show, true); this._parent = undefined; this._propertyNames = [ diff --git a/Specs/DataSources/EntitySpec.js b/Specs/DataSources/EntitySpec.js index 1c8ae3a5a0fe..e957efa3b9f3 100644 --- a/Specs/DataSources/EntitySpec.js +++ b/Specs/DataSources/EntitySpec.js @@ -27,6 +27,22 @@ import { RectangleGraphics } from "../../Source/Cesium.js"; import { WallGraphics } from "../../Source/Cesium.js"; describe("DataSources/Entity", function () { + it("should not throw when contructed with undefined or null name", function () { + var options = { + name: undefined, + }; + + expect(function () { + var entity = new Entity(options); + }).not.toThrowDeveloperError(); + + options.name = null; + + expect(function () { + var entity = new Entity(options); + }).not.toThrowDeveloperError(); + }); + it("constructor sets expected properties.", function () { var entity = new Entity(); expect(entity.id).toBeDefined(); From 436db93e19276bc72bf8306fd7f18e9e961d014c Mon Sep 17 00:00:00 2001 From: Yonatan Date: Sun, 10 May 2020 01:01:11 +0300 Subject: [PATCH 2/8] Linting, changelog and CONTRIBUTORS updates --- CHANGES.md | 1 + CONTRIBUTORS.md | 1 + Specs/DataSources/EntitySpec.js | 5 +++-- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fe3a917080f3..483bcb94e444 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,6 +9,7 @@ ##### Fixes :wrench: - This fixes a bug where a removed billboard can prevent changing of the terrainProvider [#8766](https://github.com/CesiumGS/cesium/pull/8766) +- THis fixes a but where null or undefined name property sent to Entity constructor threw an error [#8832](https://github.com/CesiumGS/cesium/pull/8832) ### 1.69.0 - 2020-05-01 diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 56a2e370dffe..d588a19ab4ee 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -256,3 +256,4 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu - [SungHo Lim](https://github.com/SambaLim) - [Michael Fink](https://github.com/vividos) - [Jakub Vrana](https://github.com/vrana) +- [Yonatan Kra](https://github.com/yonatankra) diff --git a/Specs/DataSources/EntitySpec.js b/Specs/DataSources/EntitySpec.js index e957efa3b9f3..adfb3e4aa939 100644 --- a/Specs/DataSources/EntitySpec.js +++ b/Specs/DataSources/EntitySpec.js @@ -28,18 +28,19 @@ import { WallGraphics } from "../../Source/Cesium.js"; describe("DataSources/Entity", function () { it("should not throw when contructed with undefined or null name", function () { + var entity; var options = { name: undefined, }; expect(function () { - var entity = new Entity(options); + entity = new Entity(options); }).not.toThrowDeveloperError(); options.name = null; expect(function () { - var entity = new Entity(options); + entity = new Entity(options); }).not.toThrowDeveloperError(); }); From 6136dfe59acc93992dd356f1a62f7e649d6bc20b Mon Sep 17 00:00:00 2001 From: Yonatan Date: Sun, 10 May 2020 01:20:53 +0300 Subject: [PATCH 3/8] Satisfy lint --- Specs/DataSources/EntitySpec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/Specs/DataSources/EntitySpec.js b/Specs/DataSources/EntitySpec.js index adfb3e4aa939..bd7f1f9c3031 100644 --- a/Specs/DataSources/EntitySpec.js +++ b/Specs/DataSources/EntitySpec.js @@ -28,6 +28,7 @@ import { WallGraphics } from "../../Source/Cesium.js"; describe("DataSources/Entity", function () { it("should not throw when contructed with undefined or null name", function () { + // eslint-disable-next-line no-unused-vars var entity; var options = { name: undefined, From c3e375d48b3385203b9148c69c86d03a42b8b0cc Mon Sep 17 00:00:00 2001 From: Yonatan Date: Sun, 10 May 2020 09:48:13 +0300 Subject: [PATCH 4/8] Typos in CHANGES.md --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 483bcb94e444..0c59360b85cd 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,7 +9,7 @@ ##### Fixes :wrench: - This fixes a bug where a removed billboard can prevent changing of the terrainProvider [#8766](https://github.com/CesiumGS/cesium/pull/8766) -- THis fixes a but where null or undefined name property sent to Entity constructor threw an error [#8832](https://github.com/CesiumGS/cesium/pull/8832) +- This fixes a bug where null or undefined name property sent to Entity constructor threw an error [#8832](https://github.com/CesiumGS/cesium/pull/8832) ### 1.69.0 - 2020-05-01 From de557373b753c77f4c619720bac11ae95b14a586 Mon Sep 17 00:00:00 2001 From: Yonatan Date: Tue, 12 May 2020 18:08:29 +0300 Subject: [PATCH 5/8] A better check for a an existing property --- Source/DataSources/Entity.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Source/DataSources/Entity.js b/Source/DataSources/Entity.js index f043d64816e1..81b63fce6a6a 100644 --- a/Source/DataSources/Entity.js +++ b/Source/DataSources/Entity.js @@ -109,11 +109,7 @@ function Entity(options) { this._availability = undefined; this._id = id; this._definitionChanged = new Event(); - if (options.name !== undefined && options.name !== null) { - this._name = options.name; - } else { - delete options.name; - } + this._show = defaultValue(options.show, true); this._parent = undefined; this._propertyNames = [ @@ -601,7 +597,7 @@ Entity.prototype.merge = function (source) { //Custom properties that are registered on the source entity must also //get registered on this entity. - if (!defined(targetProperty) && propertyNames.indexOf(name) === -1) { + if (!(name in this) && propertyNames.indexOf(name) === -1) { this.addProperty(name); } From 746055fa2bd3467d9e09b7d134f37572c1ae8f8e Mon Sep 17 00:00:00 2001 From: Yonatan Date: Tue, 12 May 2020 21:01:04 +0300 Subject: [PATCH 6/8] Better test description --- Specs/DataSources/EntitySpec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Specs/DataSources/EntitySpec.js b/Specs/DataSources/EntitySpec.js index bd7f1f9c3031..596cad2a637d 100644 --- a/Specs/DataSources/EntitySpec.js +++ b/Specs/DataSources/EntitySpec.js @@ -27,7 +27,7 @@ import { RectangleGraphics } from "../../Source/Cesium.js"; import { WallGraphics } from "../../Source/Cesium.js"; describe("DataSources/Entity", function () { - it("should not throw when contructed with undefined or null name", function () { + it("should not throw when constructed with undefined or null none object API property", function () { // eslint-disable-next-line no-unused-vars var entity; var options = { From 6daa2d91e1b29936fb70be1998c2d9f17b617c57 Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Mon, 29 Jun 2020 08:46:40 -0400 Subject: [PATCH 7/8] More specific fix for #8804 --- Source/DataSources/Entity.js | 9 ++++--- Specs/DataSources/EntitySpec.js | 46 ++++++++++++++++++++------------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/Source/DataSources/Entity.js b/Source/DataSources/Entity.js index 89f07bae1cf5..a542c1f6cd28 100644 --- a/Source/DataSources/Entity.js +++ b/Source/DataSources/Entity.js @@ -116,6 +116,7 @@ function Entity(options) { this._availability = undefined; this._id = id; this._definitionChanged = new Event(); + this._name = options.name; this._show = defaultValue(options.show, true); this._parent = undefined; @@ -594,8 +595,10 @@ Entity.prototype.merge = function (source) { for (var i = 0; i < propertyNamesLength; i++) { var name = sourcePropertyNames[i]; - //Ignore parent when merging, this only happens at construction time. - if (name === "parent") { + //While source is required by the API to be an Entity, we internally call this method from the + //constructor with an options object to configure initial custom properties. + //So we need to ignore reserved-non-property. + if (name === "parent" || name === "name" || name === "availability") { continue; } @@ -604,7 +607,7 @@ Entity.prototype.merge = function (source) { //Custom properties that are registered on the source entity must also //get registered on this entity. - if (!(name in this) && propertyNames.indexOf(name) === -1) { + if (!defined(targetProperty) && propertyNames.indexOf(name) === -1) { this.addProperty(name); } diff --git a/Specs/DataSources/EntitySpec.js b/Specs/DataSources/EntitySpec.js index 596cad2a637d..af2a06c54f84 100644 --- a/Specs/DataSources/EntitySpec.js +++ b/Specs/DataSources/EntitySpec.js @@ -27,24 +27,6 @@ import { RectangleGraphics } from "../../Source/Cesium.js"; import { WallGraphics } from "../../Source/Cesium.js"; describe("DataSources/Entity", function () { - it("should not throw when constructed with undefined or null none object API property", function () { - // eslint-disable-next-line no-unused-vars - var entity; - var options = { - name: undefined, - }; - - expect(function () { - entity = new Entity(options); - }).not.toThrowDeveloperError(); - - options.name = null; - - expect(function () { - entity = new Entity(options); - }).not.toThrowDeveloperError(); - }); - it("constructor sets expected properties.", function () { var entity = new Entity(); expect(entity.id).toBeDefined(); @@ -197,6 +179,34 @@ describe("DataSources/Entity", function () { } }); + it("merge does not overwrite availability", function () { + var entity = new Entity(); + + var entity2 = new Entity(); + var interval2 = TimeInterval.fromIso8601({ + iso8601: "2000-01-01/2001-01-01", + }); + entity2.availability = interval2; + + entity.merge(entity2); + expect(entity.availability).toBe(interval2); + }); + + it("merge ignores reserved property names when called with a plain object.", function () { + var entity = new Entity(); + + //Technically merge requires passing an Entity instance, but we call it internally + //with a plain object during construction to set up custom properties. + entity.merge({ + name: undefined, + availability: undefined, + parent: undefined, + }); + expect(entity.name).toBeUndefined(); + expect(entity.availability).toBeUndefined(); + expect(entity.parent).toBeUndefined(); + }); + it("merge does not overwrite availability", function () { var entity = new Entity(); var interval = TimeInterval.fromIso8601({ From 83c822010283e5bcd5635d2068fda44fe59d99e0 Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Mon, 29 Jun 2020 08:48:59 -0400 Subject: [PATCH 8/8] Remove duplicate test --- Source/DataSources/Entity.js | 1 - Specs/DataSources/EntitySpec.js | 13 ------------- 2 files changed, 14 deletions(-) diff --git a/Source/DataSources/Entity.js b/Source/DataSources/Entity.js index a542c1f6cd28..067effcd9170 100644 --- a/Source/DataSources/Entity.js +++ b/Source/DataSources/Entity.js @@ -117,7 +117,6 @@ function Entity(options) { this._id = id; this._definitionChanged = new Event(); this._name = options.name; - this._show = defaultValue(options.show, true); this._parent = undefined; this._propertyNames = [ diff --git a/Specs/DataSources/EntitySpec.js b/Specs/DataSources/EntitySpec.js index af2a06c54f84..b29c11ad8638 100644 --- a/Specs/DataSources/EntitySpec.js +++ b/Specs/DataSources/EntitySpec.js @@ -179,19 +179,6 @@ describe("DataSources/Entity", function () { } }); - it("merge does not overwrite availability", function () { - var entity = new Entity(); - - var entity2 = new Entity(); - var interval2 = TimeInterval.fromIso8601({ - iso8601: "2000-01-01/2001-01-01", - }); - entity2.availability = interval2; - - entity.merge(entity2); - expect(entity.availability).toBe(interval2); - }); - it("merge ignores reserved property names when called with a plain object.", function () { var entity = new Entity();