Adds function that inserts missing namespace into KML #5860

njrivera · 2017-09-29T14:40:22Z

This is based on an issue with some KML files having a 'xsi:schemaLocation' attribute in the Document tag without declaring the xsi namespace, causing the load to fail. The fix, so far, has been to edit the actual file manually, but many users/customers do not know how to. This function has a map with the key as the namespace as it would be used in the file ('xsi:') and a value as a 2 element array: element 0 contains the beginning of the namespace declaration ('xmlns:xsi=') and element 1 contains the urls for the declaration. This map can be updated with other namespaces that commonly cause the same issue. The new file - undeclaredNamespaces.kml includes this same issue and is used for the test.

cesium-concierge · 2017-09-29T14:40:23Z

Welcome to the Cesium community @nrivera-Novetta!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

pjcozzi · 2017-10-02T12:21:05Z

Thanks for the pull request, @nrivera-Novetta!

Do you work for Novetta and did you write this code for them? If so, we already have a CLA from Novetta that covers you.

pjcozzi · 2017-10-02T12:24:12Z

@nrivera-Novetta please add yourself to CONTRIBUTORS.md.

pjcozzi · 2017-10-02T12:32:55Z

@nrivera-Novetta please update CHANGES.md.

pjcozzi · 2017-10-02T12:33:02Z

@tfili can you review this?

tfili · 2017-10-09T18:54:00Z

Source/DataSources/KmlDataSource.js

@@ -255,6 +255,23 @@ define([
        return deferred.promise;
    }

+    function insertNamespaces(text) {
+        var namespaceMap = {


I think it would be cleaner to have the following then build the search strings in the loop.

var namespaceMap = { xsi : 'http://www.w3.org/2001/XMLSchema-instance' };

tfili · 2017-10-09T18:54:02Z

Source/DataSources/KmlDataSource.js

+        var firstPart;
+        var lastPart;
+
+        for (var key in namespaceMap){


XML doesn't allow for whitespace between < and the tag name, so you could check for <xsi:, so you don't inadvertantly match soemthing like <exsi:name>. In the loop you could check for a bad file like this

var prefix = '<' + key + ':'; var declaration = 'xmlns:' + key + '='; if(text.indexOf(prefix) !== -1 && text.indexOf(declaration) === -1) {

tfili · 2017-10-09T18:54:06Z

Source/DataSources/KmlDataSource.js

+
+        for (var key in namespaceMap){
+            if(text.indexOf(key) !== -1 && text.indexOf(namespaceMap[key][0]) === -1) {
+                firstPart = text.substr(0, text.indexOf('<kml') + 4);


You can find the index of <kml once before the loop, then you can just append to the firstPart as you iterate, then combine firstPart and lastPart after the loop.

You still only want to find <kml once, but you may want to do it in the loop after we found a problem, so we don't search a potentially long string if we don't need to. So something like this

var firstPart; var lastPart; for (var key in namespaceMap){ ... if(text.indexOf(prefix) !== -1 && text.indexOf(declaration) === -1) { if (!defined(firstPart)) { // Search for <kml and set firstPart/lastPart } ... } } if (defined(firstPart)) { text = firstPart + lastPart; } return text;

njrivera · 2017-10-10T13:28:09Z

@tfili with regard to what you said about xml not allowing for a space between a < and a tag, there is a special case that is identical to the one in my test kml in which the 'xsi:' is an attribute rather than a tag. It seems that there are a bunch of kml's floating around with this same xsi attribute and without any xsi tags in the document. I'm going to change the code to account for this and for what you mentioned and put the PR back up.

tfili · 2017-10-11T18:15:04Z

CHANGES.md

@@ -18,6 +18,7 @@ Change Log
 * Fixed a 3D Tiles point cloud bug causing a stray point to appear at the center of the screen on certain hardware. [#5599](https://github.com/AnalyticalGraphicsInc/cesium/issues/5599)
 * Fixed removing multiple event listeners within event callbacks. [#5827](https://github.com/AnalyticalGraphicsInc/cesium/issues/5827)
 * Running `buildApps` now creates a built version of Sandcastle which uses the built version of Cesium for better performance.
+* Added function that inserts missing namespace declarations into KML files. [#5860](https://github.com/AnalyticalGraphicsInc/cesium/pull/5860)


1.38 Came out already. Needs to be updated for 1.39 which will be released Nov 1.

tfili · 2017-10-11T18:28:20Z

Source/DataSources/KmlDataSource.js

+
+        for (var key in namespaceMap) {
+            if (namespaceMap.hasOwnProperty(key)) {
+                reg = RegExp('[<| ]' + key + ':');


[ ] specifies a character group where it represents any of the characters in it. This one will match <xsi:, xsi: or |xsi:. I think you were trying to use the OR syntax that you need in capture group (represented with ( )). So it can either be [< ] or (<| ). I think just removing the | from your code and going with the character group is the way to go as it doesn't have the overhead of capturing.

njrivera · 2017-10-18T18:05:47Z

@tfili hey just wondering if you could take another look at this

tfili · 2017-10-18T19:23:16Z

I'll take a look. @nrivera-Novetta, you need to add a comment when it is ready for another look or I won't be notified.

tfili

Besides the few small tweaks, this looks good.

Also, you don't have to close a PR to work on it. We won't merge it till all the tests pass and the comments have been addressed. Just comment when its ready.

Thanks again @nrivera-Novetta

tfili · 2017-10-18T19:26:14Z

Source/DataSources/KmlDataSource.js

+                reg = RegExp('[< ]' + key + ':');
+                declaration = 'xmlns:' + key + '=';
+                if (reg.test(text) && text.indexOf(declaration) === -1) {
+                    if (!firstPart) {


This should be

if (!defined(firstPart)) {

@tfili do you mean this line should actually use 'defined'? or just check if it's defined?

defined is a function in Cesium. It does some extra checking to make sure a value is actually undefined or null. This should be used in cases like this.

The magic of javascript's implicit type conversion would cause !firstpart to be true in other cases.

tfili · 2017-10-18T19:26:28Z

Source/DataSources/KmlDataSource.js

+            }
+        }
+
+        if (firstPart) {


Use defined here as well.

pjcozzi · 2017-10-18T21:29:20Z

Thanks again @nrivera-Novetta!

@tfili is this ready to merge? If so, please go ahead.

Nate Rivera added 8 commits September 27, 2017 12:54

added function to insert missing namespaces

912b44e

fixed spacing

42482ae

fixed quotes

ed093b5

added test for undeclared namespace

8c9fc84

fixed merged conflict

17c16d5

fixed spacing

debc70d

fixed spacing

1d424f8

fixed spacing

11e2e9d

pjcozzi mentioned this pull request Oct 2, 2017

Ask first-time contributors to add themselves to CONTRIBUTORS.md CesiumGS/cesium-concierge#84

Closed

pjcozzi requested a review from tfili October 2, 2017 12:33

ggetz mentioned this pull request Oct 2, 2017

Update CHANGES.md #5866

Closed

nrivera-Novetta added 7 commits October 2, 2017 11:52

Update CHANGES.md

8107abb

Update CHANGES.md

b1122d4

Update CONTRIBUTORS.md

c917ed9

Merge branch 'master' into master

18453b4

Update CHANGES.md

ff417d9

Merge branch 'master' into master

b0e69c3

fixing eslint errors

4a82295

tfili reviewed Oct 9, 2017

View reviewed changes

Nate Rivera added 3 commits October 9, 2017 17:42

optimized insertNamespaces

b6a2f81

fixing merge conflicts

4bc5ba4

fixed spacing

c0090b6

njrivera closed this Oct 9, 2017

Nate Rivera added 2 commits October 10, 2017 10:47

added regular expression for namespace key

d083f44

fixing conflict

c782808

njrivera reopened this Oct 10, 2017

Nate Rivera added 2 commits October 10, 2017 11:33

inserted missing character

24ee426

added property check

eafd921

tfili reviewed Oct 11, 2017

View reviewed changes

nrivera-Novetta added 3 commits October 12, 2017 11:07

removed extra character from regex

fb67eed

updated change to current release

379f27c

Merge branch 'master' into master

af60ced

tfili requested changes Oct 18, 2017

View reviewed changes

actually check if variable is defined

f65257d

juburr mentioned this pull request Oct 18, 2017

Fix KML ground overlay distortion bug #5915

Merged

Merge branch 'master' into master

83cd5f7

tfili approved these changes Oct 18, 2017

View reviewed changes

tfili merged commit 9b912da into CesiumGS:master Oct 18, 2017

njrivera mentioned this pull request Nov 27, 2017

Insert missing namespace declarations for KMZ files #6003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds function that inserts missing namespace into KML #5860

Adds function that inserts missing namespace into KML #5860

njrivera commented Sep 29, 2017

cesium-concierge commented Sep 29, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

tfili Oct 9, 2017

tfili Oct 9, 2017 •

edited

Loading

tfili Oct 9, 2017 •

edited

Loading

tfili Oct 9, 2017

njrivera commented Oct 10, 2017

tfili Oct 11, 2017

tfili Oct 11, 2017

njrivera commented Oct 18, 2017 •

edited

Loading

tfili commented Oct 18, 2017

tfili left a comment

tfili Oct 18, 2017

njrivera Oct 18, 2017 •

edited

Loading

tfili Oct 18, 2017

tfili Oct 18, 2017

pjcozzi commented Oct 18, 2017

Adds function that inserts missing namespace into KML #5860

Adds function that inserts missing namespace into KML #5860

Conversation

njrivera commented Sep 29, 2017

cesium-concierge commented Sep 29, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

tfili Oct 9, 2017

Choose a reason for hiding this comment

tfili Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

tfili Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

tfili Oct 9, 2017

Choose a reason for hiding this comment

njrivera commented Oct 10, 2017

tfili Oct 11, 2017

Choose a reason for hiding this comment

tfili Oct 11, 2017

Choose a reason for hiding this comment

njrivera commented Oct 18, 2017 • edited Loading

tfili commented Oct 18, 2017

tfili left a comment

Choose a reason for hiding this comment

tfili Oct 18, 2017

Choose a reason for hiding this comment

njrivera Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

tfili Oct 18, 2017

Choose a reason for hiding this comment

tfili Oct 18, 2017

Choose a reason for hiding this comment

pjcozzi commented Oct 18, 2017

tfili Oct 9, 2017 •

edited

Loading

tfili Oct 9, 2017 •

edited

Loading

njrivera commented Oct 18, 2017 •

edited

Loading

njrivera Oct 18, 2017 •

edited

Loading