[Reporting] Rewrite addForceNowQuerystring to getFullUrls #44851

tsullivan · 2019-09-05T06:23:04Z

Summary

This adds some semantics to the execute steps of PNG and PDF jobs (export types).

Both the types create jobs with relative URLs stored in the job documents, and convert the URLs to absolute when executing the job.

The PDF export type was nearly converting its URLs to absolute in two different places of the execute step: in the compatibility shim, and in addForceNowQueryString.

The important behavior being done in addForceNowQueryString was being obscured by its name. It adds a forceNow param to the URL when applicable, but the main thing it does is convert relative URLs to absolute.

The compatibility shim of execute_job (PDF only) is mostly eliminated now. It was handling object fields that aren't relevant to 7.x job doc payloads: all the execute_job functions only need to consume their type of data returned by create_job.

This PR does not change the interface between data returned from create_job and sent to execute_job.

This PR also adds some extra specificity to the JobDocPayload type, which is becoming just a base type for any type of payload from any of the export types, which each have their differences. This helped getFullUrls get a better understanding of what should be coming in and going out. That meant removing several fields from the main JobDocPayload and re-inserting them only on the extensions that need it. That had a lot of impact in this code change, mostly code removal (CSV types do not need basePath, for example).

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

elasticmachine · 2019-09-05T06:23:06Z

Pinging @elastic/kibana-stack-services

tsullivan · 2019-09-05T06:24:12Z

...legacy/plugins/reporting/export_types/printable_pdf/server/execute_job/compatibility_shim.js

-export function compatibilityShimFactory(server) {
-  const getAbsoluteUrl = getAbsoluteUrlFactory(server);
-
-  const getSavedObjectAbsoluteUrl = (job, savedObject) => {


See also: the create_job compatibility shim, which is now the only place that parses saved objects by ID into Kibana URLs

Correction: this wasn't actually parsing saved objects by ID, but it was handling a structure of data that isn't relevant to the create_job outputs for PDF and PNG. (worth double checking that)

All references to saved objects in the PDF export type should only be in the compatibility shim of create_job. This is because after job creation, only URL paths are dealt with. Multi-page PDF (canvas worksheet) would not be possible otherwise.

elasticmachine · 2019-09-05T07:41:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-05T23:31:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

tsullivan · 2019-09-06T05:21:04Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/get_conditional_headers.test.ts

-          savedObjectId: 'abc-123',
-          isImmediate: false,
-          savedObjectType: 'search',
-        },


A lot of the tests had mock data like this: job params that don't line up to existing features. The csv export type doesn't use savedObjectId and savedObjectType as jobParams. See JobParamsDiscoverCsv is the actual definition. Invalid mocks were part of the reason Typescripting is taking awhile.

Apologies on that -- I believe I wrote these a while ago. You're dead-on about TS taking so long: I believe I originally wrote these mocks with little TS input, so they were likely incorrect. In any case, sorry!

Ha, it's not problem. Mostly I write these things down because I learn as I go

tsullivan · 2019-09-06T05:21:28Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/get_conditional_headers.test.ts

-        savedObjectType: 'search',
-      },
-      basePath: jobBasePath,
-    },


putting a custom logo on a CSV export would make no sense

x-pack/legacy/plugins/reporting/export_types/common/execute_job/get_full_urls.ts

+  });
+
+  // why 2 maps
+  const urls = absoluteUrls.map((jobUrl: string) => {


tsullivan · 2019-09-06T05:28:59Z

...k/legacy/plugins/reporting/export_types/csv_from_savedobject/server/create_job/create_job.ts

@@ -94,7 +94,6 @@ function createJobFn(server: KbnServer): CreateJobFn {
      });

    return {
-      basePath: req.getBasePath(),


This is from basePath being removed from JobDocPayload which all the different JobDocPayload types extend from.

tsullivan · 2019-09-06T05:29:39Z

x-pack/legacy/plugins/reporting/types.d.ts

  title: string;
  type: string | null;
-  urls?: string[];


Removing all these makes me so happy :) The urls one was just because of unnecessary mutation in addNowForceQueryString.

tsullivan · 2019-09-06T05:35:16Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/get_full_urls.test.ts

    server: mockServer,
  });

  expect(urls[0]).toEqual('http://localhost:5601/sbp/app/kibana#/something');
 });
+
+test(`adds forceNow to each of multiple urls`, async () => {


New test! Multi-page PDF URLs!

tsullivan · 2019-09-06T05:45:36Z

x-pack/legacy/plugins/reporting/export_types/printable_pdf/types.d.ts


-// NOTE: this does not extend the main Params


The main JobParams type should go away. Nothing (correctly) uses it or extends it as the export types have each gotten their own definitions.

elasticmachine · 2019-09-06T06:53:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

tsullivan · 2019-09-06T18:34:41Z

...legacy/plugins/reporting/export_types/printable_pdf/server/execute_job/compatibility_shim.js

-    }
-
-    throw new Error(i18n.translate('xpack.reporting.exportTypes.printablePdf.compShim.unableToGenerateReportErrorMessage', {
-      defaultMessage: `Unable to generate report for url {savedObjUrl}, it's not a Kibana URL`,


Absolute URLs are derived in execute_job, so I'm not sure how this error would happen now. There's no new analogy for this error in the changes in this PR.

elasticmachine · 2019-09-06T19:57:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2019-09-06T20:31:04Z

...legacy/plugins/reporting/export_types/printable_pdf/server/execute_job/compatibility_shim.js

- * you may not use this file except in compliance with the Elastic License.
- */
-
-import url from 'url';


Removing execute_job/compatibility_shim means that Reporting won't be able to convert job params created from older version. That functionality is enabled by create_job and still works after this PR. After this PR though, this scenario won't work: (Call it Scenario A)

Use Kibana 5.x to queue a report

Before the report is executed, upgrade the Elastic Stack to 7.5.0

Kibana 7.5.0 tries to execute the job that was created with 6.8, and it won't work. As of this PR, execute_job doesn't convert the legacy params of Saved Object Type and ID into a Kibana path.

The following scenario is why we have create_job/compatibility_shim, and it will still work: (Call it Scenario B)

Use Kibana 5.x to generate a POST URL

Trigger the POST URL with external automation

Upgrade the Elastic Stack to 7.5.0

New jobs come in with 6.x job params

create_job/compatibility_shim converts the legacy params into a format that execute_job/index is able to handle

cc @kobelb I think you've pointed out this kind of thing before

Reading my earlier comment, I don't know if Scenario A from above was expected to work in the previous code.

I think there are actually some good solutions for upgrade complications:

~~A migration to update the pending job documents so they'll be ready for the next major~~

NOPE: We need to migrate Reporting to use Saved Objects before we can make migrations for the jobs.

Add documentation in the upgrade guide to explain: make sure you do not have pending reports when you start the upgrade

elasticmachine · 2019-09-06T21:37:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2019-09-07T00:02:55Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/get_full_urls.ts

+    const parsed: UrlWithParsedQuery = urlParse(jobUrl, true);
+    if (parsed.hash == null) {
+      throw new Error(
+        'No valid hash in the URL! A hash is expected for the application to route to the intended visualization.'


I think the purpose of this code is finally making sense :)

Compare these additions to the removal of export_types/common/execute_job/add_force_now_query_string.ts

tsullivan · 2019-09-07T00:18:03Z

Removed WIP, ready for review (again)

tsullivan · 2019-09-07T00:30:57Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/add_force_now_query_string.ts

-    }
-
-    const parsed: any = url.parse(jobUrl, true);
-    const hash: any = url.parse(parsed.hash.replace(/^#/, ''), true);


It was easy to get a meaningless error from here if the parsed URL has was null. I'm not sure if that has ever happened though

tsullivan · 2019-09-07T00:37:17Z

x-pack/legacy/plugins/reporting/export_types/png/types.d.ts

@@ -8,7 +8,7 @@ import { Request } from 'hapi';
 import { LayoutInstance } from '../common/layouts/layout';
 import { ConditionalHeaders, KbnServer, JobDocPayload } from '../../types';

-// NOTE: this does not extend the main Params


The comment was removed because it is moot - none of the type extend from the main params, so there probably shouldn't even be a main params. Separate issue.

tsullivan · 2019-09-07T00:40:23Z

x-pack/legacy/plugins/reporting/export_types/printable_pdf/types.d.ts

@@ -17,6 +17,18 @@ export interface JobParamsPDF {
  layout: LayoutInstance;


JobParamsPDF should not have relativeUrl: string, it should have relativeUrls: string[]. Knowing that helps relativeUrl: undefined in JobDocPayloadPDF make more sense. That is a separate issue, created from a mistake in #44130

elasticmachine · 2019-09-07T01:13:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…onsistency

joelgriffith · 2019-09-09T19:42:28Z

x-pack/legacy/plugins/reporting/export_types/png/server/execute_job/index.js

@@ -40,7 +40,7 @@ function executeJobFn(server) {
      }),
      map(omitBlacklistedHeaders),
      map(getConditionalHeaders),
-      mergeMap(addForceNowQuerystring),
+      mergeMap(getFullUrls),


Nice rename here

It was still surprisingly hard to come up with ;)

x-pack/plugins/translations/translations/ja-JP.json

joelgriffith

Sorry for the red herring tests there. IIRC I quickly put those in to get CI builds green again :/ Goes to show how much good types help

elasticmachine · 2019-09-09T20:43:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

) * this is the one * this could break a job nbd * get_full_url to observe PNG and PDF job payload types * fix typescripts * fix ts in tests * fix more types * cosmetic * remove PDF execute compatibility shim test -- that stuff is handled in now PDF executeJob * fix unit test * remove old strings * Remove pdf execute compatibility shim entirely * combine the 2 maps * More reject matchers in the test

…-to-np-ready * 'master' of github.com:elastic/kibana: (138 commits) [Canvas] i18n work on workpad header (and a few header CTAs) and convert to typescript (elastic#44943) update close/delete system index modals (elastic#45037) TS return type of createIndexPatternSelect (elastic#45107) [ML] Fix focus chart updating. (elastic#45146) [ML] Data frame transform: Fix progress in wizard create step. (elastic#45116) [Graph] Re-enable functional test (elastic#44683) [SIEM] unique table id for each top talkers table (elastic#45014) [SIEM] ip details heading draggable (elastic#45179) [Maps][File upload] Set complete on index pattern creation (elastic#44423) [Maps] unmount map embeddable component on destroy (elastic#45183) [SIEM] Adds error toasts to MapEmbeddable component (elastic#45088) fix redirect to maintain search query string (elastic#45184) [APM] One-line trace summary (elastic#44842) [Infra UI] Display non-metric details on Node Detail page (elastic#43551) [Maps][File upload] Removing bbox from parsed file pending upstream lib fix (elastic#45194) [Logs UI] Improve live streaming behavior when scrolling (elastic#44923) [APM] Fix indefinite loading state in agent settings for unauthorized user roles (elastic#44970) [Reporting] Rewrite addForceNowQuerystring to getFullUrls (elastic#44851) [Reporting/ESQueue] Improve logging of doc-update events (elastic#45077) [Reporting] Make screenshot capture less noisy by default (elastic#45185) ...

…45205) * this is the one * this could break a job nbd * get_full_url to observe PNG and PDF job payload types * fix typescripts * fix ts in tests * fix more types * cosmetic * remove PDF execute compatibility shim test -- that stuff is handled in now PDF executeJob * fix unit test * remove old strings * Remove pdf execute compatibility shim entirely * combine the 2 maps * More reject matchers in the test

tsullivan added review (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services v7.5.0 labels Sep 5, 2019

tsullivan requested review from joelgriffith and a team September 5, 2019 06:23

tsullivan commented Sep 5, 2019

View reviewed changes

tsullivan force-pushed the reporting/screenshots-with-absolute-urls-consistency branch from 3c81e8b to 73e7992 Compare September 5, 2019 22:15

tsullivan force-pushed the reporting/screenshots-with-absolute-urls-consistency branch from 73e7992 to 08bee7f Compare September 6, 2019 05:14

tsullivan commented Sep 6, 2019

View reviewed changes

x-pack/legacy/plugins/reporting/export_types/common/execute_job/get_full_urls.ts Outdated

});

// why 2 maps

const urls = absoluteUrls.map((jobUrl: string) => {

This comment was marked as outdated.

Sign in to view

tsullivan commented Sep 6, 2019

View reviewed changes

tsullivan force-pushed the reporting/screenshots-with-absolute-urls-consistency branch from 08bee7f to acd5d04 Compare September 6, 2019 05:39

tsullivan commented Sep 6, 2019

View reviewed changes

tsullivan added 7 commits September 6, 2019 10:09

this is the one

aaaf651

this could break a job nbd

1737905

get_full_url to observe PNG and PDF job payload types

9a86dc3

fix typescripts

5c46d7d

fix ts in tests

f010372

fix more types

f162de7

cosmetic

ed93c41

tsullivan commented Sep 6, 2019

View reviewed changes

remove old strings

c23af64

tsullivan marked this pull request as ready for review September 6, 2019 19:04

Remove pdf execute compatibility shim entirely

0aae22f

tsullivan commented Sep 6, 2019

View reviewed changes

tsullivan removed the review label Sep 6, 2019

tsullivan changed the title ~~[Reporting] Rewrite addForceNowQuerystring to getFullUrls~~ [WIP] [Reporting] Rewrite addForceNowQuerystring to getFullUrls Sep 6, 2019

tsullivan added 2 commits September 6, 2019 16:51

combine the 2 maps

26b9431

More reject matchers in the test

96a2e3c

tsullivan commented Sep 7, 2019

View reviewed changes

tsullivan changed the title ~~[WIP] [Reporting] Rewrite addForceNowQuerystring to getFullUrls~~ [Reporting] Rewrite addForceNowQuerystring to getFullUrls Sep 7, 2019

tsullivan commented Sep 7, 2019

View reviewed changes

Merge branch 'master' into reporting/screenshots-with-absolute-urls-c…

c4e19dd

…onsistency

joelgriffith reviewed Sep 9, 2019

View reviewed changes

x-pack/plugins/translations/translations/ja-JP.json Show resolved Hide resolved

joelgriffith approved these changes Sep 9, 2019

View reviewed changes

tsullivan merged commit ce30118 into elastic:master Sep 9, 2019

tsullivan deleted the reporting/screenshots-with-absolute-urls-consistency branch September 9, 2019 21:04

This was referenced Sep 9, 2019

[7.x] [Reporting] Rewrite addForceNowQuerystring to getFullUrls (#44851) #45205

Merged

[Reporting/Typescript] Fix types for JobParamsPDF, BrowserConfig, ChromiumDriverFactory #45209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Rewrite addForceNowQuerystring to getFullUrls #44851

[Reporting] Rewrite addForceNowQuerystring to getFullUrls #44851

tsullivan commented Sep 5, 2019 •

edited

Loading

elasticmachine commented Sep 5, 2019

tsullivan Sep 5, 2019

tsullivan Sep 5, 2019 •

edited

Loading

elasticmachine commented Sep 5, 2019

elasticmachine commented Sep 5, 2019

tsullivan Sep 6, 2019

joelgriffith Sep 9, 2019

tsullivan Sep 9, 2019

tsullivan Sep 6, 2019

This comment was marked as outdated.

tsullivan Sep 6, 2019

tsullivan Sep 6, 2019 •

edited

Loading

tsullivan Sep 6, 2019

tsullivan Sep 6, 2019

elasticmachine commented Sep 6, 2019

tsullivan Sep 6, 2019

elasticmachine commented Sep 6, 2019

tsullivan Sep 6, 2019 •

edited

Loading

tsullivan Sep 6, 2019

tsullivan Sep 6, 2019

tsullivan Sep 9, 2019 •

edited

Loading

elasticmachine commented Sep 6, 2019

tsullivan Sep 7, 2019

tsullivan commented Sep 7, 2019

tsullivan Sep 7, 2019 •

edited

Loading

tsullivan Sep 7, 2019

tsullivan Sep 7, 2019 •

edited

Loading

elasticmachine commented Sep 7, 2019

joelgriffith Sep 9, 2019

tsullivan Sep 9, 2019

joelgriffith left a comment

elasticmachine commented Sep 9, 2019

		@@ -17,6 +17,18 @@ export interface JobParamsPDF {
		layout: LayoutInstance;

[Reporting] Rewrite addForceNowQuerystring to getFullUrls #44851

[Reporting] Rewrite addForceNowQuerystring to getFullUrls #44851

Conversation

tsullivan commented Sep 5, 2019 • edited Loading

Summary

Checklist

elasticmachine commented Sep 5, 2019

Choose a reason for hiding this comment

tsullivan Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2019

💔 Build Failed

elasticmachine commented Sep 5, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

tsullivan Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 6, 2019

💔 Build Failed

Choose a reason for hiding this comment

elasticmachine commented Sep 6, 2019

💚 Build Succeeded

tsullivan Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 6, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

tsullivan commented Sep 7, 2019

tsullivan Sep 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Sep 7, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 7, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelgriffith left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 9, 2019

💚 Build Succeeded

tsullivan commented Sep 5, 2019 •

edited

Loading

tsullivan Sep 5, 2019 •

edited

Loading

tsullivan Sep 6, 2019 •

edited

Loading

tsullivan Sep 6, 2019 •

edited

Loading

tsullivan Sep 9, 2019 •

edited

Loading

tsullivan Sep 7, 2019 •

edited

Loading

tsullivan Sep 7, 2019 •

edited

Loading