rename deprecation factory does not remove an empty object

[mshustov]

version: main branch
Cloud staging is failing after #120110 with an error:

[config validation of [xpack.reporting].capture.browser]: definition for this key is missing

It's been caused by a bunch of rename deprecations in https://github.com/elastic/kibana/blob/da78a9ecd5eecfae86a31ee652857006af4141a5/x-pack/plugins/screenshotting/server/config/index.ts.

Therefore, if Config contains at least a single property under xpack.reporting.capture.browser.* namespace, Kibana will move it under xpack.screenshotting.browser.*. However, the config validation will fail for xpack.reporting.capture.browser since it contains an empty object without a validation schema.
@elastic/kibana-reporting-services can't change the deprecation declaration from xpack.reporting.capture to xpack.screenshotting because it contains other properties besides browser.
Another option is to declare the rename operation on a level higher but it's rather a non-obvious workaround

    renameFromRoot(
      'xpack.reporting.capture.browser',
      'xpack.screenshotting.browser'
    ),

My suggestion is to extend rename functionality to remove an empty property after the rename operation.
now:

renameFromRoot( 'a.b.c', 'a.x.y')({ a: { b : { c: 1 } } }) === { a: { b: {} , x: { y: 1 } } }

with fix:

renameFromRoot( 'a.b.c', 'a.x.y')({ a: { b : { c: 1 } } }) === { a: { x: { y: 1 } } }

Downsides:
Some code may expect an empty object to present after the rename operation.

[elasticmachine]

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

[afharo]

++ to removing the empty properties. IMO, if renaming, the old name should completely go away.

Some code may expect an empty object to present after the rename operation.

Do we know how many cases are affected by this statement?

[pgayvallet]

Another option would be to allow rename and renameFromRoot to allow the destination to be present, and perform a merge. It may be a pain to handle the logic required in the ConfigDeprecationCommand return though + the fact that we'll probably need/want a deep merge, which always has an opinionated implementation regarding edge cases, so it's probably worse that the suggested solution.

I'm ++ to remove all parents of the source property of the rename operation that no longer have any remaining properties after the rename/move.

Some code may expect an empty object to present after the rename operation.

I would say it's unlikely for configuration schemas as most/all properties are optional by nature. We could eventually introduce an option for these deprecations to disable this 'delete the empty parents behavior', but I feel like this will not be necessary.

[Bamieh]

Maybe this step of removing empty objects can be done after all factory functions are run instead of doing something specific to rename. Because we will probably need something for unset and custom deprecations since they are no longer allowed to change the shape of the config object. And it helps keep things consistent

Another thing I have in mind but it might not be a concern is; Since the deprecation updates are run sequentially we might remove an object that will later get repopulated?

We also want to keep in mind that we are tracking deprecated configs through telemetry and we do not want to break the usage sent around that

[pgayvallet]

Because we will probably need something for unset and custom deprecations since they are no longer allowed to change the shape of the config object. And it helps keep things consistent

unset can return the parent's path instead of the property path when matching the condition, no?

e.g

renameFromRoot('foo.bar', 'hello.dolly');

would return, when the config is {foo: {bar : 42}}

{
      set: [{ path:  'hello.dolly', value: valueOfFooBar }],
      unset: [{ path: 'foo' }],
}

and when the config is {foo: {bar : 42, baz: 9000}}

{
      set: [{ path:  'hello.dolly', value: valueOfFooBar }],
      unset: [{ path: 'foo.bar' }],
}

Or am I missing something?

we might remove an object that will later get repopulated?

Wasn't this already the case when using rename to move non-leaf properties? ATM, Returning a set: [{ path: 'some.path.no.longer.present', value: 'over 9000'}] already recreate the parent I believe?

[afharo]

when looking at the actual implementation, I think that we need to apply this parent cleanup check after every unset. As @Bamieh mentioned, this issue can happen on rename and on unused as well.

I'm submitting the PR now #120889.

unset can return the parent's path instead of the property path when matching the condition, no?

@pgayvallet the problem I see with that approach is that renameFromRoot is not aware of other declared changes on the same object: In this example, they are renaming many individual keys under the same object. At the time of compiling the set/unset actions, it cannot know if the key will be the last one to move in the parent object.

[pgayvallet]

At the time of compiling the set/unset actions, it cannot know if the key will be the last one to move in the parent object.

We're applying the set and unset in-between calls to deprecations, so the next deprecation should be aware that the key it removes is the last one, as the config object passed is already updated by the previous deprecation:

kibana/packages/kbn-config/src/deprecation/apply_deprecations.ts

Lines 36 to 49 in 58bab91

	deprecations.forEach(({ deprecation, path, context }) => {
	const commands = deprecation(result, path, createAddDeprecation(path), context);
	if (commands) {
	if (commands.set) {
	changedPaths.set.push(...commands.set.map((c) => c.path));
	commands.set.forEach(function ({ path: commandPath, value }) {
	set(result, commandPath, value);
	});
	}
	if (commands.unset) {
	changedPaths.unset.push(...commands.unset.map((c) => c.path));
	commands.unset.forEach(function ({ path: commandPath }) {
	unset(result, commandPath);
	});

unless I'm missing something?

[afharo]

@pgayvallet you are right! I was misreading the code. Do you think it's best to change the behaviours of the APIs unused and rename instead of cleaning any empty parents on command.unset?

[pgayvallet]

Na, your implementation looks fine, and I don't think mine is better in any way since we don't want to add the option to enabled or disable the behavior from the deprecation's options (mostly wanted to understand, thanks!).