From 0f15482fcfaa89619e3e47a41c3a54291e1b0e56 Mon Sep 17 00:00:00 2001 From: Matt Robenolt Date: Mon, 14 Sep 2015 11:05:06 -0700 Subject: [PATCH 1/2] Refactor makeRequest to prepare to be overridden --- src/raven.js | 52 +++++++++++--------- test/raven.test.js | 120 +++++++++++++++++++++++++++++++++------------ 2 files changed, 118 insertions(+), 54 deletions(-) diff --git a/src/raven.js b/src/raven.js index df66adb37e34..e956a74e8095 100644 --- a/src/raven.js +++ b/src/raven.js @@ -748,35 +748,41 @@ function send(data) { // Set lastEventId after we know the error should actually be sent lastEventId = data.event_id || (data.event_id = uuid4()); - makeRequest(data); -} + logDebug('debug', 'Raven about to send:', data); + if (!isSetup()) return; -function makeRequest(data) { - var img, - src; + var url = globalServer + authQueryString; - logDebug('debug', 'Raven about to send:', data); + makeRequest({ + url: url, + data: data, + options: globalOptions, + onSuccess: function success() { + triggerEvent('success', { + data: data, + src: url + }); + }, + onError: function failure() { + triggerEvent('failure', { + data: data, + src: url + }); + } + }); +} - if (!isSetup()) return; - img = newImage(); - src = globalServer + authQueryString + '&sentry_data=' + encodeURIComponent(JSON.stringify(data)); - if (globalOptions.crossOrigin || globalOptions.crossOrigin === '') { - img.crossOrigin = globalOptions.crossOrigin; +function makeRequest(opts) { + var img = newImage(), + src = opts.url + '&sentry_data=' + encodeURIComponent(JSON.stringify(opts.data)); + + if (opts.options.crossOrigin || opts.options.crossOrigin === '') { + img.crossOrigin = opts.options.crossOrigin; } - img.onload = function success() { - triggerEvent('success', { - data: data, - src: src - }); - }; - img.onerror = img.onabort = function failure() { - triggerEvent('failure', { - data: data, - src: src - }); - }; + img.onload = opts.onSuccess; + img.onerror = img.onabort = opts.onError; img.src = src; } diff --git a/test/raven.test.js b/test/raven.test.js index 6c55c5f10916..52e1f2544391 100644 --- a/test/raven.test.js +++ b/test/raven.test.js @@ -881,7 +881,7 @@ describe('globals', function() { }; send({foo: 'bar'}); - assert.deepEqual(window.makeRequest.lastCall.args[0], { + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', logger: 'javascript', platform: 'javascript', @@ -913,7 +913,7 @@ describe('globals', function() { globalUser = {name: 'Matt'}; send({foo: 'bar'}); - assert.deepEqual(window.makeRequest.lastCall.args, [{ + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', logger: 'javascript', platform: 'javascript', @@ -929,7 +929,7 @@ describe('globals', function() { }, foo: 'bar', extra: {'session:duration': 100} - }]); + }); }); it('should merge in global tags', function() { @@ -948,7 +948,7 @@ describe('globals', function() { send({tags: {tag2: 'value2'}}); - assert.deepEqual(window.makeRequest.lastCall.args, [{ + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', logger: 'javascript', platform: 'javascript', @@ -961,7 +961,7 @@ describe('globals', function() { event_id: 'abc123', tags: {tag1: 'value1', tag2: 'value2'}, extra: {'session:duration': 100} - }]); + }); assert.deepEqual(globalOptions, { logger: 'javascript', tags: {tag1: 'value1'} @@ -984,7 +984,7 @@ describe('globals', function() { send({extra: {key2: 'value2'}}); - assert.deepEqual(window.makeRequest.lastCall.args, [{ + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', logger: 'javascript', platform: 'javascript', @@ -996,7 +996,7 @@ describe('globals', function() { }, event_id: 'abc123', extra: {key1: 'value1', key2: 'value2', 'session:duration': 100} - }]); + }); assert.deepEqual(globalOptions, { logger: 'javascript', extra: {key1: 'value1'} @@ -1018,10 +1018,10 @@ describe('globals', function() { globalUser = {name: 'Matt'}; send({foo: 'bar'}); - assert.deepEqual(window.makeRequest.lastCall.args, [{ + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { lol: 'ibrokeit', event_id: 'abc123', - }]); + }); }); it('should ignore dataCallback if it does not return anything', function() { @@ -1041,7 +1041,7 @@ describe('globals', function() { }; send({foo: 'bar'}); - assert.deepEqual(window.makeRequest.lastCall.args[0], { + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', logger: 'javascript', platform: 'javascript', @@ -1072,7 +1072,7 @@ describe('globals', function() { }; send({foo: 'bar', tags: {}, extra: {}}); - assert.deepEqual(window.makeRequest.lastCall.args[0], { + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', logger: 'javascript', platform: 'javascript', @@ -1103,7 +1103,7 @@ describe('globals', function() { }; send({foo: 'bar'}); - assert.deepEqual(window.makeRequest.lastCall.args[0], { + assert.deepEqual(window.makeRequest.lastCall.args[0].data, { project: '2', release: 'abc123', logger: 'javascript', @@ -1119,40 +1119,86 @@ describe('globals', function() { extra: {'session:duration': 100} }); }); - }); - describe('makeRequest', function() { - var imageCache; + it('should pass correct opts to makeRequest', function() { + this.sinon.stub(window, 'isSetup').returns(true); + this.sinon.stub(window, 'makeRequest'); + this.sinon.stub(window, 'getHttpData').returns({ + url: 'http://localhost/?a=b', + headers: {'User-Agent': 'lolbrowser'} + }); - beforeEach(function () { - imageCache = []; - this.sinon.stub(window, 'newImage', function(){ var img = {}; imageCache.push(img); return img; }); - }) + globalServer = 'http://localhost/store/'; + authQueryString = '?lol' + + globalOptions = { + projectId: 2, + logger: 'javascript', + release: 'abc123', + }; + + send({foo: 'bar'}); + var args = window.makeRequest.lastCall.args; + assert.equal(args.length, 1); + var opts = args[0]; + assert.equal(opts.url, 'http://localhost/store/?lol'); + assert.deepEqual(opts.data, { + project: '2', + release: 'abc123', + logger: 'javascript', + platform: 'javascript', + request: { + url: 'http://localhost/?a=b', + headers: { + 'User-Agent': 'lolbrowser' + } + }, + event_id: 'abc123', + foo: 'bar', + extra: {'session:duration': 100}, + }); + assert.deepEqual(opts.options, globalOptions); + assert.isFunction(opts.onSuccess); + assert.isFunction(opts.onError); + }); it('should check `isSetup`', function() { this.sinon.stub(window, 'isSetup').returns(false); - makeRequest({foo: 'bar'}); + this.sinon.stub(window, 'makeRequest'); + send({foo: 'bar'}); assert.isTrue(window.isSetup.called); }); - it('should not create the image if `isSetup` is false', function() { + it('should not makeRequest if `isSetup` is false', function() { this.sinon.stub(window, 'isSetup').returns(false); - makeRequest({foo: 'bar'}); - assert.isFalse(window.newImage.called); + this.sinon.stub(window, 'makeRequest'); + send({foo: 'bar'}); + assert.isFalse(window.makeRequest.called); }); it('should log to console', function() { this.sinon.stub(window, 'isSetup').returns(true); this.sinon.stub(window, 'logDebug'); - makeRequest({foo: 'bar'}); + this.sinon.stub(window, 'makeRequest'); + send({foo: 'bar'}); assert.isTrue(window.logDebug.called); }); + }); - it('should load an Image', function() { - authQueryString = '?lol'; - globalServer = 'http://localhost/'; + describe('makeRequest', function() { + var imageCache; - makeRequest({foo: 'bar'}); + beforeEach(function () { + imageCache = []; + this.sinon.stub(window, 'newImage', function(){ var img = {}; imageCache.push(img); return img; }); + }) + + it('should load an Image', function() { + makeRequest({ + url: 'http://localhost/?lol', + data: {foo: 'bar'}, + options: globalOptions + }); assert.equal(imageCache.length, 1); assert.equal(imageCache[0].src, 'http://localhost/?lol&sentry_data=%7B%22foo%22%3A%22bar%22%7D'); }); @@ -1161,7 +1207,11 @@ describe('globals', function() { globalOptions = { crossOrigin: 'something' }; - makeRequest({foo: 'bar'}); + makeRequest({ + url: globalServer, + data: {foo: 'bar'}, + options: globalOptions + }); assert.equal(imageCache.length, 1); assert.equal(imageCache[0].crossOrigin, 'something'); }); @@ -1170,7 +1220,11 @@ describe('globals', function() { globalOptions = { crossOrigin: '' }; - makeRequest({foo: 'bar'}); + makeRequest({ + url: globalServer, + data: {foo: 'bar'}, + options: globalOptions + }); assert.equal(imageCache.length, 1); assert.equal(imageCache[0].crossOrigin, ''); }); @@ -1179,7 +1233,11 @@ describe('globals', function() { globalOptions = { crossOrigin: false }; - makeRequest({foo: 'bar'}); + makeRequest({ + url: globalServer, + data: {foo: 'bar'}, + options: globalOptions + }); assert.equal(imageCache.length, 1); assert.isUndefined(imageCache[0].crossOrigin); }); From 2e4a92c2115a4156e235a8544bfaed6ea17dddc7 Mon Sep 17 00:00:00 2001 From: Matt Robenolt Date: Mon, 14 Sep 2015 11:42:33 -0700 Subject: [PATCH 2/2] Get rid of setAuthQueryString and pass all the bits to makeRequest --- src/raven.js | 40 +++++++++++++++++++++------------------- test/raven.test.js | 32 +++++++++++++++++++------------- 2 files changed, 40 insertions(+), 32 deletions(-) diff --git a/src/raven.js b/src/raven.js index e956a74e8095..bfca257ac3ff 100644 --- a/src/raven.js +++ b/src/raven.js @@ -23,7 +23,6 @@ var _Raven = window.Raven, maxMessageLength: 100, extra: {} }, - authQueryString, isRavenInstalled = false, objectPrototype = Object.prototype, // capture references to window.console *and* all its methods first @@ -114,8 +113,6 @@ var Raven = { TraceKit.collectWindowErrors = !!globalOptions.collectWindowErrors; - setAuthQueryString(); - // return for chaining return Raven; }, @@ -508,15 +505,6 @@ function each(obj, callback) { } } - -function setAuthQueryString() { - authQueryString = - '?sentry_version=4' + - '&sentry_client=raven-js/' + Raven.VERSION + - '&sentry_key=' + globalKey; -} - - function handleStackInfo(stackInfo, options) { var frames = []; @@ -752,31 +740,36 @@ function send(data) { if (!isSetup()) return; - var url = globalServer + authQueryString; - makeRequest({ - url: url, + url: globalServer, + auth: { + sentry_version: '4', + sentry_client: 'raven-js/' + Raven.VERSION, + sentry_key: globalKey + }, data: data, options: globalOptions, onSuccess: function success() { triggerEvent('success', { data: data, - src: url + src: globalServer }); }, onError: function failure() { triggerEvent('failure', { data: data, - src: url + src: globalServer }); } }); } - function makeRequest(opts) { + // Tack on sentry_data to auth options, which get urlencoded + opts.auth.sentry_data = JSON.stringify(opts.data); + var img = newImage(), - src = opts.url + '&sentry_data=' + encodeURIComponent(JSON.stringify(opts.data)); + src = opts.url + '?' + urlencode(opts.auth); if (opts.options.crossOrigin || opts.options.crossOrigin === '') { img.crossOrigin = opts.options.crossOrigin; @@ -876,4 +869,13 @@ function afterLoad() { Raven.config(RavenConfig.dsn, RavenConfig.config).install(); } } + +function urlencode(o) { + var pairs = []; + each(o, function(key, value) { + pairs.push(encodeURIComponent(key) + '=' + encodeURIComponent(value)); + }); + return pairs.join('&'); +} + afterLoad(); diff --git a/test/raven.test.js b/test/raven.test.js index 52e1f2544391..f90da7aafc93 100644 --- a/test/raven.test.js +++ b/test/raven.test.js @@ -1,5 +1,4 @@ function flushRavenState() { - authQueryString = undefined; hasJSON = !isUndefined(window.JSON); lastCapturedException = undefined; lastEventId = undefined; @@ -360,14 +359,6 @@ describe('globals', function() { }); }); - describe('setAuthQueryString', function() { - it('should return a properly formatted string and cache it', function() { - var expected = '?sentry_version=4&sentry_client=raven-js/<%= pkg.version %>&sentry_key=abc'; - setAuthQueryString(); - assert.strictEqual(authQueryString, expected); - }); - }); - describe('parseDSN', function() { it('should do what it advertises', function() { var pieces = parseDSN('http://abc@example.com:80/2'); @@ -1129,7 +1120,6 @@ describe('globals', function() { }); globalServer = 'http://localhost/store/'; - authQueryString = '?lol' globalOptions = { projectId: 2, @@ -1141,7 +1131,7 @@ describe('globals', function() { var args = window.makeRequest.lastCall.args; assert.equal(args.length, 1); var opts = args[0]; - assert.equal(opts.url, 'http://localhost/store/?lol'); + assert.equal(opts.url, 'http://localhost/store/'); assert.deepEqual(opts.data, { project: '2', release: 'abc123', @@ -1157,6 +1147,11 @@ describe('globals', function() { foo: 'bar', extra: {'session:duration': 100}, }); + assert.deepEqual(opts.auth, { + sentry_client: 'raven-js/<%= pkg.version %>', + sentry_key: 'abc', + sentry_version: '4' + }); assert.deepEqual(opts.options, globalOptions); assert.isFunction(opts.onSuccess); assert.isFunction(opts.onError); @@ -1195,12 +1190,13 @@ describe('globals', function() { it('should load an Image', function() { makeRequest({ - url: 'http://localhost/?lol', + url: 'http://localhost/', + auth: {a: '1', b: '2'}, data: {foo: 'bar'}, options: globalOptions }); assert.equal(imageCache.length, 1); - assert.equal(imageCache[0].src, 'http://localhost/?lol&sentry_data=%7B%22foo%22%3A%22bar%22%7D'); + assert.equal(imageCache[0].src, 'http://localhost/?a=1&b=2&sentry_data=%7B%22foo%22%3A%22bar%22%7D'); }); it('should populate crossOrigin based on globalOptions', function() { @@ -1209,6 +1205,7 @@ describe('globals', function() { }; makeRequest({ url: globalServer, + auth: {lol: '1'}, data: {foo: 'bar'}, options: globalOptions }); @@ -1222,6 +1219,7 @@ describe('globals', function() { }; makeRequest({ url: globalServer, + auth: {lol: '1'}, data: {foo: 'bar'}, options: globalOptions }); @@ -1235,6 +1233,7 @@ describe('globals', function() { }; makeRequest({ url: globalServer, + auth: {lol: '1'}, data: {foo: 'bar'}, options: globalOptions }); @@ -1418,6 +1417,13 @@ describe('globals', function() { ]).source, 'a|b|a\\.b|d|[0-9]'); }); }); + + describe('urlencode', function() { + it('should work', function() { + assert.equal(urlencode({}), ''); + assert.equal(urlencode({'foo': 'bar', 'baz': '1 2'}), 'foo=bar&baz=1%202'); + }); + }); }); describe('Raven (public API)', function() {