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

[m-amr]

…essages configurable

Closes #14744

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
Sometimes AngularJS throw error with long messages, very long URL referenced error
Will cause 414 Request-URI Too Large as in Issue #14744

What is the new behavior (if this is a feature change)?
make the Url reference error length configurable.

    errorHandlingConfig({urlMaxLength: 500});

Does this PR introduce a breaking change?
NO

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
Microsoft Internet Explorer has a maximum uniform resource locator (URL) length of 2,083 characters. Internet Explorer also has a maximum path length of 2,048 characters. This limit applies to both POST request and GET request URLs. https://support.microsoft.com/en-us/help/208427/maximum-url-length-is-2,083-characters-in-internet-explorer which is less than all the other browsers, so i added the default value for url max length to be 2000 characters.

I don't know what should be the minimum length for URL reference error but i implemented it with a constrains to be more than 200.

[gkalpak]

Don't we have tests for errorHandlingConfig()? 😱
Can you add them?

[gkalpak]

This like is too long. Also, why a 200 limit?

[gkalpak]

If there is no good reason for the 200 limit. Let's remove it.
And then we can reuse isValidObjectMaxDepth() for both validations (under a different, more generic name 😉).

[gkalpak]

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?)

[Narretz]

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

[gkalpak]

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

[m-amr]

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.

[gkalpak]

This seems more complex than necessary :confused. I wonder if we could simplify the implementation.

[gkalpak]

I don't think we need a helper for this. IIRC, we use new Array(size).join('a') elsewhere in the tests.

[gkalpak]

No need for these assertions (here and below).

[m-amr]

These assertions are important to test that the default value is 2000 and doesn't change.

[gkalpak]

You essentially testing two different things: errorHandlingConfig and minErr.
There should be different tests about errorHandlingConfig, that verify that it works. Then here, you don't need to verify that. Just that minErr works as expected

[m-amr]

There are tests for 'errorHandlingConfig'

it('should slice error reference URL in the message if it exceeds url max length', function() {
    var a = createDummyString(2000);

    var myError = testError('26', 'a when default=2000 is {0}', a);
    var url = extractUrlFromErrorMessage(myError.message);
    expect(url.length).toBe(2000);
    expect(errorHandlingConfig().urlMaxLength).toBe(2000);

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

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

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

Do you mean to create a separate test for errorHandlingConfig? and the same will be for objectMaxDepth.

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

  it('should set urlMaxLength', function(){
      errorHandlingConfig({urlMaxLength:3000});
      expect(errorHandlingConfig().urlMaxLength).toBe(3000);
   });

   they('should set urlMaxLength to be NAN when it is setting to $prop', 
        [false, true, -1,  null, NAN], function(maxLength){
            errorHandlingConfig({urlMaxLength: maxLength});
            expect(errorHandlingConfig().urlMaxLength).toBeNAN();;
   });
});

[gkalpak]

Yes. We should have tests for errorHandlingConfig() (independent of minErr) that verify things like:

• Default values.
• It works as a setter.
• It works as a getter.
• It sanitizes the inputs.

[m-amr]

@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 ?

[gkalpak]

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.

[Narretz]

If we cannot truncate the url reliably, can we truncate the stack instead? This means we won't know exactly how long the url is, but this isn't really needed anyway. We just want a url that has a managable size.

[Narretz]

So, it turns out the error stack is not an explicit parameter to the minErr. It simply gets added whenever throwing a minErr triggers another minErr. A common cause is when you include a module that doesn't exist: this triggers "nomod" and then "modulerr", i.e. modulerr includes the message (good) and stack (bad) of nomod, e.g. this error url

So we cannot say "truncate only the stack", but we can say "truncate all error template arguments", which will sometimes include the stack of another error.
However, even if you say argLength = 0 you will still get a stack appended in the console, which is normal browser behavior, but since it is errors in errors it can still be unwieldy, depending on the error.

We could also allow urlArgsMaxLength=0 and then remove the part with the parameters completely, as proposed in #16283

Here's a PoC:
http://plnkr.co/edit/6zfFojrkH4xha9HmXodj?p=preview

In Chrome you get a "nice" stack which is the moderr, and then for some reason 4 times the same modulerr.

In Firefox, it's just a giant blob that is not even clickable (but that is the default behavior).

So by restricting the argument length, it becomes a bit better (and the urls actually work), but the stacks can still be confusing.

[m-amr]

I think that PR #16283 has a better solution than this PR
I will close my pull request when that PR is merged.