Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(errorHandlingConfig): make the length of URL refrence in error m… #15707

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"toString": false,
"minErrConfig": false,
"errorHandlingConfig": false,
"isValidObjectMaxDepth": false,
"isValidNumberForMinErrConfig": false,
"ngMinErr": false,
"_angular": false,
"angularModule": false,
Expand Down
24 changes: 17 additions & 7 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
toString,
minErrConfig,
errorHandlingConfig,
isValidObjectMaxDepth,
isValidNumberForMinErrConfig,
ngMinErr,
angularModule,
uid,
Expand Down Expand Up @@ -129,7 +129,8 @@ var VALIDITY_STATE_PROPERTY = 'validity';
var hasOwnProperty = Object.prototype.hasOwnProperty;

var minErrConfig = {
objectMaxDepth: 5
objectMaxDepth: 5,
urlMaxLength: 2000
};

/**
Expand All @@ -143,6 +144,8 @@ var minErrConfig = {
* current configuration if used as a getter. The following options are supported:
*
* - **objectMaxDepth**: The maximum depth to which objects are traversed when stringified for error messages.
* - **urlMaxLength**: The maximum length of the URL reference in the error messages.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a note explaining the consequences this will have to longer URLs (e.g. will they break? will they lack information? will the stack traces be truncated?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restricting this will definitely mess with the error parsing in the docs app. I was wondering if we should additionally to objectMaxDepth have arrayMaxLength? Because when you have an array with 1000 objects, then objectMaxDepth isn't going to help your shortening the error output

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting the array length makes sense (although not directly related to this PR).

Copy link
Contributor Author

@m-amr m-amr Feb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that each parameter value is encoded using encodeURIComponent
so when slice this encoded value it become invalid for some cases.

for (i = 0, paramPrefix = '?'; i < templateArgs.length; i++, paramPrefix = '&') {
      url += paramPrefix + 'p' + i + '=' + encodeURIComponent(templateArgs[i]);

      if (url.length > minErrConfig.urlMaxLength) {
        url = url.substr(0, minErrConfig.urlMaxLength - 3) + '...';
        break;
      }
    }

for example:
This link produced when i set urlMaxLength to 400
http://errors.angularjs.org/1.6.3-local+sha.48e0b69/$injector/modulerr?p0=n…cies%20as%20the%20second%20argument.%0Ahttp%3A%2F%2Ferrors.angularjs.org%2...

it look like it turn into
https://docs.angularjs.org/error/$injector/modulerr?p0=undefined

I will try to find a solution that slice the url without harming the encoded data.

* if the url exceeds the max length it will be truncated, so it will lack of information.
*
* Omitted or undefined options will leave the corresponding configuration values unchanged.
*
Expand All @@ -152,11 +155,18 @@ var minErrConfig = {
* * `objectMaxDepth` **{Number}** - The max depth for stringifying objects. Setting to a
* non-positive or non-numeric value, removes the max depth limit.
* Default: 5
*
* * `urlMaxLength` **{Number}** - The max length of the URL reference in the error messages. Setting to a
* non-positive or non-numeric value, removes the url max length limit.
* Default: 2000
*/
function errorHandlingConfig(config) {
if (isObject(config)) {
if (isDefined(config.objectMaxDepth)) {
minErrConfig.objectMaxDepth = isValidObjectMaxDepth(config.objectMaxDepth) ? config.objectMaxDepth : NaN;
minErrConfig.objectMaxDepth = isValidNumberForMinErrConfig(config.objectMaxDepth) ? config.objectMaxDepth : NaN;
}
if (isDefined(config.urlMaxLength)) {
minErrConfig.urlMaxLength = isValidNumberForMinErrConfig(config.urlMaxLength) ? config.urlMaxLength : NaN;
}
} else {
return minErrConfig;
Expand All @@ -165,11 +175,11 @@ function errorHandlingConfig(config) {

/**
* @private
* @param {Number} maxDepth
* @param {Number} num
* @return {boolean}
*/
function isValidObjectMaxDepth(maxDepth) {
return isNumber(maxDepth) && maxDepth > 0;
function isValidNumberForMinErrConfig(num) {
return isNumber(num) && num > 0;
}

/**
Expand Down Expand Up @@ -897,7 +907,7 @@ function arrayRemove(array, value) {
function copy(source, destination, maxDepth) {
var stackSource = [];
var stackDest = [];
maxDepth = isValidObjectMaxDepth(maxDepth) ? maxDepth : NaN;
maxDepth = isValidNumberForMinErrConfig(maxDepth) ? maxDepth : NaN;

if (destination) {
if (isTypedArray(destination) || isArrayBuffer(destination)) {
Expand Down
10 changes: 8 additions & 2 deletions src/minErr.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,19 @@ function minErr(module, ErrorConstructor) {
return match;
});

message += '\nhttp://errors.angularjs.org/"NG_VERSION_FULL"/' +
var url = 'http://errors.angularjs.org/"NG_VERSION_FULL"/' +
(module ? module + '/' : '') + code;

for (i = 0, paramPrefix = '?'; i < templateArgs.length; i++, paramPrefix = '&') {
message += paramPrefix + 'p' + i + '=' + encodeURIComponent(templateArgs[i]);
url += paramPrefix + 'p' + i + '=' + encodeURIComponent(templateArgs[i]);

if (url.length > minErrConfig.urlMaxLength) {
url = url.substr(0, minErrConfig.urlMaxLength - 3) + '...';
break;
Copy link
Contributor Author

@m-amr m-amr Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak
I feel it is confusing too, if we set urlMaxlength to 1000, then it will substr(0, 997) since it will add '...'
So the max length become 997 not 1000.
Should I remove '...' ? but still need something to tell that this is not the full url.
Should I substr(0, 1000) then add '...' and handle this in the unit test by asserting it to equal 1003

Do you have any suggestion ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ... (or whatever we choose to indicate the URL was truncated) is not actually part of the URL. So, it shouldn't count against its length.

I suggest, you don't take it into account when calculating the URL length. And instead of appending ... (which might not be clear), I suggest we add soething like (truncated URL) right after.

}
}

message += '\n' + url;
return new ErrorConstructor(message);
};
}
2 changes: 1 addition & 1 deletion src/stringify.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ function serializeObject(obj, maxDepth) {
// There is no direct way to stringify object until reaching a specific depth
// and a very deep object can cause a performance issue, so we copy the object
// based on this specific depth and then stringify it.
if (isValidObjectMaxDepth(maxDepth)) {
if (isValidNumberForMinErrConfig(maxDepth)) {
obj = copy(obj, null, maxDepth);
}
return JSON.stringify(obj, function(key, val) {
Expand Down
54 changes: 54 additions & 0 deletions test/AngularSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,67 @@ Float32Array, Float64Array, */

describe('angular', function() {
var element, document;
var originalObjectMaxDepthInErrorMessage = minErrConfig.objectMaxDepth;
var originalUrlMaxLengthInErrorMessage = minErrConfig.urlMaxLength;

beforeEach(function() {
document = window.document;
});

afterEach(function() {
dealoc(element);
minErrConfig.objectMaxDepth = originalObjectMaxDepthInErrorMessage;
minErrConfig.urlMaxLength = originalUrlMaxLengthInErrorMessage;
});

describe('errorHandlingConfig', function() {
it('should get default objectMaxDepth', function() {
expect(errorHandlingConfig().objectMaxDepth).toBe(5);
});

it('should set objectMaxDepth only', function() {
errorHandlingConfig({objectMaxDepth: 3});
expect(errorHandlingConfig()).toEqual({
objectMaxDepth: 3,
urlMaxLength: originalUrlMaxLengthInErrorMessage
});
});

it('should not change objectMaxDepth when undefined is supplied', function() {
errorHandlingConfig({objectMaxDepth: undefined});
expect(errorHandlingConfig().objectMaxDepth).toBe(originalObjectMaxDepthInErrorMessage);
});

they('should set objectMaxDepth to NAN when $prop is supplied',
[NaN, null, true, false, -1, 0], function(maxDepth) {
errorHandlingConfig({objectMaxDepth: maxDepth});
expect(errorHandlingConfig().objectMaxDepth).toBeNaN();
}
);

it('should get default urlMaxLength', function() {
expect(errorHandlingConfig().urlMaxLength).toBe(2000);
});

it('should set urlMaxLength only', function() {
errorHandlingConfig({urlMaxLength: 500});
expect(errorHandlingConfig()).toEqual({
'objectMaxDepth': originalObjectMaxDepthInErrorMessage,
'urlMaxLength': 500
});
});

it('should not change urlMaxLength when undefined is supplied', function() {
errorHandlingConfig({urlMaxLength: undefined});
expect(errorHandlingConfig().urlMaxLength).toBe(originalUrlMaxLengthInErrorMessage);
});

they('should set urlMaxLength to NAN when $prop is supplied',
[NaN, null, true, false, -1, 0], function(maxLength) {
errorHandlingConfig({urlMaxLength: maxLength});
expect(errorHandlingConfig().urlMaxLength).toBeNaN();
}
);
});

describe('case', function() {
Expand Down
48 changes: 41 additions & 7 deletions test/minErrSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,21 @@ describe('minErr', function() {
testError = minErr('test');

var originalObjectMaxDepthInErrorMessage = minErrConfig.objectMaxDepth;
var originalUrlMaxLengthInErrorMessage = minErrConfig.urlMaxLength;
afterEach(function() {
minErrConfig.objectMaxDepth = originalObjectMaxDepthInErrorMessage;
minErrConfig.urlMaxLength = originalUrlMaxLengthInErrorMessage;
});

function extractUrlFromErrorMessage(message) {
var match = message.match(/http[\s\S]*\?p0=/);
var urlStartAt = message.indexOf(match[0]);
if (urlStartAt < 0) {
throw new Error('Could not find url');
}
return message.slice(urlStartAt);
}

it('should return an Error factory', function() {
var myError = testError('test', 'Oops');
expect(myError instanceof Error).toBe(true);
Expand Down Expand Up @@ -78,32 +89,28 @@ describe('minErr', function() {

var myError = testError('26', 'a when objectMaxDepth is default=5 is {0}', a);
expect(myError.message).toMatch(/a when objectMaxDepth is default=5 is {"b":{"c":{"d":{"e":{"f":"..."}}}}}/);
expect(errorHandlingConfig().objectMaxDepth).toBe(5);


errorHandlingConfig({objectMaxDepth: 1});
myError = testError('26', 'a when objectMaxDepth is set to 1 is {0}', a);
expect(myError.message).toMatch(/a when objectMaxDepth is set to 1 is {"b":"..."}/);
expect(errorHandlingConfig().objectMaxDepth).toBe(1);

errorHandlingConfig({objectMaxDepth: 2});
myError = testError('26', 'a when objectMaxDepth is set to 2 is {0}', a);
expect(myError.message).toMatch(/a when objectMaxDepth is set to 2 is {"b":{"c":"..."}}/);
expect(errorHandlingConfig().objectMaxDepth).toBe(2);

errorHandlingConfig({objectMaxDepth: undefined});
myError = testError('26', 'a when objectMaxDepth is set to undefined is {0}', a);
expect(myError.message).toMatch(/a when objectMaxDepth is set to undefined is {"b":{"c":"..."}}/);
expect(errorHandlingConfig().objectMaxDepth).toBe(2);
});

they('should handle arguments that are objects and ignore max depth when objectMaxDepth = $prop',
[NaN, null, true, false, -1, 0], function(maxDepth) {
var a = {b: {c: {d: 1}}};
var a = {b: {c: {d: {e: {f: {g: 1}}}}}};

errorHandlingConfig({objectMaxDepth: maxDepth});
var myError = testError('26', 'a is {0}', a);
expect(myError.message).toMatch(/a is {"b":{"c":{"d":1}}}/);
expect(errorHandlingConfig().objectMaxDepth).toBeNaN();
expect(myError.message).toMatch(/a is {"b":{"c":{"d":{"e":{"f":{"g":1}}}}}}/);
}
);

Expand Down Expand Up @@ -143,4 +150,31 @@ describe('minErr', function() {
expect(testError('acode', 'aproblem', 'a', 'b', 'value with space').message)
.toMatch(/^[\s\S]*\?p0=a&p1=b&p2=value%20with%20space$/);
});

it('should slice error reference URL in the message if it exceeds url max length', function() {
var a = new Array(3000).join('a');
var myError = testError('26', 'a is {0}', a);
var url = extractUrlFromErrorMessage(myError.message);
expect(url.length).toBe(2000);

errorHandlingConfig({urlMaxLength: 500});
myError = testError('26', 'a is {0}', a);
url = extractUrlFromErrorMessage(myError.message);
expect(url.length).toBe(500);

errorHandlingConfig({urlMaxLength: undefined});
myError = testError('26', 'a is {0}', a);
url = extractUrlFromErrorMessage(myError.message);
expect(url.length).toBe(500);
});

they('should ignore url max length when urlMaxLength = $prop',
[NaN, null, true, false, -1, 0], function(maxLength) {
var a = new Array(3000).join('a');
errorHandlingConfig({urlMaxLength: maxLength});
var myError = testError('26', 'a is {0}', a);
var url = extractUrlFromErrorMessage(myError.message);
expect(url.length).toBeGreaterThan(3000);
}
);
});