adds strict types to Alerting Client #53821

gmmorris · 2019-12-27T12:44:50Z

Summary

The AlertsClient API currently returns mixed inferred types instead of a clear strict type, making it harder to work with the client's type signatures.
The root causes for this difficulty is that we have to support the SavedObjects API which allows partial updates of types, and the implementation of code that converts the SavedObject from a RawAlert to an Alert in a non type-strict manner.

To address this we've added concrete types on the AlertsClient APIs, using Partial on update due to the SavedObjects API, and a strict Alert on the other APIs.

closes #49703

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~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

* master: Possibility to filter when testing scripted fields (elastic#35379) (elastic#44220) Update maps telemetry mappings to account for recent updates (elastic#53803) [Maps] Only show legend when layer is visible (elastic#53781) remove use of experimental fs.promises api (elastic#53346) [APM] Add log statements for flaky test (elastic#53775)

kibanamachine · 2019-12-27T14:05:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 430fe9b

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

elasticmachine · 2019-12-27T17:14:28Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

kibanamachine · 2019-12-27T18:26:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f6b0dfb

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

* master: Allow chromeless applications to render via non-/app routes (elastic#51527) Add server rendering service to enable standalone route rendering (elastic#52161)

…ris/kibana into alerting/created_at-and-updated_at * 'alerting/created_at-and-updated_at' of github.com:gmmorris/kibana: Allow chromeless applications to render via non-/app routes (elastic#51527) Add server rendering service to enable standalone route rendering (elastic#52161)

* master: Bump year in NOTICE.txt Add kibanamachine support to Github PR comments (elastic#53852) Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333) Skip failing test suite [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799) Do not remount applications between page navigations (elastic#53851) [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129) [Canvas] Migrate usage collector to NP plugin (elastic#53303)

pmuellr

LGTM; some nit notes on the definition of PartialAlert, and potential typo in the name of a variable in a test

pmuellr · 2020-01-02T20:36:16Z

x-pack/legacy/plugins/alerting/server/types.ts

  apiKey: string | null;
  apiKeyOwner: string | null;
  throttle: string | null;
  muteAll: boolean;
  mutedInstanceIds: string[];
 }

+export type PartialAlert = Pick<Alert, 'id'> & Partial<Omit<Alert, 'id'>>;


could this be shorted to:

export type PartialAlert = Pick<Alert, 'id'> & Partial<Alert>;

Seems like TS usually does a good job allowing some "slop" here - basically, id is defined twice - required on the left type, optional on the right, so . ... it's required in the final type, and the types of id are the same so ... "it works". Not quite sure tho :-)

I feel like there must be a good, simple pattern for building and naming (heh) these sort of structures, but I've not quite figured it out yet ...

Sadly no, because that results in id becoming string | undefined which is exactly what we don't want here...

pmuellr · 2020-01-02T20:43:18Z

x-pack/legacy/plugins/alerting/server/routes/create.test.ts

@@ -30,6 +31,17 @@ const mockedAlert = {
  ],
 };

+const mockedAlertComputerFields = {


Computer fields? I was thinking maybe that was a mistype of Computed, but that doesn't seem quite right either ...

Yeah, was meant to be computed... inlined the variable, it isn't really needed as a stand alone

…t-types * alerting/created_at-and-updated_at: fixed misuse of null on updatedAt used createdAt in place of empty updatedAt added missing assetion

…t-types * alerting/created_at-and-updated_at: fixed tests fixed tests

…ris/kibana into alerting/created_at-and-updated_at * 'alerting/created_at-and-updated_at' of github.com:gmmorris/kibana: Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use [email protected] (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)

* master: Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)

…t-types * alerting/created_at-and-updated_at: updatedAt should equal createdAt on creation Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use [email protected] (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)

…t-types * alerting/created_at-and-updated_at: removed assertions for updatedAt in non-update operations

* master: [SR] Enable component integration tests (elastic#53893)

* master: [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

mikecote

Changes LGTM! It would be nice to have the update function return a full alert someday but that is out of context for this PR 👍

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798)

kibanamachine · 2020-01-06T11:08:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8f5c13d

History

💚 Build #17826 succeeded ae0314d
💚 Build #17801 succeeded d0452b2
💚 Build #17792 succeeded 543c057
💔 Build #17759 failed b04189e
💚 Build #17727 succeeded c4817e5

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

The AlertsClient API currently returns mixed inferred types instead of a clear strict type, making it harder to work with the client's type signatures. The root causes for this difficulty is that we have to support the SavedObjects API which allows partial updates of types, and the implementation of code that converts the SavedObject from a RawAlert to an Alert in a non type-strict manner. To address this we've added concrete types on the AlertsClient APIs, using Partial on update due to the SavedObjects API, and a strict Alert on the other APIs.

* master: adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768)

…nsole-dependencies * 'master' of github.com:elastic/kibana: (33 commits) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768) increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) ... # Conflicts: # yarn.lock

The AlertsClient API currently returns mixed inferred types instead of a clear strict type, making it harder to work with the client's type signatures. The root causes for this difficulty is that we have to support the SavedObjects API which allows partial updates of types, and the implementation of code that converts the SavedObject from a RawAlert to an Alert in a non type-strict manner. To address this we've added concrete types on the AlertsClient APIs, using Partial on update due to the SavedObjects API, and a strict Alert on the other APIs.

* master: allows Alerts to recover gracefully from Executor errors (elastic#53688) [Console] Fix OSS build (elastic#53885) migrate xsrf / version-check / custom-headers handlers to NP (elastic#53684) use NP deprecations in uiSettings (elastic#53755) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768)

gmmorris added 3 commits December 24, 2019 13:14

added createdAt and updatedAt fields in alerting

dcd574e

use the SavedObjets updated_at instead of a new field in alerting

1721ec2

gmmorris changed the title ~~Alerting/client types~~ [DRAFT] adds strict types to Alerting Client Dec 27, 2019

added concrete types on AlertClients public APIs

f6b0dfb

gmmorris added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.6.0 v8.0.0 labels Dec 27, 2019

gmmorris changed the title ~~[DRAFT] adds strict types to Alerting Client~~ adds strict types to Alerting Client Dec 30, 2019

gmmorris marked this pull request as ready for review December 30, 2019 09:55

gmmorris and others added 7 commits December 30, 2019 10:30

removed generics in AlertsClient

36ea491

Merge branch 'master' into alerting/client-types

be3b3a2

* master: Allow chromeless applications to render via non-/app routes (elastic#51527) Add server rendering service to enable standalone route rendering (elastic#52161)

Merge branch 'master' into alerting/created_at-and-updated_at

b6c9385

added missing assetion

e8dbc15

pmuellr approved these changes Jan 2, 2020

View reviewed changes

gmmorris added 7 commits January 3, 2020 09:56

used createdAt in place of empty updatedAt

1fb5735

fixed misuse of null on updatedAt

2f50541

Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…

1457a5a

…t-types * alerting/created_at-and-updated_at: fixed misuse of null on updatedAt used createdAt in place of empty updatedAt added missing assetion

cleaned up tests

19063db

fixed tests

5a4057d

fixed tests

954a71a

Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…

c4817e5

…t-types * alerting/created_at-and-updated_at: fixed tests fixed tests

elasticmachine and others added 9 commits January 3, 2020 03:48

Merge branch 'master' into alerting/created_at-and-updated_at

db0d578

updatedAt should equal createdAt on creation

924393f

Merge branch 'master' into alerting/created_at-and-updated_at

cf24b03

* master: Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)

removed assertions for updatedAt in non-update operations

6dff233

Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…

543c057

…t-types * alerting/created_at-and-updated_at: removed assertions for updatedAt in non-update operations

Merge branch 'master' into alerting/client-types

d0452b2

* master: [SR] Enable component integration tests (elastic#53893)

Merge branch 'master' into alerting/client-types

ae0314d

* master: [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

mikecote approved these changes Jan 3, 2020

View reviewed changes

gmmorris merged commit 8992a43 into elastic:master Jan 6, 2020

gmmorris mentioned this pull request Jan 6, 2020

[7.x] adds strict types to Alerting Client (#53821) #54007

Merged

gmmorris added the backported label Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds strict types to Alerting Client #53821

adds strict types to Alerting Client #53821

gmmorris commented Dec 27, 2019 •

edited

Loading

kibanamachine commented Dec 27, 2019

elasticmachine commented Dec 27, 2019

kibanamachine commented Dec 27, 2019

pmuellr left a comment

pmuellr Jan 2, 2020

gmmorris Jan 3, 2020

pmuellr Jan 2, 2020

gmmorris Jan 3, 2020

mikecote left a comment

kibanamachine commented Jan 6, 2020

adds strict types to Alerting Client #53821

adds strict types to Alerting Client #53821

Conversation

gmmorris commented Dec 27, 2019 • edited Loading

Summary

Checklist

For maintainers

kibanamachine commented Dec 27, 2019

💚 Build Succeeded

elasticmachine commented Dec 27, 2019

kibanamachine commented Dec 27, 2019

💚 Build Succeeded

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

gmmorris Jan 3, 2020

Choose a reason for hiding this comment

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

gmmorris Jan 3, 2020

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 6, 2020

💚 Build Succeeded

History

gmmorris commented Dec 27, 2019 •

edited

Loading