[Reporting] Separate internal and public API endpoints in Reporting by tsullivan · Pull Request #162288 · elastic/kibana

tsullivan · 2023-07-19T23:25:35Z

Summary

Closes #160827
Closes #134517
Closes #149942

In this PR, the following API endpoints are moved into an internal namespace:

New endpoint path	Previous
`/internal/reporting/diagnose/browser`	`/api/reporting/diagnose/browser`
`/internal/reporting/diagnose/screenshot`	`/api/reporting/diagnose/screenshot`
`/internal/reporting/generate/immediate/csv_searchsource`	`/api/reporting/v1/generate/immediate/csv_searchsource`
`/internal/reporting/generate/{exportTypeId}`	`/api/reporting/generate/{exportTypeId}`
`/internal/reporting/jobs/count`	`/api/reporting/jobs/count`
`/internal/reporting/jobs/delete/{jobId}`	`/api/reporting/jobs/delete/{jobId}`
`/internal/reporting/jobs/info/{jobId}`	`/api/reporting/jobs/info/{jobId}`
`/internal/reporting/jobs/list`	`/api/reporting/jobs/list`

Support for the public APIs continues:

Public endpoint path
`/api/reporting/generate/{exportTypeId}`
`/api/reporting/jobs/delete/{jobId}`
`/api/reporting/jobs/download/{jobId}`

Other changes

Set access options on the routes
Removed API Counter functional tests, which were skipped to begin with.
Replaced functional tests with Jest integration tests.
Consolidated code in the generation routes by creating the getJobParams method of the RequestHandler class.
Added a new test for getJobParams
Consolidated code in the job management routes
Added new code for shared helpers in job management routes

Reorganized libs used for route handlers:

routes/lib/request_handler.ts             =>  routes/common/generate/request_handler.ts                              
routes/lib/job_management_pre_routing.ts  =>  routes/common/jobs/job_management_pre_routing.ts
routes/lib/jobs_query.ts                  =>  routes/common/jobs/jobs_query.ts
routes/lib/get_document_payload.ts        =>  routes/common/jobs/get_document_payload.ts
routes/lib/get_counter.ts                 =>  routes/common/get_counter.ts
routes/lib/authorized_user_pre_routing.ts =>  routes/common/authorized_user_pre_routing.ts
routes/lib/get_user.ts                    =>  routes/common/get_user.ts

Release Note

Updated API endpoint paths for Reporting to clarify which routes are public and which are not. Make sure that any custom script or application that uses Reporting endpoints only uses the public endpoints.

tsullivan · 2023-07-19T23:26:44Z

x-pack/plugins/reporting/public/lib/reporting_api_client/reporting_api_client.ts

deleting a report is manually done by the user: not a system request

tsullivan · 2023-07-19T23:27:02Z

x-pack/plugins/reporting/public/lib/reporting_api_client/reporting_api_client.ts

getting report info is manually done by the user: not a system request

tsullivan · 2023-07-19T23:27:26Z

x-pack/plugins/reporting/public/lib/reporting_api_client/reporting_api_client.ts

using the diagnostic tool is manually done by the user: not a system request

tsullivan · 2023-07-19T23:28:24Z

...gins/reporting/public/share_context_menu/reporting_panel_content/reporting_panel_content.tsx

I can't figure out why my editor says this line uses a deprecated signature, so the comment isn't valid

Nice I'm also not seeing any issues in my editor 👍🏻

…-ref HEAD~1..HEAD --fix'

tsullivan · 2023-07-20T23:07:08Z

x-pack/plugins/reporting/public/lib/reporting_api_client/reporting_api_client.test.ts


-      expect(httpClient.post).toHaveBeenCalledWith(
-        expect.stringContaining('/diagnose/screenshot'),
-        expect.any(Object)


the options object was removed from the API calls for many of these, since they mistakenly were used to send the asSystemRequest field

tsullivan · 2023-07-27T23:48:49Z

x-pack/plugins/reporting/server/routes/common/generate/request_handler.ts

-    counters: Counters
-  ) {
+  public getJobParams(): BaseParams {
+    let jobParamsRison: null | string = null;


I've added this helper because without it, this section of code would have had to be duplicated in x-pack/plugins/reporting/server/routes/internal/generate/generate_from_jobparams.ts and x-pack/plugins/reporting/server/routes/public/generate_from_jobparams.ts

tsullivan · 2023-07-27T23:52:04Z

x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts

+  res: KibanaResponseFactory;
+}
+
+export const commonJobsRouteHandlerFactory = (reporting: ReportingCore) => {


Added these helpers to avoid duplicated code in x-pack/plugins/reporting/server/routes/internal/management/jobs.ts and x-pack/plugins/reporting/server/routes/public/jobs.ts

tsullivan · 2023-07-27T23:53:37Z

x-pack/plugins/reporting/server/routes/internal/generate/generate_from_jobparams.ts

+  // TODO: find a way to abstract this using ExportTypeRegistry: it needs a new
+  // public method to return this array
+  // const registry = reporting.getExportTypesRegistry();
+  // const kibanaAccessControlTags = registry.getAllAccessControlTags();


I'm not sure if we still care about this, I just copied this comment from x-pack/plugins/reporting/server/routes/generate/generate_from_jobparams.ts

I am also tempted to remove this comment - I don't think it's really relevant anymore

tsullivan · 2023-07-28T00:12:38Z

x-pack/plugins/reporting/server/routes/internal/management/jobs.ts

@@ -7,29 +7,26 @@

 import { schema } from '@kbn/config-schema';
 import { ROUTE_TAG_CAN_REDIRECT } from '@kbn/security-plugin/server';
-import { promisify } from 'util';


A file was deleted and made into 2 versions, but Github shows this as a rename, so this diff makes a good outline of the goals of this PR:

Use constants for the route path strings

Use helpers within route handler functions to avoid duplicated code

rshen91

I love the constants/routes file making everything so much clearer

rshen91 · 2023-07-28T14:17:14Z

...gins/reporting/public/share_context_menu/reporting_panel_content/reporting_panel_content.tsx

Nice I'm also not seeing any issues in my editor 👍🏻

rshen91 · 2023-07-28T14:28:33Z

x-pack/plugins/reporting/server/routes/internal/generate/csv_searchsource_immediate.ts


-const API_BASE_URL_V1 = '/api/reporting/v1';
-const API_BASE_GENERATE_V1 = `${API_BASE_URL_V1}/generate`;
+const path = INTERNAL_ROUTES.DOWNLOAD_CSV;


I love that there's the constants/routes file in this PR to reference these vs it being spread across the plugin

rshen91 · 2023-07-28T14:29:21Z

x-pack/plugins/reporting/server/routes/internal/generate/generate_from_jobparams.ts

+  // TODO: find a way to abstract this using ExportTypeRegistry: it needs a new
+  // public method to return this array
+  // const registry = reporting.getExportTypesRegistry();
+  // const kibanaAccessControlTags = registry.getAllAccessControlTags();


I am also tempted to remove this comment - I don't think it's really relevant anymore

elasticmachine · 2023-07-28T16:31:09Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

kibana-ci · 2023-07-31T21:05:45Z

💚 Build Succeeded

Buildkite Build
Commit: 248219d

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`reporting`	90	91	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`reporting`	59.1KB	59.1KB	+6.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`reporting`	42.8KB	43.1KB	+308.0B

History

💚 Build #145668 succeeded 44c9f03
💚 Build #145371 succeeded 760bceb
💚 Build #145317 succeeded 53ac996
💚 Build #145243 succeeded 5cfda74
💔 Build #145090 failed 80ae40a
💔 Build #145083 failed 73500d4

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tsullivan

…lastic#162288) ## Summary Closes elastic#160827 Closes elastic#134517 Closes elastic#149942 In this PR, the following API endpoints are moved into an internal namespace: | New endpoint path | Previous | |---|---| | `/internal/reporting/diagnose/browser` | `/api/reporting/diagnose/browser` | | `/internal/reporting/diagnose/screenshot` | `/api/reporting/diagnose/screenshot` | | `/internal/reporting/generate/immediate/csv_searchsource` | `/api/reporting/v1/generate/immediate/csv_searchsource` | | `/internal/reporting/generate/{exportTypeId}` | `/api/reporting/generate/{exportTypeId}` | | `/internal/reporting/jobs/count` | `/api/reporting/jobs/count` | | `/internal/reporting/jobs/delete/{jobId}` | `/api/reporting/jobs/delete/{jobId}` | | `/internal/reporting/jobs/info/{jobId}` | `/api/reporting/jobs/info/{jobId}` | | `/internal/reporting/jobs/list` | `/api/reporting/jobs/list` | Support for the public APIs continues: | Public endpoint path | |---| | `/api/reporting/generate/{exportTypeId}` | | `/api/reporting/jobs/delete/{jobId}` | | `/api/reporting/jobs/download/{jobId}` | ## Other changes 1. Set access options on the routes 2. Removed API Counter functional tests, which were skipped to begin with. 3. Replaced functional tests with Jest integration tests. 4. Consolidated code in the generation routes by creating the `getJobParams` method of the `RequestHandler` class. 5. Added a new test for `getJobParams` 6. Consolidated code in the job management routes 7. Added new code for shared helpers in job management routes 8. Reorganized libs used for route handlers: ``` routes/lib/request_handler.ts => routes/common/generate/request_handler.ts routes/lib/job_management_pre_routing.ts => routes/common/jobs/job_management_pre_routing.ts routes/lib/jobs_query.ts => routes/common/jobs/jobs_query.ts routes/lib/get_document_payload.ts => routes/common/jobs/get_document_payload.ts routes/lib/get_counter.ts => routes/common/get_counter.ts routes/lib/authorized_user_pre_routing.ts => routes/common/authorized_user_pre_routing.ts routes/lib/get_user.ts => routes/common/get_user.ts ``` ## Release Note Updated API endpoint paths for Reporting to clarify which routes are public and which are not. Make sure that any custom script or application that uses Reporting endpoints only uses the public endpoints. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

tsullivan commented Jul 19, 2023

View reviewed changes

tsullivan force-pushed the reporting/use-internal-endpoints branch 2 times, most recently from 4e7152f to c9941b7 Compare July 20, 2023 02:11

tsullivan self-assigned this Jul 20, 2023

tsullivan added zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// labels Jul 20, 2023

tsullivan and others added 18 commits July 20, 2023 15:30

prep to use internal prefix for APIs

51b049a

internal and public routes objects

8a859d8

UI Api Client mistakenly set everything asSystemRequest

27ae018

Use the right route paths

f09c9b7

use the public API endpoint in the POST URL

d59f0d8

fix public download path for post response

d1710b5

fix tests import

01c6b5d

update comment

8ef4e04

Use more correct endpoint paths in tests

a6950af

Delete endpoint should be public

9379f44

reorganize part i

0594845

register public routes

264b644

fix tests

99ba556

update snapshot

6d46174

fix tests

fc30dee

Tests

664aaa4

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

167bc55

…-ref HEAD~1..HEAD --fix'

Fix tests

61c611e

tsullivan force-pushed the reporting/use-internal-endpoints branch from fffb568 to 61c611e Compare July 20, 2023 22:55

fix more tests

40f4721

tsullivan commented Jul 20, 2023

View reviewed changes

tsullivan force-pushed the reporting/use-internal-endpoints branch from 4305bc8 to 5122ed6 Compare July 27, 2023 23:43

tsullivan commented Jul 27, 2023

View reviewed changes

polish diff

760bceb

tsullivan commented Jul 28, 2023

View reviewed changes

rshen91 approved these changes Jul 28, 2023

View reviewed changes

tsullivan marked this pull request as ready for review July 28, 2023 16:31

tsullivan requested a review from a team as a code owner July 28, 2023 16:31

rshen91 approved these changes Jul 28, 2023

View reviewed changes

Merge branch 'main' into reporting/use-internal-endpoints

0f68a4c

tsullivan added the release_note:skip Skip the PR/issue when compiling release notes label Jul 28, 2023

tsullivan added 2 commits July 28, 2023 10:10

remove unimportant TODOs

5afefd4

add jest testing for requestHandler.getJobParams

0400bc7

tsullivan requested a review from a team July 28, 2023 17:35

tsullivan added 2 commits July 28, 2023 11:12

add access:internal option to internal route registrations

9bbc650

Merge branch 'main' into reporting/use-internal-endpoints

44c9f03

tsullivan added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 28, 2023

Merge branch 'main' into reporting/use-internal-endpoints

248219d

tsullivan merged commit a2e4279 into elastic:main Aug 1, 2023

kibanamachine added v8.10.0 backport:skip This PR does not require backporting labels Aug 1, 2023

tsullivan deleted the reporting/use-internal-endpoints branch August 1, 2023 15:24

tsullivan mentioned this pull request Aug 1, 2023

[Reporting] Ensure that API endpoints are not directly exposing persistence schemas #152870

Closed

rhr323 mentioned this pull request Sep 4, 2023

Telemetry test cases support 8.10 and later Kibana versions elastic/cloud-on-k8s#7150

Merged

tsullivan mentioned this pull request Nov 20, 2023

Migrate from path based versioning to header based versioning #159181

Open

Conversation

tsullivan commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Other changes

Release Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshen91 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 28, 2023

Uh oh!

kibana-ci commented Jul 31, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tsullivan commented Jul 19, 2023 •

edited

Loading

tsullivan Jul 19, 2023 •

edited

Loading

tsullivan Jul 27, 2023 •

edited

Loading

tsullivan Jul 28, 2023 •

edited

Loading