Migrate `joi` to `17.4.0` and adapt the codebase #99899

pgayvallet · 2021-05-12T10:56:34Z

Summary

Bump joi from 13.5.2 to 17.4.0, and adapt usages, mostly in @kbn/config-schema

From the joi upgrade performance benchmark done here gcanti/io-ts-benchmarks#6, we should expect a +~30% perf gain for successful validations, and ~200% for unsuccessful validations.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…tion-of-pain

pgayvallet

Self-review

pgayvallet · 2021-05-18T14:57:51Z

package.json

-    "joi": "^13.5.2",
+    "joi": "^17.4.0",


pgayvallet · 2021-05-18T15:02:25Z

packages/kbn-config-schema/src/types/buffer_type.test.ts

-  expect(() => schema.buffer().validate('abc')).toThrowErrorMatchingInlineSnapshot(
-    `"expected value of type [Buffer] but got [string]"`
-  );


joi.binary now converts strings to buffers (using the default encoding)

I thought it was fine (at least it didn't break anything), but if we want to forbid that, I can add our own coerce function for the binary type

pgayvallet · 2021-05-18T15:03:17Z

packages/kbn-config-schema/src/types/maybe_type.ts

-        .default(() => undefined, 'undefined')
+        .default(() => undefined)


No longer allowed to define a value description when setting a default value. We weren't using that anywhere anyway.

pgayvallet · 2021-05-18T15:05:12Z

packages/kbn-config-schema/src/types/stream_type.test.ts

        "_maxListeners": undefined,
+        Symbol(kCapture): false,


This snapshot change is caused by a fix in the internal deep copy function joi was using. Symbols are now properly copied.

Maybe we should refactor the test to assert against Stream instance?

pgayvallet · 2021-05-18T15:22:02Z

packages/kbn-config-schema/src/types/string_type.test.ts

@@ -71,14 +85,13 @@ describe('#hostname', () => {
    expect(hostNameSchema.validate('www.example.com')).toBe('www.example.com');
    expect(hostNameSchema.validate('3domain.local')).toBe('3domain.local');
    expect(hostNameSchema.validate('hostname')).toBe('hostname');
-    expect(hostNameSchema.validate('2387628')).toBe('2387628');


joi hostname validation was improved

DOMAIN_INVALID_TLDS_CHARS (last segment must start with alphanum) - https://github.com/sideway/address/blob/master/lib/domain.js#L88

Looking at the changelog for joi 17.0.0, it appears that they also added support for non-ascii chars in hostnames: hapijs/joi#2163 😄

pgayvallet · 2021-05-18T15:40:27Z

src/core/server/csp/config.test.ts

-      '[disableEmbedding.0]: expected value to equal [false]'
+      '[disableEmbedding]: expected value to equal [false]'


single-type alternative, so the error message changed (see #99899 (comment))

pgayvallet · 2021-05-18T15:42:42Z

src/core/server/http/http_config.ts

-      validate(value) {
-        if (value === '0') {
-          return 'value 0 is not a valid hostname (use "0.0.0.0" to bind to all interfaces)';
-        }
-      },


0 is now (correctly) considered as an invalid hostname by joi, so this custom validation is no longer required (and not invoked anyway if the value is 0).

We'll have to use a oneOf here when backporting to 7.x to allow 0, as the branch is still supposed to support this value.

pgayvallet · 2021-05-18T15:46:34Z

x-pack/plugins/reporting/server/config/schema.ts

-  hostname: schema.maybe(
-    schema.string({
-      validate(value) {
-        if (value === '0') {
-          return 'must not be "0" for the headless browser to correctly resolve the host';
-        }
-      },
-      hostname: true,
-    })
-  ),
+  hostname: schema.maybe(schema.string({ hostname: true })),


@elastic/kibana-reporting-services 0 is now correctly considered as an invalid hostname by joi. We're loosing the custom error message as the internal validation failing is performed before calling it.

Can you confirm this is acceptable?

pgayvallet · 2021-05-18T15:52:49Z

x-pack/plugins/security_solution/common/endpoint/schema/trusted_apps.ts

-const EntrySchemaDependingOnOS = schema.conditional(
+const EntriesSchema = schema.conditional(
  schema.siblingRef('os'),
  OperatingSystem.WINDOWS,
-  WindowsEntrySchema,
+  schema.arrayOf(WindowsEntrySchema, entriesSchemaOptions),
  schema.conditional(
    schema.siblingRef('os'),


@elastic/security-solution joi's reference implementation is now less permissive.

You were using a reference from the schema of the type of an array to reference a sibling of the whole array's type

{ os: schema.something(), entries: schema.arrayOf(schema.conditional(schema.siblingRef('os'), [...])) }

This is no longer allowed, so I had to move the conditional to wrap the whole arrayOf type instead. (thanks for the great test coverage on this file btw)

pgayvallet · 2021-05-18T15:55:02Z

packages/kbn-config-schema/src/internals/index.ts

+export const internals: JoiRoot = Joi.extend(
  {
-    name: 'boolean',
-
+    type: 'boolean',
    base: Joi.boolean(),


Pretty much everything in the extension API changed, I will not explain all the changes in this file, but feel free to ask questions if you want info on some specific parts.

elasticmachine · 2021-05-18T17:24:04Z

Pinging @elastic/kibana-core (Team:Core)

jportner

Code review only, security changes (src/core/server/csp/config.test.ts) LGTM.

I also had a look at the Joi changelog and the breaking changes between major versions, nothing stood out to me.

jbudz

kbn-test changes LGTM

mshustov

mshustov · 2021-05-19T08:23:45Z

packages/kbn-config-schema/src/internals/index.ts

      }
-      if (options.convert && typeof value === 'string') {
+      if (prefs.convert && typeof value === 'string') {


Are we relying on joi defaults for convert? Maybe we shouldn't?

prefs.convert defaults to true, and config-schema doesn't provide any way to change this value, so effectively this part of the condition could be removed. I thought it was better to keep it, just to kinda preserve consistency with how the underlying library is working. But maybe not.

mshustov · 2021-05-19T08:29:03Z

packages/kbn-config-schema/src/types/stream_type.test.ts

        "_maxListeners": undefined,
+        Symbol(kCapture): false,


Maybe we should refactor the test to assert against Stream instance?

packages/kbn-config-schema/src/types/type.ts

packages/kbn-config-schema/src/types/union_type.test.ts

packages/kbn-config-schema/types/joi.d.ts

paul-tavares

🔥

paul-tavares · 2021-05-19T19:34:58Z

x-pack/plugins/security_solution/common/endpoint/schema/trusted_apps.ts

  schema.siblingRef('os'),
  OperatingSystem.WINDOWS,
-  WindowsEntrySchema,
+  schema.arrayOf(WindowsEntrySchema, entriesSchemaOptions),


LGTM. Thanks @pgayvallet

@dasansol92 FYI ☝️

wylieconlon · 2021-05-19T19:36:22Z

@pgayvallet I tested the performance impact of this on a dashboard with dozens of panels and found that it was ~30% faster, as you said. I didn't do any code review, but that's great!

…tion-of-pain

kibanamachine · 2021-05-20T08:16:12Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`remoteClusters`	4	-	-4

Unknown metric groups

API count

id	before	after	diff
`remoteClusters`	4	-	-4

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`infra`	261	149	-112
`lens`	67	45	-22
`licensing`	18	15	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`stackAlerts`	101	95	-6
total			-295

History

💚 Build #126396 succeeded d1c61fe
💚 Build #126383 succeeded 76d5445
💔 Build #126352 failed d3617ed
💔 Build #125696 failed 3aa3b71
💔 Build #125651 failed 73bd9f6

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

* bump joi to 17.4.0, start adapting stuff * remove custom validation rule, adapt instead * fix error handling * fix error handling again * fix strings type & validation * fix buffers and arrays * fix bytes * fix bytes_size type * update conditional_type error messages in tests * fix duration and map types * first attempt to fix union type error messages * revert conditional type assertions back to master state * fix object type * fix record type * fix stream types * rename test files to match sources * fix union type tests * temporary adapt feature/home usages of Joi * fix lint * adapt test assertion * fix http config schema validation * fix @kbn/test Config class * fix config again * fix reporting schema tests * fix security solution schema * adapt url tests * remove useless comment * remove space * typo * review comments # Conflicts: # src/core/server/http/__snapshots__/http_config.test.ts.snap

) * Migrate `joi` to `17.4.0` and adapt the codebase (#99899) * bump joi to 17.4.0, start adapting stuff * remove custom validation rule, adapt instead * fix error handling * fix error handling again * fix strings type & validation * fix buffers and arrays * fix bytes * fix bytes_size type * update conditional_type error messages in tests * fix duration and map types * first attempt to fix union type error messages * revert conditional type assertions back to master state * fix object type * fix record type * fix stream types * rename test files to match sources * fix union type tests * temporary adapt feature/home usages of Joi * fix lint * adapt test assertion * fix http config schema validation * fix @kbn/test Config class * fix config again * fix reporting schema tests * fix security solution schema * adapt url tests * remove useless comment * remove space * typo * review comments # Conflicts: # src/core/server/http/__snapshots__/http_config.test.ts.snap * allow '0' value for server.host * fix config def * update snapshots

* bump joi to 17.4.0, start adapting stuff * remove custom validation rule, adapt instead * fix error handling * fix error handling again * fix strings type & validation * fix buffers and arrays * fix bytes * fix bytes_size type * update conditional_type error messages in tests * fix duration and map types * first attempt to fix union type error messages * revert conditional type assertions back to master state * fix object type * fix record type * fix stream types * rename test files to match sources * fix union type tests * temporary adapt feature/home usages of Joi * fix lint * adapt test assertion * fix http config schema validation * fix @kbn/test Config class * fix config again * fix reporting schema tests * fix security solution schema * adapt url tests * remove useless comment * remove space * typo * review comments

pgayvallet added 22 commits May 11, 2021 21:29

bump joi to 17.4.0, start adapting stuff

eefd427

remove custom validation rule, adapt instead

a148a25

fix error handling

f7591b2

fix error handling again

6c86026

fix strings type & validation

e23bcb0

fix buffers and arrays

a8789e4

fix bytes

1c454eb

fix bytes_size type

f4578fa

update conditional_type error messages in tests

5eab450

fix duration and map types

831649f

first attempt to fix union type error messages

e76029d

revert conditional type assertions back to master state

a6fd0e6

fix object type

83d41ab

fix record type

69007a3

fix stream types

56eb879

rename test files to match sources

cbf105e

fix union type tests

c238519

temporary adapt feature/home usages of Joi

1783023

fix lint

f344583

adapt test assertion

acf5073

fix http config schema validation

3513b24

fix @kbn/test Config class

73bd9f6

pgayvallet force-pushed the kbn-xxx-joi-migration-of-pain branch from b85a955 to 73bd9f6 Compare May 12, 2021 17:33

fix config again

3aa3b71

mshustov mentioned this pull request May 17, 2021

Migrate from Joi to @kbn/config-schema in "home" and "features" plugins #100201

Merged

2 tasks

pgayvallet mentioned this pull request May 18, 2021

Upgrade Joi version or remove dependency on it #84624

Closed

pgayvallet added 3 commits May 18, 2021 14:02

Merge remote-tracking branch 'upstream/master' into kbn-xxx-joi-migra…

d3617ed

…tion-of-pain

fix reporting schema tests

1dfe1b0

fix security solution schema

76d5445

pgayvallet added the performance label May 18, 2021

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.14.0 labels May 18, 2021

pgayvallet added 4 commits May 18, 2021 17:20

adapt url tests

344926d

remove useless comment

0952a6d

remove space

645f9fb

typo

d1c61fe

pgayvallet commented May 18, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review May 18, 2021 17:24

pgayvallet requested review from a team as code owners May 18, 2021 17:24

jportner approved these changes May 18, 2021

View reviewed changes

jbudz approved these changes May 18, 2021

View reviewed changes

mshustov approved these changes May 19, 2021

View reviewed changes

paul-tavares approved these changes May 19, 2021

View reviewed changes

pgayvallet added 2 commits May 20, 2021 08:13

Merge remote-tracking branch 'upstream/master' into kbn-xxx-joi-migra…

d0ff45d

…tion-of-pain

review comments

c69cbf1

pgayvallet merged commit 8fc9115 into elastic:master May 20, 2021

pgayvallet mentioned this pull request May 20, 2021

[7.x] Migrate joi to 17.4.0 and adapt the codebase (#99899) #100378

Merged

axw mentioned this pull request May 25, 2021

7.14.0-SNAPSHOT Docker container won't start #100533

Closed

lukeelmers mentioned this pull request Aug 3, 2022

@kbn/config-schema is slow #78351

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `joi` to `17.4.0` and adapt the codebase #99899

Migrate `joi` to `17.4.0` and adapt the codebase #99899

pgayvallet commented May 12, 2021 •

edited

Loading

pgayvallet left a comment

pgayvallet May 18, 2021

pgayvallet May 18, 2021

pgayvallet May 18, 2021

pgayvallet May 18, 2021

mshustov May 19, 2021

pgayvallet May 18, 2021

jportner May 18, 2021

pgayvallet May 18, 2021 •

edited

Loading

pgayvallet May 18, 2021

pgayvallet May 18, 2021

pgayvallet May 18, 2021 •

edited

Loading

pgayvallet May 18, 2021

elasticmachine commented May 18, 2021

jportner left a comment

jbudz left a comment

mshustov left a comment

mshustov May 19, 2021 •

edited

Loading

pgayvallet May 19, 2021

mshustov May 19, 2021

paul-tavares left a comment

paul-tavares May 19, 2021

wylieconlon commented May 19, 2021

kibanamachine commented May 20, 2021

API count

References to deprecated APIs

		.default(() => undefined, 'undefined')
		.default(() => undefined)

		'[disableEmbedding.0]: expected value to equal [false]'
		'[disableEmbedding]: expected value to equal [false]'

Migrate joi to 17.4.0 and adapt the codebase #99899

Migrate joi to 17.4.0 and adapt the codebase #99899

Conversation

pgayvallet commented May 12, 2021 • edited Loading

Summary

Checklist

pgayvallet left a comment

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

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

pgayvallet May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 18, 2021

jportner left a comment

Choose a reason for hiding this comment

jbudz left a comment

Choose a reason for hiding this comment

mshustov left a comment

Choose a reason for hiding this comment

mshustov May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wylieconlon commented May 19, 2021

kibanamachine commented May 20, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

References to deprecated APIs

History

Migrate `joi` to `17.4.0` and adapt the codebase #99899

Migrate `joi` to `17.4.0` and adapt the codebase #99899

pgayvallet commented May 12, 2021 •

edited

Loading

pgayvallet May 18, 2021 •

edited

Loading

pgayvallet May 18, 2021 •

edited

Loading

mshustov May 19, 2021 •

edited

Loading