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.

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

Fixing CopyFrom method in OdataMessageReader and OdataMessageWriter to include Arraypool #1595

Merged
merged 12 commits into from
Nov 20, 2019
1 change: 1 addition & 0 deletions src/Microsoft.OData.Core/ODataMessageReaderSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ private void CopyFrom(ODataMessageReaderSettings other)
this.LibraryCompatibility = other.LibraryCompatibility;
this.Version = other.Version;
this.ReadAsStreamFunc = other.ReadAsStreamFunc;
this.ArrayPool = other.ArrayPool;
}
}
}
1 change: 1 addition & 0 deletions src/Microsoft.OData.Core/ODataMessageWriterSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ private void CopyFrom(ODataMessageWriterSettings other)
this.ThrowIfTypeConflictsWithMetadata = other.ThrowIfTypeConflictsWithMetadata;
this.ThrowOnDuplicatePropertyNames = other.ThrowOnDuplicatePropertyNames;
this.ThrowOnUndeclaredPropertyForNonOpenType = other.ThrowOnUndeclaredPropertyForNonOpenType;
this.ArrayPool = other.ArrayPool;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
<Compile Include="..\Evaluation\ODataMetadataContextTests.cs" />
<Compile Include="..\Evaluation\ODataMetadataSelectorTests.cs" />
<Compile Include="..\Evaluation\ODataMissingOperationGeneratorTests.cs" />
<Compile Include="..\Evaluation\TestMetadataSelector.cs" />
<Compile Include="..\Evaluation\TestMetadataSelector.cs" />
<Compile Include="..\Evaluation\TestModel.cs" />
<Compile Include="..\HttpHeaderValueElementTests.cs" />
<Compile Include="..\HttpHeaderValueLexerTests.cs" />
Expand Down Expand Up @@ -281,6 +281,7 @@
<Compile Include="..\ScenarioTests\UriParser\TopAndSkipFunctionalTests.cs" />
<Compile Include="..\ScenarioTests\UriParser\VerificationHelpers.cs" />
<Compile Include="..\ScenarioTests\Writer\JsonLight\WriteFeedWithoutNavigationSourceTests.cs" />
<Compile Include="..\TestCharArrayPool.cs" />
<Compile Include="..\UriParser\Binders\MetadataBindingTests.cs" />
<Compile Include="..\UriParser\Binders\MetadataBindingUtilsTests.cs" />
<Compile Include="..\UriParser\EntitySetNode.cs" />
Expand Down Expand Up @@ -450,6 +451,7 @@
<Compile Include="..\UriParser\Visitors\IsCollectionTranslatorTests.cs" />
<Compile Include="..\UriParser\Visitors\QueryNodeVisitorTests.cs" />
<Compile Include="..\UriParser\Visitors\SyntacticTreeVisitorTests.cs" />
<Compile Include="..\ValidationHelper.cs" />
<Compile Include="..\ValidationUtilsTests.cs" />
<Compile Include="..\WriterUtilsTests.cs" />
<Compile Include="..\ScenarioTests\Streaming\ODataJsonLightStreamReadingTests.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,24 +187,9 @@ private void CompareMessageReaderSettings(ODataMessageReaderSettings expected, O
{
return;
}

Assert.True(expected != null, "expected settings cannot be null");
Assert.True(actual != null, "actual settings cannot be null");
Assert.True(expected.Validations == actual.Validations, "Validations does not match");
Assert.True(Uri.Compare(expected.BaseUri, actual.BaseUri, UriComponents.AbsoluteUri, UriFormat.Unescaped, StringComparison.CurrentCulture) == 0,
"BaseUri does not match");
Assert.True(expected.ClientCustomTypeResolver == actual.ClientCustomTypeResolver, "ClientCustomTypeResolver does not match");
Assert.True(expected.PrimitiveTypeResolver == actual.PrimitiveTypeResolver, "PrimitiveTypeResolver does not match");
Assert.True(expected.EnableMessageStreamDisposal == actual.EnableMessageStreamDisposal, "EnableMessageStreamDisposal does not match");
Assert.True(expected.EnablePrimitiveTypeConversion == actual.EnablePrimitiveTypeConversion, "EnablePrimitiveTypeConversion does not match");
Assert.True(expected.EnableCharactersCheck == actual.EnableCharactersCheck, "CheckCharacters does not match");
Assert.True(expected.ShouldIncludeAnnotation == actual.ShouldIncludeAnnotation, "UseKeyAsSegment does not match");
Assert.True(expected.Validations == actual.Validations, "Validations does not match");
Assert.True(expected.MaxProtocolVersion == actual.MaxProtocolVersion, "MaxProtocolVersion does not match.");
Assert.True(expected.MessageQuotas.MaxPartsPerBatch == actual.MessageQuotas.MaxPartsPerBatch, "MaxPartsPerBatch does not match");
Assert.True(expected.MessageQuotas.MaxOperationsPerChangeset == actual.MessageQuotas.MaxOperationsPerChangeset, "MaxOperationsPerChangeset does not match");
Assert.True(expected.MessageQuotas.MaxNestingDepth == actual.MessageQuotas.MaxNestingDepth, "MaxNestingDepth does not match");
Assert.True(expected.MessageQuotas.MaxReceivedMessageSize == actual.MessageQuotas.MaxReceivedMessageSize, "MaxMessageSize does not match");

var differences = ValidationHelper.GetDifferences<ODataMessageReaderSettings>(expected, actual);
Assert.True(differences.Count == 0, String.Join(",", differences));
Copy link
Member

Choose a reason for hiding this comment

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

If you add the "GetDifferences" function call, how about the codes from Line 191 to Line 207?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to change the existing code. Existing lines will function as such

Copy link
Member

Choose a reason for hiding this comment

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

That's my concern. It seems we verify the same twice.


In reply to: 348177998 [](ancestors = 348177998)

Copy link
Member

Choose a reason for hiding this comment

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

  [](start = 79, length = 6)

remove the unused whitespace

}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,48 @@ public void CopyConstructorShouldCopyODataUri()
Assert.Equal(select, "*");
}
}

[Fact]
public void CopyConstructorShouldCopyAll()
{
this.settings.MetadataSelector = new TestMetadataSelector() { PropertyToOmit = "TestProperty" };
this.settings.SetContentType("application/json,application/atom+xml", "iso-8859-5, unicode-1-1;q=0.8");
this.settings.SetServiceDocumentUri(new Uri("http://example.com"));
this.settings.ArrayPool = new TestCharArrayPool(5);
this.settings.EnableMessageStreamDisposal = false;
this.settings.EnableCharactersCheck = true;

var edmModel = new EdmModel();
var defaultContainer = new EdmEntityContainer("TestModel", "DefaultContainer");
edmModel.AddElement(defaultContainer);
var cityType = new EdmEntityType("TestModel", "City");
var cityIdProperty = cityType.AddStructuralProperty("Id", EdmCoreModel.Instance.GetInt32(/*isNullable*/false));
cityType.AddKeys(cityIdProperty);
cityType.AddStructuralProperty("Name", EdmCoreModel.Instance.GetString(/*isNullable*/false));
cityType.AddStructuralProperty("Size", EdmCoreModel.Instance.GetInt32(/*isNullable*/false));
edmModel.AddElement(cityType);
var citySet = defaultContainer.AddEntitySet("Cities", cityType);

var result = new ODataQueryOptionParser(edmModel, cityType, citySet, new Dictionary<string, string> { { "$expand", "" }, { "$select", "Id,*" } }).ParseSelectAndExpand();

this.settings.ODataUri = new ODataUri()
{
ServiceRoot = new Uri("http://test.org"),
SelectAndExpand = result,
Path = new ODataUriParser(edmModel, new Uri("http://test.org"), new Uri("http://test.org/Cities(1)/Name")).ParsePath()
};

this.settings.LibraryCompatibility = ODataLibraryCompatibility.Version6;
this.settings.Version = ODataVersion.V4;

Func<string, bool> filter = name => true;
this.settings.ShouldIncludeAnnotation = filter;

var newSetting = this.settings.Clone();

var differences = ValidationHelper.GetDifferences<ODataMessageWriterSettings>(this.settings, newSetting);
Assert.True(differences.Count == 0, String.Join(",", differences));
}
#endregion Copy constructor tests

#region Property getters and setters tests
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//---------------------------------------------------------------------
// <copyright file="TestCharArrayPool.cs" company="Microsoft">
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information.
// </copyright>
//---------------------------------------------------------------------

using System;
using Microsoft.OData.Buffers;

Copy link
Member

Choose a reason for hiding this comment

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

copyright

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

namespace Microsoft.OData.Tests
{
public class TestCharArrayPool : ICharArrayPool
Copy link
Member

Choose a reason for hiding this comment

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

I remember I had a default implementation for ICharArrayPool. Don't I?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kept the Testchararraypool as such to be reused later as the the testarraypool implementation was inside another class file.

{
public char[] Buffer { get { return Rent(MinSize); } }

public int MinSize { set; get; }

public TestCharArrayPool(int minSize)
{
this.MinSize = minSize;
}

public char[] Rent(int minSize)
{
return new char[minSize];
}

public void Return(char[] array)
{
throw new NotImplementedException();
}
}
}
106 changes: 106 additions & 0 deletions test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
//---------------------------------------------------------------------
// <copyright file="ValidationHelper.cs" company="Microsoft">
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information.
// </copyright>
//---------------------------------------------------------------------

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
Copy link
Member

Choose a reason for hiding this comment

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

copyright

Copy link
Contributor Author

Choose a reason for hiding this comment

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

REsolved

using System.Reflection;

namespace Microsoft.OData.Tests
{
public static class ValidationHelper
{
/// <summary>
/// This method takes 2 generic types and see if one is the deep copy of other
/// </summary>
/// <typeparam name="T">Method accepts Generic type</typeparam>
/// <param name="obj">Object to be compared with</param>
/// <param name="copy">Copy object to compare</param>
/// <returns>Returns a list of differences</returns>
public static List<string> GetDifferences<T>(T obj, T copy)
{
var diff = new List<string>();
EvaluateDifferences(obj, copy, diff);

return diff;
}


/// <summary>
/// To evaluate the differences of 2 generic types, called recursively
/// </summary>
/// <typeparam name="T">Method accepts Generic type</typeparam>
/// <param name="obj">Object to be compared with</param>
/// <param name="copy">Copy object to compare</param>
/// <param name="diff">List Of Differences</param>
/// <param name="objName">Optional parameter for objectname, to be used in differences string, default empty string</param>
/// <param name="isCollection">Optional parameter to see if its a collection, default false</param>
private static void EvaluateDifferences<T>(T obj, T copy, List<string> diff,string objName ="", bool isCollection =false)
Copy link
Member

Choose a reason for hiding this comment

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

,string objName ="" [](start = 83, length = 19)

add the whitespace correctly. for example:
add whitespace between the parameter, add the whitespace before and after "="?

{
if (obj == null && copy == null)
{
return;
}

string collectionStatement = isCollection?" collection":string.Empty;

if (obj == null || copy == null)
{
diff.Add(string.Format("Value of{0} {1} does not match", collectionStatement, objName));
return;
}

var objType = obj.GetType();

if (typeof(Uri) == objType)
{
if (Uri.Compare(new Uri(obj.ToString()), new Uri(copy.ToString()), UriComponents.AbsoluteUri, UriFormat.Unescaped, StringComparison.CurrentCulture) != 0)
{
diff.Add(string.Format("Value of Uri{0} {1} does not match.", collectionStatement,objName));
}
}
else if (typeof(IComparable).IsAssignableFrom(objType) || objType.IsPrimitive() || objType.IsValueType())
{
if ((!obj.Equals(copy)))
{
diff.Add(string.Format("Value of{0} {1} does not match.",collectionStatement, objName));
}
}
else if (objType.IsArray || typeof(IEnumerable).IsAssignableFrom(objType))
{
var collection1 = ((IEnumerable)obj).Cast<object>();
var collection2 = ((IEnumerable)copy).Cast<object>();

if (collection1.Count() != collection2.Count())
{
diff.Add(string.Format("Collection count of {0} does not match.", objName));
}
else
{
for (int i = 0; i < collection1.Count(); i++)
{
var element1 = collection1.ElementAt(i);
var element2 = collection2.ElementAt(i);

EvaluateDifferences(element1, element2, diff, collection1.GetType().Name, true);
}
}
}
else if ( objType.IsClass())
{
foreach (var prop in obj.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance).Where(x => x.CanRead && x.CanWrite))
{
var val1 = prop.GetValue(obj);
var val2 = prop.GetValue(copy);

EvaluateDifferences(val1, val2, diff,prop.Name);
}
}

}
}
}