fix(fsrepo): deep merge when setting config

[kylehuntsman]

The Problem

Key retrieval for config keys that were flagged with the omitempty json encoding option would throw an error when setting/getting those keys when the values were considered empty.

$ ipfs config --json Swarm.DisableRelay true
$ ipfs config --json Swarm.DisableRelay false
Error: failed to get config value: "Swarm key has no attributes"

When a config key is set, a config struct is being merged with a map read from disk to ensure user provided keys are not removed in the process of trying to write a valid config file. The merging implementation is only updating root keys, overwriting any child values in the map read from disk with the values in the map from the config struct. The problem with this being that the marshalling of a config struct to a map omits empty values. Upon retrieval, the key does not exist because the merge removes it.

From the encoding/json package in the Go docs:

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

The Solution

The merge when setting a config now does a deep merge of nested map properties. This ensures that nested properties that are flagged as 'omitempty' in the map created from the config struct are not touched, while all other values can be updated.

• Fixes Unnecessary error when clearing omitempty fields using ipfs config #8088
• Fixes Cannot "get" values omitted from the config. #7482

Additionally...

This PR includes a commit that improves the error response when a key is not found. Initially discussed in #8628, but ultimately including here.

[welcome]

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

• The context for this PR is clear, with relevant discussion, decisions
  and stakeholders linked/mentioned.

• Your contribution itself is clear (code comments, self-review for the
  rest) and in its best form. Follow the code contribution
  guidelines
  if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

• A maintainer will triage and assign priority to this PR, commenting on
  any missing things and potentially assigning a reviewer for high
  priority items.

• The PR gets reviews, discussed and approvals as needed.

• The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

[kylehuntsman]

So I see some of the sharness tests failing. I wasn't quite sure how to run those or where to put new tests, but now that I see them failing, I found where to look. I'll work on getting those to pass and amend a commit.

[BigLep]

@kylehuntsman : I moved this back to draft for now. Feel free to publish again when tests are passing. I haven't looked closely at this, but my guess is we should use @schomatis as the reviewer.

[schomatis]

I'm not super sold on this solution but will go with it if we fix the reflection manipulations and the tests pass.

Still, let me give one last plug to what I described (very poorly) in #8088 (comment): we should be reading/writing generic JSON nested maps in the config file to avoid the weird omit-empty dynamic, never writing encoded structs to the file. We should decode into the go-ipfs config structure only to validate the generic map is correct before writing it (to make sure when we read it back we can safely decode it and feed it to the daemon).

[kylehuntsman]

Given the context that I now have after diving into this further, I understand what you are saying. Your description here also does make it a little more clear as to what you were talking about. 😅

I had this big write up I was going to post about the weirdness with some of the config types that I was having after retrieving them via reflect, and how I was proposing to solve those problems, specifically the Strings type. It would have been a brittle solution with control flows for that type specifically called out though, but it would have worked.

I see the generic map solution being easier to reason through by someone with less context. It's also more similar to the existing logic than the reflect logic. I'll give it a shot. I'll set the reflect solution aside and see if I can get something working with generic maps as you described.

[kylehuntsman]

I pushed a commit that reads and writes maps. I see something is failing regarding the peer config, so I'm going to look into that. I now know I need to rebuild the entire package to run tests against it. I naively thought the tests would just run the existing code. 😓 Had a case of, "it works on my machine" for a sec.

[kylehuntsman]

Okay, so I think I got a little zealous with my changes thinking the duplicated read/writes were unimportant. Apparently duplicating the read/writes in setConfigUnsynced actually does something important when setting a key, causing the output of the entire config file to be different. Haven't quite nailed down what that is yet. I pushed a minimal commit that only alters the merging function. I confirmed locally that the previously failing peer config tests pass, and I got to set a test expecting a failure to expecting a success! 😄

[schomatis]

This is in my list for this week.

[kylehuntsman]

Included error message improvement implementation originally discussed in #8628. Should I squash this into the original commit, or leave it as a separate one?

[schomatis]

We should be comparing the entire maps with require.Equal, that way the comment actually becomes the test, and we don't need specific equality checks.

[schomatis]

This goes in the right direction and we can approve and merge as is if needed, but let me suggest an additional change that is not the responsibility of this PR but addresses old technical debt that would make me very happy.

We're still going back and forth between the JSON maps and the config structs unnecessarily. I still don't understand the use of the undocumented setConfigUnsynced function; it forces SetConfigKey to convert the map to the struct just to be converted back again to merge the maps.

https://github.com/ipfs/go-ipfs/blob/d5ad847e05865e81957c43f526600860c06dbb84/repo/fsrepo/fsrepo.go#L644-L651

It even has a TODO to unroll that logic, which would make it even more apparent that we're doing the back and forth conversion. If we can't replace setConfigUnsynced entirely let's at least refactor it to avoid this.

[kylehuntsman]

@schomatis I can take another stab at fixing that function. I tried originally and ran into some problems, so I backtracked and just implemented the bare minimum with the deep merge. If I spend too much time on it and don't get anywhere, we could make a new issue and merge this as is as suggested.

[schomatis]

You can just unroll the function call there, remove the conversions (basically leaving just your deep merge) and call it a day if it gets too complicated.

[schomatis]

@kylehuntsman Were you able to make progress with this one? If you are busy with other work feel free to re-assign to me to give the finishing touches. You already did the heavy lifting here, thanks.

[kylehuntsman]

@schomatis Yeah, I thought I would have more time last week, but I guess not. 😅 I've assigned back to you. Thanks.

[schomatis]

Continuing in #8793.

[schomatis]

(can't push to this branch)