Remove try/catch for short url so the appropriate errors will be propagated to the UI #13004

stacey-gammon · 2017-07-20T15:58:44Z

Previously, any error with a short url, e.g. looking for one that didn't exist, or encountering an auth exception, would all result in the same ambiguous error:

{"statusCode":406,"error":"Not Acceptable","message":"Short url target path must be in the format \"/app/{{appId}}\", found \"/\""}

With this change, the right "not found" Boom error will be sent to the user, as should auth exceptions (that was hard to test since you can't log in to kibana without read permissions, but theoretically it should be propagated as well).

Fixes #10456

cc @spalger - you commented on the original issue so tagging you if you want to take a look.

…agated to the UI

spalger · 2017-07-20T17:09:08Z

I'm concerned with the potential BWC impact of a change like this.

Also, since this is going to lead to more error responses I took a look at src/server/http/short_url_error.js, which I think needs to be updated to just wrap the error object rather than replace it with new Boom errors. At least the function is checking if the error is a Boom error first, but replacing the entire function with Boom.wrap(error, error.status || 500); would do the same thing without discarding the original error.

PS: unlike the name suggests, Boom.wrap() preserves the original error but decorates it with the necessary properties to behave like a Boom error

stacey-gammon · 2017-07-20T17:17:23Z

Can you explain a bit more about the BWC concerns? As far as I can tell, any error previously would have showed up with the '/' error, but now the error will be different. You think people will be relying on the '/' error? I def don't want to cause any harm, but it does seem weird that errors are swallowed and replaced with a different, random error.

If this gets in to 6.0, we don't have to be as concerned with BWC... ?

…original error details.

spalger · 2017-07-20T17:35:31Z

Yeah, that's the only thing I was concerned with. That said, I think as long as what was once an error is still an error I'm good with it. And yeah, if we get this into 6.0 then there really aren't any BWC concerns.

…how-short-url-errors

stacey-gammon · 2017-07-21T16:24:14Z

Failing on:

  single test that uses esArchiver
     │ debg  Loading config file from "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/functional_test_runner/__tests__/fixtures/with_es_archiver/config.js"
     │ info  starting elasticsearch
       │ debg  WARN: bootstrap_checks - max virtual memory areas vm.max_map_count [65530] is too low, increase to at least [262144]
       │ debg  WARN: unicast_zen_ping - [1] failed send ping to {#zen_unicast_127.0.0.1_0#}{tbmLBf5RSxerQY61HCAOTA}{127.0.0.1}{127.0.0.1:9300}
       │ debg  WARN: unicast_zen_ping - [1] failed send ping to {#zen_unicast_127.0.0.1_0#}{tbmLBf5RSxerQY61HCAOTA}{127.0.0.1}{127.0.0.1:9300}
       │ debg  WARN: unicast_zen_ping - [1] failed send ping to {#zen_unicast_127.0.0.1_0#}{tbmLBf5RSxerQY61HCAOTA}{127.0.0.1}{127.0.0.1:9300}
     │ info  starting kibana
{"type":"log","@timestamp":"2017-07-21T12:37:39Z","tags":["listening","info"],"pid":10383,"message":"Server running at http://localhost:5701"}
     │ debg   info  Config loaded
     │        info  Starting tests
     │       
     │        └-: tests
     │          └-> "before all" hook
     │            │ info  [test_archive] Loading "mappings.json"
     │          └- ✖ fail: "tests "before all" hook"
     │          │        [illegal_argument_exception] unknown setting [index.mapping.single_type] please check that any required plugins are installed, or check the breaking changes documentation for removed settings
     │          │         :: {"path":"/.kibana","query":{},"body":"{\"settings\":{\"index\":{\"mapping\":{\"single_type\":\"false\"},\"number_of_shards\":\"1\",\"mapper\":{\"dynamic\":\"fal

Will merge again with master and push

…how-short-url-errors

spalger · 2017-07-21T18:15:40Z

Sorry @stacey-gammon, fixed that in #13021

spalger · 2017-07-21T18:17:54Z

jenkins, test this

…how-short-url-errors

stacey-gammon · 2017-07-21T20:17:59Z

Hmm, thought I merged with the latest to grab the fix. Failing on:

  single test that uses esArchiver
     │ debg  Loading config file from "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-intake/src/functional_test_runner/__tests__/fixtures/with_es_archiver/config.js"
     │ info  starting elasticsearch
       │ debg  WARN: bootstrap_checks - max virtual memory areas vm.max_map_count [65530] is too low, increase to at least [262144]
       │ debg  WARN: unicast_zen_ping - [1] failed send ping to {#zen_unicast_127.0.0.1_0#}{OGSHWNcqR0GPXdMRvM6gig}{127.0.0.1}{127.0.0.1:9300}
       │ debg  WARN: unicast_zen_ping - [1] failed send ping to {#zen_unicast_127.0.0.1_0#}{OGSHWNcqR0GPXdMRvM6gig}{127.0.0.1}{127.0.0.1:9300}
       │ debg  WARN: unicast_zen_ping - [1] failed send ping to {#zen_unicast_127.0.0.1_0#}{OGSHWNcqR0GPXdMRvM6gig}{127.0.0.1}{127.0.0.1:9300}
     │ info  starting kibana
{"type":"log","@timestamp":"2017-07-21T19:27:01Z","tags":["listening","info"],"pid":11150,"message":"Server running at http://localhost:5701"}
     │ debg   info  Config loaded
     │        info  Starting tests
     │       
     │        └-: tests
     │          └-> "before all" hook
     │            │ info  [test_archive] Loading "mappings.json"
     │          └- ✖ fail: "tests "before all" hook"
     │          │        [illegal_argument_exception] Rejecting mapping update to [.kibana] as the final mapping would have more than 1 type: [server, index-pattern, visualization, search, timelion-sheet, config, url, dashboard]
     │          │         :: {"path":"/.kibana","query":{},"body":"{\"settings\":{\"index\":{\"number_of_shards\":\"1\",\"mapper\"

stacey-gammon · 2017-07-21T20:19:01Z

Confirmed i'm on the latest version.

jenkins, test this

…how-short-url-errors

…ests.

stacey-gammon · 2017-07-24T15:47:30Z

tests now passing, ready for a final look.

…how-short-url-errors

spalger · 2017-07-25T19:40:28Z

src/server/http/short_url_error.js

-  if (err.status === 404) return Boom.notFound();
-  return Boom.badImplementation();
+export function handleShortUrlError(error) {
+  if (!error.isBoom && !(error instanceof Error)) {


We really should not support non-errors here. Pretty sure this was spurred by the tests failing because they aren't sending real errors, which means that this code isn't actually testing what would happen in production with this condition. I think this check should go, we should always use Boom.wrap() and the tests should use real error objects.

I can do that, I was just worried about changing implementation details, not knowing if I would create a subtle bug somewhere.

stacey-gammon · 2017-07-25T20:19:48Z

jenkins, test this

…how-short-url-errors

tylersmalley · 2017-07-26T00:02:22Z

jenkins, test it

stacey-gammon · 2017-07-27T14:03:34Z

Failed on:

281      └-: data-shared-item
01:48:58.281        └-> should have correct data-shared-item title and description
01:48:58.281          └-> "before each" hook: global before each
01:48:58.288            │ debg  Kibana uiSettings are in elasticsearch and the server is reporting a green status
01:48:58.288          │ debg  TestSubjects.find(discoverOpenButton)
01:48:58.290          │ debg  in displayedByCssSelector: [data-test-subj~="discoverOpenButton"]
01:48:58.421          │ debg  TestSubjects.find(globalLoadingIndicator)
01:48:58.421          │ debg  in displayedByCssSelector: [data-test-subj~="globalLoadingIndicator"]
01:48:58.607          │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/discover app discover app data-shared-item should have correct data-shared-item title and description.png"
01:48:58.866        └- ✖ fail: "discover app discover app data-shared-item should have correct data-shared-item title and description"
01:48:58.867        │       
01:48:58.867        │         Error: expected 'Query # 1' to sort of equal 'A Saved Search'
01:48:58.867        │         + expected - actual
01:48:58.867        │       
01:48:58.867        │         -Query # 1
01:48:58.867        │         +A Saved Search
01:48:58.867        │         
01:48:58.867        │         at Assertion.assert (node_mo

Looks like a flaky test. Will sync with master and try again.

…how-short-url-errors

spalger · 2017-07-27T14:12:33Z

src/server/http/__tests__/short_url_error.js

-    it(`should handle ${err.status} errors`, function () {
-      expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status);
+    it(`should handle ${err.status || err.statusCode} errors`, function () {
+      expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status || err.statusCode);


The first error in uncaughtErrors is going to have a status and statusCode of undefined, right? Isn't that I think these three tests should just be written separately, rather than implementing unnecessary looping/conditional checking

++

I split out the status and statusCode checks, but the errors in those two test loops should always have either status or statusCode defined. Only uncaughtErrors test has a case when they are both undefined.

…agated to the UI (elastic#13004) * Remove try/catch for short url so the appropriate errors will be propagated to the UI * Simply ensure the error is a Boom error by wrapping it, but keep the original error details. * Boom.wrap can't handle non Error instances, as exist in some of the tests. * Only support Error objects, and check both status and statusCode * fix test * Fix test errors for reals this time * Break out status and statusCode short url error tests

…agated to the UI (#13004) (#13179) * Remove try/catch for short url so the appropriate errors will be propagated to the UI * Simply ensure the error is a Boom error by wrapping it, but keep the original error details. * Boom.wrap can't handle non Error instances, as exist in some of the tests. * Only support Error objects, and check both status and statusCode * fix test * Fix test errors for reals this time * Break out status and statusCode short url error tests

stacey-gammon requested a review from tylersmalley July 20, 2017 15:59

Remove try/catch for short url so the appropriate errors will be prop…

b62b879

…agated to the UI

stacey-gammon force-pushed the fix/show-short-url-errors branch from c72a288 to b62b879 Compare July 20, 2017 16:22

Simply ensure the error is a Boom error by wrapping it, but keep the …

2295921

…original error details.

stacey-gammon added 2 commits July 20, 2017 13:37

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

5b90bc8

…how-short-url-errors

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

b836eeb

…how-short-url-errors

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

3873b33

…how-short-url-errors

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

a729049

…how-short-url-errors

stacey-gammon added :Sharing review labels Jul 21, 2017

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

716eea2

…how-short-url-errors

stacey-gammon mentioned this pull request Jul 24, 2017

Remove translations from management section #13049

Merged

Boom.wrap can't handle non Error instances, as exist in some of the t…

d5f622f

…ests.

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

9cd4ce4

…how-short-url-errors

stacey-gammon added the v6.1.0 label Jul 25, 2017

spalger suggested changes Jul 25, 2017

View reviewed changes

Only support Error objects, and check both status and statusCode

109a67f

stacey-gammon added 2 commits July 25, 2017 16:35

fix test

a7d36dc

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

d4d7b15

…how-short-url-errors

Fix test errors for reals this time

b7870ab

stacey-gammon force-pushed the fix/show-short-url-errors branch from a509e00 to b7870ab Compare July 27, 2017 13:19

Merge branch 'master' of https://github.com/elastic/kibana into fix/s…

9f971e5

…how-short-url-errors

spalger reviewed Jul 27, 2017

View reviewed changes

Break out status and statusCode short url error tests

a2e864e

spalger approved these changes Jul 28, 2017

View reviewed changes

stacey-gammon merged commit 3f54e94 into elastic:master Jul 28, 2017

stacey-gammon mentioned this pull request Jul 28, 2017

[6.x] Remove try/catch for short url so the appropriate errors will be prop… #13179

Merged

stacey-gammon deleted the fix/show-short-url-errors branch October 24, 2017 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove try/catch for short url so the appropriate errors will be propagated to the UI #13004

Remove try/catch for short url so the appropriate errors will be propagated to the UI #13004

stacey-gammon commented Jul 20, 2017 •

edited

Loading

spalger commented Jul 20, 2017 •

edited

Loading

stacey-gammon commented Jul 20, 2017

spalger commented Jul 20, 2017

stacey-gammon commented Jul 21, 2017

spalger commented Jul 21, 2017

spalger commented Jul 21, 2017

stacey-gammon commented Jul 21, 2017

stacey-gammon commented Jul 21, 2017

stacey-gammon commented Jul 24, 2017

spalger Jul 25, 2017

stacey-gammon Jul 25, 2017

stacey-gammon commented Jul 25, 2017

tylersmalley commented Jul 26, 2017

stacey-gammon commented Jul 27, 2017

spalger Jul 27, 2017 •

edited

Loading

stacey-gammon Jul 27, 2017

Remove try/catch for short url so the appropriate errors will be propagated to the UI #13004

Remove try/catch for short url so the appropriate errors will be propagated to the UI #13004

Conversation

stacey-gammon commented Jul 20, 2017 • edited Loading

spalger commented Jul 20, 2017 • edited Loading

stacey-gammon commented Jul 20, 2017

spalger commented Jul 20, 2017

stacey-gammon commented Jul 21, 2017

spalger commented Jul 21, 2017

spalger commented Jul 21, 2017

stacey-gammon commented Jul 21, 2017

stacey-gammon commented Jul 21, 2017

stacey-gammon commented Jul 24, 2017

spalger Jul 25, 2017

Choose a reason for hiding this comment

stacey-gammon Jul 25, 2017

Choose a reason for hiding this comment

stacey-gammon commented Jul 25, 2017

tylersmalley commented Jul 26, 2017

stacey-gammon commented Jul 27, 2017

spalger Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

stacey-gammon Jul 27, 2017

Choose a reason for hiding this comment

stacey-gammon commented Jul 20, 2017 •

edited

Loading

spalger commented Jul 20, 2017 •

edited

Loading

spalger Jul 27, 2017 •

edited

Loading