Merge pull request #442 from Microsoft/develop

Merge develop to master for 1.0.6

Library/EnvelopeFactory.ts

@@ -330,4 +330,4 @@ class _StackFrame {
     }
 }
 
-export = EnvelopeFactory;
+export = EnvelopeFactory;

Library/Sender.ts

@@ -224,7 +224,10 @@ class Sender {
             return callback(null, Sender.ACL_IDENTITY);
         }
         var psProc = child_process.spawn(Sender.POWERSHELL_PATH, 
-            ["-Command", "[System.Security.Principal.WindowsIdentity]::GetCurrent().Name"], <any>{windowsHide: true});
+            ["-Command", "[System.Security.Principal.WindowsIdentity]::GetCurrent().Name"], <any>{
+                windowsHide: true,
+                stdio: ['ignore', 'pipe', 'pipe'] // Needed to prevent hanging on Win 7
+            });
         let data = "";
         psProc.stdout.on("data", (d: string) => data += d);
         psProc.on("error", (e: Error) => callback(e, null));
@@ -243,7 +246,10 @@ class Sender {
         // Some very old versions of Node (< 0.11) don't have this
         if (child_process.spawnSync) {
             var psProc = child_process.spawnSync(Sender.POWERSHELL_PATH, 
-                ["-Command", "[System.Security.Principal.WindowsIdentity]::GetCurrent().Name"], <any>{windowsHide: true});
+                ["-Command", "[System.Security.Principal.WindowsIdentity]::GetCurrent().Name"], <any>{
+                    windowsHide: true,
+                    stdio: ['ignore', 'pipe', 'pipe'] // Needed to prevent hanging on Win 7
+                });
             if (psProc.error) {
                 throw psProc.error;
             } else if (psProc.status !== 0) {
@@ -270,6 +276,11 @@ class Sender {
 
         // For performance, only run ACL rules if we haven't already during this session
         if (Sender.ACLED_DIRECTORIES[directory] === undefined) {
+            // Avoid multiple calls race condition by setting ACLED_DIRECTORIES to false for this directory immediately
+            // If batches are being failed faster than the processes spawned below return, some data won't be stored to disk
+            // This is better than the alternative of potentially infinitely spawned processes
+            Sender.ACLED_DIRECTORIES[directory] = false;
+
             // Restrict this directory to only current user and administrator access
             this._getACLIdentity((err, identity) => {
                 if (err) {

Library/Util.ts

@@ -117,6 +117,11 @@ class Util {
         return obj instanceof Error;
     }
 
+    public static isPrimitive(input: any): boolean {
+        const propType = typeof input;
+        return propType === "string" || propType === "number" || propType === "boolean";
+    }
+
     /**
      * Check if an object is of type Date
      */
@@ -145,34 +150,77 @@ class Util {
         return daysText + hour + ":" + min + ":" + sec;
     }
 
+    /**
+     * Using JSON.stringify, by default Errors do not serialize to something useful:
+     * Simplify a generic Node Error into a simpler map for customDimensions
+     * Custom errors can still implement toJSON to override this functionality
+     */
+    protected static extractError(err: Error): { message: string, code: string } {
+        // Error is often subclassed so may have code OR id properties:
+        // https://nodejs.org/api/errors.html#errors_error_code
+        const looseError = err as any;
+        return {
+            message: err.message,
+            code: looseError.code || looseError.id || "",
+        }
+    }
+
+    /**
+     * Manually call toJSON if available to pre-convert the value.
+     * If a primitive is returned, then the consumer of this function can skip JSON.stringify.
+     * This avoids double escaping of quotes for Date objects, for example.
+     */
+    protected static extractObject(origProperty: any): any {
+        if (origProperty instanceof Error) {
+            return Util.extractError(origProperty);
+        }
+        if (typeof origProperty.toJSON === "function") {
+            return origProperty.toJSON();
+        }
+        return origProperty;
+    }
+
     /**
      * Validate that an object is of type { [key: string]: string }
      */
     public static validateStringMap(obj: any): { [key: string]: string } {
-        var map: { [key: string]: string };
-        if(typeof obj === "object") {
-            map = <{ [key: string]: string }>{};
-            for (var field in obj) {
-                var property = obj[field];
-                var propertyType = typeof property;
-                if (propertyType !== "string") {
-                    if (property != null && typeof property.toString === "function") {
-                        property = property.toString();
+        if(typeof obj !== "object") {
+            Logging.info("Invalid properties dropped from payload");
+            return;
+        }
+        const map: { [key: string]: string } = {};
+        for (let field in obj) {
+            let property: string = '';
+            const origProperty: any = obj[field];
+            const propType = typeof origProperty;
+
+            if (Util.isPrimitive(origProperty)) {
+                property = origProperty.toString();
+            } else if (origProperty === null || propType === "undefined") {
+                property = "";
+            } else if (propType === "function") {
+                Logging.info("key: " + field + " was function; will not serialize");
+                continue;
+            } else {
+                const stringTarget = Util.isArray(origProperty) ? origProperty : Util.extractObject(origProperty);
+                try {
+                    if (Util.isPrimitive(stringTarget)) {
+                        property = stringTarget;
                     } else {
-                        Logging.info("key: " + field + ", invalid property type: " + propertyType);
-                        continue;
+                        property = JSON.stringify(stringTarget);
                     }
+                } catch (e) {
+                    property = origProperty.constructor.name.toString() + " (Error: " + e.message + ")";
+                    Logging.info("key: " + field + ", could not be serialized");
                 }
-
-                map[field] = property.trim(0, Util.MAX_PROPERTY_LENGTH);
             }
-        } else {
-            Logging.info("Invalid properties dropped from payload");
-        }
 
+            map[field] = property.substring(0, Util.MAX_PROPERTY_LENGTH);
+        }
         return map;
     }
 
+
     /**
      * Checks if a request url is not on a excluded domain list
      * and if it is safe to add correlation headers
@@ -283,4 +331,4 @@ class Util {
 
     };
 }
-export = Util;
+export = Util;

Tests/EndToEnd.tests.ts

@@ -527,6 +527,63 @@ describe("EndToEnd", () => {
             });
         });
 
+        it("refuses to query for ACL identity twice (process never returned)", (done) => {
+            spawn.restore();
+            var tempSpawn = sinon.stub(child_process, 'spawn').returns({
+                on: (type: string, cb: any) => {
+                    return; // do nothing
+                },
+                stdout: {
+                    on: (type: string, cb: any) => {
+                        return; // do nothing
+                    }
+                }
+            });
+
+            var req = new fakeRequest();
+
+            var client = new AppInsights.TelemetryClient("uniquekey");
+            client.channel.setUseDiskRetryCaching(true);
+            var origICACLS = (<any>client.channel._sender.constructor).USE_ICACLS;
+            (<any>client.channel._sender.constructor).USE_ICACLS = true; // Simulate ICACLS environment even on *nix
+
+            // Set ICACLS caches for test purposes
+            (<any>client.channel._sender.constructor).ACL_IDENTITY = null;
+            (<any>client.channel._sender.constructor).ACLED_DIRECTORIES = {};
+
+            client.trackEvent({ name: "test event" });
+
+            this.request.returns(req);
+
+            client.flush({
+                callback: (response: any) => {
+                    // yield for the caching behavior
+                    setImmediate(() => {
+                        assert(writeFile.callCount === 0);
+                        assert.equal(tempSpawn.callCount, 1);
+
+                        client.trackEvent({ name: "test event" });
+                        this.request.returns(req);
+
+                        client.flush({
+                            callback: (response: any) => {
+                                // yield for the caching behavior
+                                setImmediate(() => {
+                                    // The call counts shouldnt have changed
+                                    assert(writeFile.callCount === 0);
+                                    assert.equal(tempSpawn.callCount, 1);
+
+                                    tempSpawn.restore();
+                                    (<any>client.channel._sender.constructor).USE_ICACLS = origICACLS;
+                                    done();
+                                });
+                            }
+                        });
+                    });
+                }
+            });
+        });
+
         it("refuses to store data if ICACLS fails", (done) => {
             spawn.restore();
             var tempSpawn = sinon.stub(child_process, 'spawn').returns({

Tests/Library/Util.tests.ts

@@ -184,16 +184,61 @@ describe("Library/Util", () => {
         });
     });
 
-    describe("#enforceStringMap", () => {
+    describe("#validateStringMap", () => {
         it("should only allow string:string", () => {
             assert.equal(Util.validateStringMap(undefined), undefined);
             assert.equal(Util.validateStringMap(1), undefined);
             assert.equal(Util.validateStringMap(true), undefined);
             assert.equal(Util.validateStringMap("test"), undefined);
             assert.equal(Util.validateStringMap(():void => null), undefined);
-            assert.deepEqual(Util.validateStringMap({ a: {} }), { a: "[object Object]" });
+            assert.deepEqual(Util.validateStringMap({ a: {} }), { a: "{}" });
             assert.deepEqual(Util.validateStringMap({ a: 3, b: "test" }), { a: "3", b: "test" });
-            assert.deepEqual(Util.validateStringMap({ a: 0, b: null, c: undefined, d: [], e: '', f: -1 }), { a: "0", d: "", e: "", f: "-1" });
+            assert.deepEqual(Util.validateStringMap({ a: 0, b: null, c: undefined, d: [], e: '', f: -1, g: true, h: false }), { a: "0", b: "", c: "", d: "[]", e: "", f: "-1", g: "true", h: "false" });
+            assert.deepEqual(Util.validateStringMap({ d: new Date("1995-12-17T03:24:00") }), { d: new Date("1995-12-17T03:24:00").toJSON() });
+        });
+        it("skips functions", () => {
+            assert.deepEqual(Util.validateStringMap({ f: function () { } }), { });
+        });
+        it("should gracefully handle errors", () => {
+            const vanillaError = new Error("Test userland error");
+            const mapped = Util.validateStringMap({ error: vanillaError });
+            const stringMapped = JSON.parse(mapped.error);
+            assert.equal(stringMapped.message, "Test userland error");
+            assert.equal(stringMapped.stack, undefined);
+            assert.equal(stringMapped.code, "");
+            const errorWithCode = new Error("Test error with code");
+            (errorWithCode as any).code = 418;
+            const idMapped = Util.validateStringMap({ error: errorWithCode });
+            assert.equal(JSON.parse(idMapped.error).code, 418);
+        });
+        it("supports object and string .toJSON return values", () => {
+            const complex = {
+                secret: "private",
+                isPublic: "public",
+                toJSON: function() {
+                    return {
+                        isPublic: this.isPublic,
+                    }
+                },
+            };
+            const d = new Date(1971, 5, 28);
+            const mapped = Util.validateStringMap({ date: d, complex });
+            assert.deepEqual(JSON.parse(mapped.complex), { isPublic: "public" });
+            assert.equal(mapped.date, d.toJSON());
+        });
+        it("should handle circular references", () => {
+            const circObj = <{[key: string]: any}>{};
+            circObj.test = true;
+            circObj.circular = circObj;
+            circObj.arr = [0, 1, circObj.circular];
+            assert.deepEqual(
+                Util.validateStringMap(circObj),
+                {
+                    test: "true",
+                    circular: "Object (Error: Converting circular structure to JSON)",
+                    arr: "Array (Error: Converting circular structure to JSON)",
+                }
+            );
         });
     });
 

package.json

@@ -2,7 +2,7 @@
   "name": "applicationinsights",
   "license": "MIT",
   "bugs": "https://github.com/Microsoft/ApplicationInsights-node.js/issues",
-  "version": "1.0.5",
+  "version": "1.0.6",
   "description": "Microsoft Application Insights module for Node.js",
   "repository": {
     "type": "git",