Skip to content
This repository was archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5ee47a
[REEF-2044] Serializing a List in Tang for C# has missing functionality
singlis Sep 12, 2018
0aaf08a
Merge remote-tracking branch 'origin/master' into REEF-2044
singlis Oct 16, 2018
354a880
Merge remote-tracking branch 'origin/master' into REEF-2044
singlis Oct 16, 2018
f847c75
- Added more unit tests, plus checks for null/empty strings in lists
singlis Oct 18, 2018
11b134d
- Updating to tests to handle the binding exception for null and emp…
singlis Oct 19, 2018
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
174 changes: 171 additions & 3 deletions lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
// specific language governing permissions and limitations
// under the License.

using System;
using System.Collections.Generic;
using Org.Apache.REEF.Common.Tasks;
using Org.Apache.REEF.Examples.Tasks.HelloTask;
using Org.Apache.REEF.Tang.Annotations;
Expand All @@ -31,6 +29,9 @@
using Org.Apache.REEF.Tang.Protobuf;
using Org.Apache.REEF.Tang.Tests.ScenarioTest;
using Org.Apache.REEF.Tang.Util;
using System;
using System.Collections.Generic;
using System.Linq;
using Xunit;

namespace Org.Apache.REEF.Tang.Tests.Configuration
Expand Down Expand Up @@ -425,6 +426,144 @@ public void TestTimerConfigurationWithClassHierarchy()
timer.sleep();
}

[Fact]
public void TestListConfig()
{
IList<string> stringList1 = new List<string>();
stringList1.Add("foo");
stringList1.Add("bar");
Copy link
Contributor

@motus motus Jan 5, 2019

Choose a reason for hiding this comment

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

Here and everywhere: can be simply

var stringList1 = new List<string> { "foo", "bar" };


IList<string> stringList2 = new List<string>();
stringList2.Add("test1");
stringList2.Add("test2");
stringList2.Add("test3");
stringList2.Add("test4");

IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList1)
.BindList<ListOfStrings2, string>(GenericType<ListOfStrings2>.Class, stringList2)
.Build();
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

No need for the first argument in BindList(): that information is already passed as a generic parameter. can simply write

IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
    .BindList<ListOfStrings, string>(stringList1)
    .BindList<ListOfStrings2, string>(stringList2)
    .Build();

(also, maybe define static ITang Tang = TangFactory.GetTang(); that can be used in all tests?)


string s = ConfigurationFile.ToConfigurationString(conf);
IConfiguration conf2 = ConfigurationFile.GetConfiguration(s);

ListInjectTest injectTest = (ListInjectTest)TangFactory.GetTang().
NewInjector(conf2).GetInstance(typeof(ListInjectTest));
Copy link
Contributor

@motus motus Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No need to cast - simply write

var injectTest = TangFactory.GetTang().NewInjector(conf2).GetInstance<ListInjectTest>();


Assert.Equal(stringList1, injectTest.List1);
Assert.Equal(stringList2, injectTest.List2);
}

[Fact]
public void TestListConfigWithEmptyList()
{
IList<string> stringList1 = new List<string>();

IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList1)
.Build();
string s = ConfigurationFile.ToConfigurationString(conf);
// string will be empty since there is nothing to save
Assert.Equal(0, s.Length);
}

[Fact]
public void TestListConfigWithEmptyString()
{
IList<string> stringList1 = new List<string>();
stringList1.Add("");
Copy link
Contributor

@jwang98052 jwang98052 Oct 19, 2018

Choose a reason for hiding this comment

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

check the behavior of set for empty string to make the behavior consistent.


try
{
IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList1)
.Build();
}
catch(BindException)
{
return;
}

Assert.True(false, "Failed to throw expected exception.");
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

same as above: the entire test can be written as

Assert.Throws<BindException>(() =>
    Tang.NewConfigurationBuilder()
        .BindList<ListOfStrings, string>(new List<string>() { "" })
        .Build());


}

[Fact]
public void TestListConfigWithNullStringValue()
{
IList<string> stringList1 = new List<string>();
stringList1.Add(null);
Copy link
Contributor

@jwang98052 jwang98052 Oct 19, 2018

Choose a reason for hiding this comment

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

Same for null


try
{
IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList1)
.Build();
}
catch(BindException)
{
return;
}

Assert.True(false, "Failed to throw expected exception.");
Copy link
Contributor

@motus motus Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Here and everywhere: the whole test can be just

Assert.Throws<BindException>(() =>
    TangFactory.GetTang().NewConfigurationBuilder()
        .BindList<ListOfStrings, string>(new List<string>() { null })
        .Build());

}

private void TestSerializeListHelper(IList<string> items, int expectedLists = 1)
{
IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, items)
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

here and everywhere: no need to pass GenericType<ListOfStrings>.Class explicitly

.Build();

var serializer = new AvroConfigurationSerializer();
byte[] bytes = serializer.ToByteArray(conf);
IConfiguration conf2 = serializer.FromByteArray(bytes);
Assert.Equal(expectedLists, conf2.GetBoundList().Count);
if (expectedLists > 1)
{
var actualList = conf2.GetBoundList().First().Value;
Assert.Equal(items, actualList);
}
}

[Fact]
public void TestListSerialize()
{
IList<string> stringList = new List<string>();
stringList.Add("foo");
stringList.Add("bar");
TestSerializeListHelper(stringList);
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

just

TestSerializeListHelper(new List<string> { "foo", "bar" });

}

[Fact]
public void TestListSerializeNullStringValue()
{
string msg = null;
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

msg is not used anywhere

IList<string> stringList = new List<string>();
stringList.Add(null);

var builder = TangFactory.GetTang().NewConfigurationBuilder();
Assert.Throws<BindException>(() => builder.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList));
}

[Fact]
public void TestListSerializeEmptyList()
{
IList<string> stringList = new List<string>();
TestSerializeListHelper(stringList, 0);
}

[Fact]
public void TestListSerializeEmptyStrings()
{
string msg = null;
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

unused msg variable

IList<string> stringList = new List<string>();
stringList.Add("");
stringList.Add("");

var builder = TangFactory.GetTang().NewConfigurationBuilder();
Assert.Throws<BindException>(() => builder.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList));
Copy link
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

just

Assert.Throws<BindException>(() => builder.BindList<ListOfStrings, string>(new List<string> { "", "" }));

}

[Fact]
public void TestSetConfig()
{
Expand All @@ -448,6 +587,7 @@ public void TestSetConfig()
Assert.True(actual.Contains("six"));
}


Copy link
Contributor

@motus motus Jan 5, 2019

Choose a reason for hiding this comment

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

why an extra empty line?

[Fact]
public void TestSetConfigWithAvroSerialization()
{
Expand All @@ -471,6 +611,7 @@ public void TestSetConfigWithAvroSerialization()
Assert.True(actual.Contains("six"));
}


[Fact]
public void TestNullStringValue()
{
Expand Down Expand Up @@ -508,11 +649,38 @@ public void TestSetConfigNullValue()
}
}

[NamedParameter]
class ListOfStrings : Name<IList<string>>
{

}

[NamedParameter]
class ListOfStrings2 : Name<IList<string>>
{

}

class ListInjectTest
{
public IList<string> List1;
public IList<string> List2;

[Inject]
ListInjectTest([Parameter(typeof(ListOfStrings))] IList<string> list1,
[Parameter(typeof(ListOfStrings2))] IList<string> list2)
{
this.List1 = list1;
this.List2 = list2;
}
}

[NamedParameter(DefaultValues = new string[] { "one", "two", "three" })]
class SetOfNumbers : Name<ISet<string>>
{
}


class Box
{
public ISet<string> Numbers;
Expand Down Expand Up @@ -560,4 +728,4 @@ public string GetString()
return str;
}
}
}
}
36 changes: 28 additions & 8 deletions lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,6 @@
// specific language governing permissions and limitations
// under the License.

using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.Serialization;
using System.Text;
using Microsoft.Hadoop.Avro;
using Microsoft.Hadoop.Avro.Container;
using Newtonsoft.Json;
Expand All @@ -34,6 +27,13 @@
using Org.Apache.REEF.Tang.Types;
using Org.Apache.REEF.Tang.Util;
using Org.Apache.REEF.Utilities.Logging;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.Serialization;
using System.Text;

namespace Org.Apache.REEF.Tang.Formats
{
Expand Down Expand Up @@ -254,12 +254,32 @@ public AvroConfiguration ToAvroConfiguration(IConfiguration c)
}
else
{
Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);
throw new TangApplicationException("Unable to serialize set of type {e.Value.GetType()}");
Copy link
Contributor

@motus motus Jan 5, 2019

Choose a reason for hiding this comment

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

Forgot to use $"" for interpolation.

}

l.Add(new ConfigurationEntry(e.Key.GetFullName(), val));
}

foreach (var kvp in conf.GetBoundList())
{
foreach (var item in kvp.Value)
{
string val = null;
if (item is string)
{
val = (string)item;
}
else if (item is INode)
{
val = ((INode)item).GetFullName();
}
else
{
throw new TangApplicationException("Unable to serialize list of type {item.GetType()}");
Copy link
Contributor

@motus motus Jan 5, 2019

Choose a reason for hiding this comment

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

same as above - use $"". please check for other places as well

}
l.Add(new ConfigurationEntry(kvp.Key.GetFullName(), val));
}
}
Copy link
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

Also, maybe the whole loop can be expressed in Linq terms? e.g. along the lines of

l.UnionWith(conf.GetBoundList().SelectMany(kvp =>
{
    string key = kvp.Key.GetFullName();
    return kvp.Value.Select(item =>
    {
        if (item is string val) return new ConfigurationEntry(key, val);
        if (item is INode node) return new ConfigurationEntry(key, node.GetFullName());
        throw new TangApplicationException($"Unable to serialize list of type {item.GetType()}");
    });
}));

return new AvroConfiguration(Language.Cs.ToString(), l);
}

Expand Down
35 changes: 28 additions & 7 deletions lang/cs/Org.Apache.REEF.Tang/Formats/ConfigurationFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@
// specific language governing permissions and limitations
// under the License.

using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using Org.Apache.REEF.Tang.Exceptions;
using Org.Apache.REEF.Tang.Implementations.Configuration;
using Org.Apache.REEF.Tang.Implementations.Tang;
using Org.Apache.REEF.Tang.Interface;
using Org.Apache.REEF.Tang.Types;
using Org.Apache.REEF.Tang.Util;
using Org.Apache.REEF.Utilities.Logging;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;

namespace Org.Apache.REEF.Tang.Formats
{
Expand Down Expand Up @@ -133,12 +133,33 @@ public static HashSet<string> ToConfigurationStringList(IConfiguration c)
}
else
{
Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);
throw new BindException($"Failed to serialize set of unsupported type {e.Value.GetType()}");
}

l.Add(GetFullName(e.Key) + '=' + Escape(val));
}

foreach(var kvp in conf.GetBoundList())
{
foreach (var item in kvp.Value)
{
string val = null;
if (item is string)
{
val = GetFullName((string)item);
}
else if (kvp.Value is INode)
Copy link
Contributor

@jwang98052 jwang98052 Oct 19, 2018

Choose a reason for hiding this comment

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

You can try a test case for List of instances from different subclasses of the same interface, similar to Set to see if this scenario is working.

Copy link
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

shouldn't it be item is INode instead of kvp.Value is INode? also, can use if (item is INode node) ... syntax to avoid the cast

{
val = GetFullName((INode)kvp.Value);
}
else
{
throw new BindException($"Failed to serialize list of unsupported type {item.GetType()}");
}
l.Add(GetFullName(kvp.Key) + '=' + Escape(val));
}
}

return l;
}

Expand Down
20 changes: 20 additions & 0 deletions lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationBuilderImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,10 @@ public void BindParameter(INamedParameterNode name, string value)
{
BindSetEntry((INamedParameterNode)name, value);
}
else if (name.IsList())
{
BindList((INamedParameterNode)name, value);
}
else
{
try
Expand All @@ -289,6 +293,17 @@ public void BindParameter(INamedParameterNode name, string value)
}
}

public void BindList(INamedParameterNode iface, string impl)
{
IList<object> l;
if (!BoundLists.TryGetValue(iface, out l))
Copy link
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

can join these two lines, i.e.

if (!BoundLists.TryGetValue(iface, out IList<object> l))

etc.

{
l = new List<object>();
BoundLists.Add(iface, l);
}
l.Add((object)impl);
Copy link
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

no need to cast

}

public void BindImplementation(IClassNode n, IClassNode m)
{
if (this.ClassHierarchy.IsImplementation(n, m))
Expand Down Expand Up @@ -337,6 +352,11 @@ public void BindList(INamedParameterNode iface, IList<string> impl)
IList<object> l = new List<object>();
foreach (var n in impl)
{
if (string.IsNullOrEmpty(n))
{
throw new ArgumentException("List cannot contain string that are null or empty");
}

l.Add((object)n);
Copy link
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

cast is redundant

Copy link
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

Also, maybe we can write something like

if (impl.Any(string.IsNullOrEmpty))
{
    throw new ArgumentException("List cannot contain string that are null or empty");
}
BoundLists.Add(iface, impl.ToList<object>());

}
BoundLists.Add(iface, l);
Expand Down
Loading