Skip to content

Fix config.Root calculation #199

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.

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

Merged
merged 7 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 1 addition & 1 deletion src/Hocon.Configuration.Test/DebuggerSpec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void Should_dump_HOCON_with_2_Fallback()
.SafeWithFallback(myHocon2)
.SafeWithFallback(myHocon3);

//fullHocon.GetString("akka.remote.transport").Should().Be("foo");
fullHocon.GetString("akka.remote.transport").Should().Be("foo");

var dump = fullHocon.DumpConfig();
dump.Should().Contain(myHocon1.PrettyPrint(2));
Expand Down
73 changes: 40 additions & 33 deletions src/Hocon.Configuration/Config.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,41 @@ namespace Hocon
/// </summary>
public class Config : HoconRoot
{
[Obsolete("For json serialization/deserialization only", true)]
private Config()
{
}

/// <inheritdoc />
/// <summary>
/// Initializes a new instance of the <see cref="Config" /> class.
/// </summary>
private Config()
private Config(HoconValue value, Config fallback)
{
Fallback = fallback;
Value = value;

Root = GetRootValue();
}

/// <inheritdoc cref="Config()" />
/// <inheritdoc cref="Config(HoconValue, Config)" />
/// <param name="root">The root node to base this configuration.</param>
/// <exception cref="T:System.ArgumentNullException">"The root value cannot be null."</exception>
public Config(HoconRoot root) : base(root?.Value, root?.Substitutions ?? Enumerable.Empty<HoconSubstitution>())
{
Root = GetRootValue();
}

/// <inheritdoc cref="Config()" />
/// <inheritdoc cref="Config(HoconValue, Config)" />
/// <param name="source">The configuration to use as the primary source.</param>
/// <param name="fallback">The configuration to use as a secondary source.</param>
/// <exception cref="ArgumentNullException">The source configuration cannot be null.</exception>
public Config(HoconRoot source, Config fallback) : base(source?.Value,
source?.Substitutions ?? Enumerable.Empty<HoconSubstitution>())
{
Fallback = fallback;

Root = GetRootValue();
}

/// <summary>
Expand All @@ -54,27 +66,12 @@ public Config(HoconRoot source, Config fallback) : base(source?.Value,
/// <summary>
/// The configuration used as a secondary source.
/// </summary>
public Config Fallback { get; private set; }
public Config Fallback { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fallback should never change. It is either set, or not set for given config.

Copy link
Member

Choose a reason for hiding this comment

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

Great idea.


/// <summary>
/// The root node of this configuration section
/// </summary>
public virtual HoconValue Root
{
get
{
var elements = new List<IHoconElement>();
for (var config = this; config != null; config = config.Fallback)
{
elements.AddRange(config.Value);
}

var aggregated = new HoconValue(null);
aggregated.AddRange(elements.AsEnumerable().Reverse());

return aggregated;
}
}
public virtual HoconValue Root { get; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is where we will safe on allocations. Root is immutable and calculated only once, in constructor

Copy link
Member

Choose a reason for hiding this comment

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

👍


/// <summary>
/// Returns string representation of <see cref="Config" />, allowing to include fallback values
Expand All @@ -95,11 +92,7 @@ public string ToString(bool useFallbackValues)
protected Config Copy()
{
//deep clone
return new Config
{
Fallback = Fallback?.Copy(),
Value = (HoconValue) Value.Clone(null)
};
return new Config((HoconValue) Value.Clone(null), Fallback?.Copy());
}

protected override HoconValue GetNode(HoconPath path, bool throwIfNotFound = false)
Expand Down Expand Up @@ -144,14 +137,10 @@ public virtual Config WithFallback(Config fallback)
{
if (fallback == this)
throw new ArgumentException("Config can not have itself as fallback", nameof(fallback));

var clone = Copy();

var current = clone;
while (current.Fallback != null) current = current.Fallback;
current.Fallback = fallback.Copy();

return clone;

// If Fallback is not set - we will set it in new copy
// If Fallback was set - just use it, but with adding new fallback values
return new Config((HoconValue) Value.Clone(null), Fallback?.WithFallback(fallback) ?? fallback);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turned out to be much more elegant solution for getting new config with fallback, and without internal state mutations.

}

/// <summary>
Expand Down Expand Up @@ -199,6 +188,24 @@ public override IEnumerable<KeyValuePair<string, HoconField>> AsEnumerable()
used.Add(kvp.Key);
}
}

/// <summary>
/// Performs aggregation of Value and all Fallbacks into single <see cref="HoconValue"/> object
/// </summary>
private HoconValue GetRootValue()
Copy link
Member

Choose a reason for hiding this comment

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

I'm much happier with this now that it's only run once.

{
var elements = new List<IHoconElement>();

for (var config = this; config != null; config = config.Fallback?.Copy())
{
elements.AddRange(config.Value);
}

var aggregated = new HoconValue(null);
aggregated.AddRange(elements.AsEnumerable().Reverse());

return aggregated;
}
}

/// <summary>
Expand Down