-
Notifications
You must be signed in to change notification settings - Fork 494
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
SpatialPoint Serialization/Deserialization: Fixes spatial point serialization/deserialization bug #4801
base: master
Are you sure you want to change the base?
Conversation
…lization/deserilization bug
…lization/deserilization bug
return; | ||
} | ||
|
||
writer.WriteStartObject("boundingBox"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Add the strings as const at the class level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other converters too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still making changes to the codebase. Please hold off on the review.
{ | ||
if (property.NameEquals("min")) | ||
{ | ||
min = System.Text.Json.JsonSerializer.Deserialize<Position>(property.Value.ToString(), options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its materializing the value to string and Deserialize<> will parse string and convert to JsonElement again.
Number of points might be higher in a single response increasing the overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still making changes to the codebase. Please hold off on the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code changes done. Please review.
{ | ||
throw new JsonException(RMResources.JsonUnexpectedToken); | ||
} | ||
if (reader.TokenType == JsonTokenType.Null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is null check needed front to cover for null assignment scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still making changes to the codebase. Please hold off on the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code changes done. Please review.
using System.Collections.Generic; | ||
using System.Text; | ||
|
||
internal class GeometryParamsHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this type used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still making changes to the codebase. Please hold off on the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code changes done. Please review.
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/Spatial/PointTest.cs
Outdated
Show resolved
Hide resolved
@@ -25,7 +25,7 @@ public class PointTest | |||
[TestMethod] | |||
public void TestPointSerialization() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these tests needs to run for both STJ and newtonsoft.
Simple dirty way to achieve it might be
public static IEnumerable<object[]> Serializers
{
get
{
yield return new object[] { new CosmosJsonDotNetSerializer() };
yield return new object[] { new CosmosSystemTextJsonSerializer(new JsonSerializerOptions()) };
}
}
/// <summary>
/// Tests serialization/deserialization.
/// </summary>
[TestMethod]
[DynamicData(nameof(Serializers))]
public void TestPointSerialization(CosmosSerializer serializer)
var point = serializer.FromStream<Point>(new MemoryStream(Encoding.UTF8.GetBytes(json)));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be covered for all spatial tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still making changes to the codebase. Please hold off on the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code changes done. Please review.
If you are still making the changes, you can convert this PR to "draft" mode. |
@dibahlfi please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Pull Request Template
Description
This change is to fix serialization/deserialization bug in the Point class. Please see(#4744) for more information.
The crux of the problem is the use of both .NET native(System.Text.Json) and Newtonsoft serialization libraries in the codebase. The core class that does the serialization of Point is CosmosSystemTextJsonSerializer which uses .NET native library but the Point/Position classes are built leveraging Newtonsoft library so attributes like [JsonConverter(typeof(PositionJsonConverter))] in the Position class are not kicking in to do their job as they are Newtonsoft contructs and don't get invoked when serialization happens using .NET native(System.Text.Json) library.
To fix the issue I had to relax the visibility levels of constructors and a readOnly collection. An alternative could be to create a new subclass of CosmosLinqSerializer that uses Newtonsoft but that is more involved and could have broader implications.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #4744