#4353 Config.WithFallback is acting inconsistently #4358
Conversation
|
@Arkatufus what exactly was the issue and how does this fix it? Asking because I'm in the dark about this |
|
there was some inconsistency in accessing Config and keys missing during fallback merge due to keys not being copied to the new config object |
|
interesting :) |
| @@ -196,6 +196,8 @@ public void Merge(HoconObject other) | |||
| //if both values are objects, merge them | |||
| if (thisItem.IsObject() && otherItem.Value.IsObject()) | |||
| thisItem.GetObject().Merge(otherItem.Value.GetObject()); | |||
| else | |||
There was a problem hiding this comment.
ok, so if I'm reading this right, if both items are objects, they are to be merged, but how is the alternative supposed to work? what is otherItem's type?
There was a problem hiding this comment.
HOCON have 3 basic datatypes, Object, Array, and Literal.
Array and Literal will always override merged object fields, but when an Object met another Object, they got merged instead.
There was a problem hiding this comment.
The full specification for HOCON can be read here: https://github.com/akkadotnet/hocon
There was a problem hiding this comment.
let's give it a shot then
|
@mmisztal1980 I'll go over this and if it's in good shape, merge it and kick it off in a nightly build. You keen to give it a try once that goes through? |
| @@ -228,6 +241,8 @@ internal HoconObject MergeImmutable(HoconObject other) | |||
| mergedValue.AppendValue(mergedObject); | |||
| thisItems[otherItem.Key] = mergedValue; | |||
| } | |||
| else | |||
| thisItems[otherItem.Key] = otherItem.Value; | |||
There was a problem hiding this comment.
So for non-HoconObject types, we just ignored them on merge before?
There was a problem hiding this comment.
yeah, it causes a lot of missing object fields, I'm surprised that this bug went through the test suite
There was a problem hiding this comment.
Is there a simple test case we can add for this to the HOCON test suite? Thinking about it mostly as a regression test.
There was a problem hiding this comment.
We do have rigorous testing suite in the standalone HOCON project, I think we used to have a limited HOCON test suite in Akka, they might be accidentally removed.
ofcourse |
…kkadotnet#4358)" This reverts commit 3788ced.
Arkatufus
deleted the
#4353_Akka.Remote_port_config_reverts_to_default
branch
Possible fix for #4353, there might be other factors involved.