Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#4353 Config.WithFallback is acting inconsistently #4358

Merged
merged 9 commits into from
Mar 25, 2020
Merged

#4353 Config.WithFallback is acting inconsistently #4358

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b7cc0d7
Add bug spec
Arkatufus Mar 25, 2020
52d166e
Missing keys during merge
Arkatufus Mar 25, 2020
c9ac33c
Remove weird workaround that doesn't make any sense
Arkatufus Mar 25, 2020
0d2bf60
How root are calculated neeed to be reversed
Arkatufus Mar 25, 2020
3223e6b
Original node value need to be preserved
Arkatufus Mar 25, 2020
4d5d60a
use Config.Root to standardize Config access
Arkatufus Mar 25, 2020
4695e87
Possible fix for "collection was modified" exception during merge
Arkatufus Mar 25, 2020
e3b1800
Minor optimization
Arkatufus Mar 25, 2020
8ce4683
Merge branch 'dev' into #4353_Akka.Remote_port_config_reverts_to_default
Arkatufus Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/core/Akka/Configuration/Hocon/HoconObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full specification for HOCON can be read here: https://github.com/akkadotnet/hocon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, I see it now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's give it a shot then

modified.Add(new KeyValuePair<string, HoconValue>(otherItem.Key, otherItem.Value));
}
else
{
Expand Down Expand Up @@ -228,6 +230,8 @@ internal HoconObject MergeImmutable(HoconObject other)
mergedValue.AppendValue(mergedObject);
thisItems[otherItem.Key] = mergedValue;
}
else
thisItems[otherItem.Key] = otherItem.Value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for non-HoconObject types, we just ignored them on merge before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it causes a lot of missing object fields, I'm surprised that this bug went through the test suite

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a simple test case we can add for this to the HOCON test suite? Thinking about it mostly as a regression test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else
{
Expand Down