Added MongoSerializer and Mongo ObjectId backing type #62

marspion · 2022-03-29T14:18:29Z

Hi, pull request is in draft but I would like to know your opinion about my idea for ObjectId support.

andrewlock · 2022-03-29T19:47:54Z

Hey, thanks! In principle, I'm certainly happy to support ObjectId 🙂 I wonder if this is the right way to handle it though 🤔 Would it be better to have ObjectId as a backing type, rather than just a serializer? Given you can't use any old string with objectid, the approach here looks like it could have some problems to me..

marspion · 2022-04-04T11:43:46Z

Hi, I thought about your opinion and you have right. I added ObjectId backing type and modified MongoSerializer to work with all backing types instead of only string and nullable string. Please have a look after changes. If everything will be fine I will implement some unit and integration tests.

andrewlock · 2022-04-05T18:59:17Z

src/StronglyTypedIds/Templates/NewId/NewId_MongoSerializer.cs

+        {
+            public override TESTID Deserialize(MongoDB.Bson.Serialization.BsonDeserializationContext context, MongoDB.Bson.Serialization.BsonDeserializationArgs args)
+            {
+                return new TESTID(MassTransit.NewId.FromGuid(new System.Guid(context.Reader.ReadString())));


I don't think you need to go through Guid here? You could use new NewId(context.Reader.ReadString()) I think 🙂 (similar for Serialize())

You have right :) I simplified it

andrewlock · 2022-04-05T19:00:19Z

Hi, I thought about your opinion and you have right. I added ObjectId backing type and modified MongoSerializer to work with all backing types instead of only string and nullable string. Please have a look after changes. If everything will be fine I will implement some unit and integration tests.

I think that looks much better @marspion, thanks! 🙂

marspion · 2022-05-09T12:22:09Z

Hello @andrewlock, I added some integration tests. Please verify it in your free time. Thanks!

…ode based on review suggestion.

andrewlock

Thanks, this looks really good! I think there's a little inconsistency in the handling of null/"" values in the serializers, but otherwise I think this is pretty good to merge 🙂

andrewlock · 2022-05-09T19:43:16Z

src/StronglyTypedIds/Templates/ObjectId/ObjectId_SystemTextJsonConverter.cs

+            public override TESTID Read(ref System.Text.Json.Utf8JsonReader reader, System.Type typeToConvert, System.Text.Json.JsonSerializerOptions options)
+            {
+                var result = reader.GetString();
+                return string.IsNullOrEmpty(result) ? TESTID.Empty : new TESTID(new MongoDB.Bson.ObjectId(result));


I think there's a discrepancy in the way you're handling null/empty string in these serializers. Some return null, some return TESTID.Empty, and some defer to the default handler

Ups, you have right. I will correct it.

Hi @marspion, so it looks like you've made add the serializers return TESTID.Empty if the object can't be parsed? I'm not sure if that's the best behaviour. I'd expect a serializer to throw if it can't be parsed I think? Otherwise you could get unexpected behaviour?

Hi @andrewlock

Sorry for my mistake but I mixed some cases and I noticed that serializers should not be responsible for it. Definitely it was bad idea to return TESTID.Empty. Now serializers assume that value should be valid otherwise they will throw.

andrewlock · 2022-05-09T19:45:17Z

test/StronglyTypedIds.IntegrationTests/StronglyTypedIds.IntegrationTests.csproj

@@ -19,23 +19,29 @@
  <ItemGroup Condition=" '$(TargetFramework)' == 'net6.0'">
    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="6.0.0-*" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="6.0.0-*" />
+    <PackageReference Include="Mongo2Go" Version="3.1.1" />


TIL about Mongo2Go 🙂

What do you mean by TIL?

Today I Learned 😄

Heh so TIL about TIL 😃

marspion · 2022-05-09T20:04:35Z

@andrewlock Thank you for comment! I will correct code as soon as possible based on your review and also I will look up into errors returned from build pipeline.

marspion · 2022-05-09T21:48:32Z

@andrewlock I am not familiar with snapshots in tests. Do I have to change something before run StronglyTypedIds.Tests or everything should be green?

andrewlock · 2022-05-11T18:07:55Z

Thanks @marspion - snapshot testing is a way of confirming the output is what you expect. It's using this library, Verify. Dan Clarke has a good introduction to it here.

When you run the tests locally, the tests will all fail initially, as there aren't any "verified" files. The process to create them is generally:

Run the tests, generate the "received.txt" files. The tests will file
Take a look at the files-do they look like you would expect.
If you're happy, rename the received files to be "verified.txt" instead
Re-run the tests, they should now pass!

We currently generate every combination of serializer and attribute etc, so there's a lot of snapshots. You don't have to check every one, just make sure they look about right 🙂

P.S. You can also install the DiffEngineTray tool to make your life a little easier!

If you don't want to/can't get this working, I don't mind taking a look and pushing the snapshots for you to review instead

…hot files.

marspion · 2022-05-15T19:45:59Z

Thank you @andrewlock for brief summary about test with snapshots! It is awesome!

As you recommended I used DiffEngineTray and I can agree that it is very helpful tool 🙂

To be honest I wasn't able to verify all snapshot files. I randomly verified some of them (different combinations).

I provided some changes to solve your CodeReview suggestions. Please take a look in your free time.

andrewlock · 2022-05-15T21:42:10Z

To be honest I wasn't able to verify all snapshot files. I randomly verified some of them (different combinations).

Yeah, there's so many combinations now that's about the best we can do 😆 I think it's going to be best to cut them down in a later PR.

It looks like there's a problem running Mongo2Go on Linux 🤔 Will take a look when I get a chance if you don't get to it first!

marspion · 2022-05-16T15:40:27Z

I also will try to investigate what's going on with Mongo2Go on Linux

marspion · 2022-05-20T22:23:53Z

Hi @andrewlock I found some issue on Mongo2Go repository. Probably there is a problem with binaries search pattern on Linux OS (only some versions) but it has not resolved yet. As a workaround with access only to code I was able to override it.

Can you try to run build pipeline? I hope that will solve the problem.

Thanks!

andrewlock · 2022-05-24T21:32:29Z

test/StronglyTypedIds.IntegrationTests/ObjectIdTests.cs

-    [Fact]
-    public void CanSerializeToNullable_WithNewtonsoftJsonProvider()
-    {
-        var entity = new EntityWithNullableId { Id = null };
-
-        var json = NewtonsoftJsonSerializer.SerializeObject(entity);
-        var deserialize = NewtonsoftJsonSerializer.DeserializeObject<EntityWithNullableId>(json);
-
-        Assert.NotNull(deserialize);
-        Assert.Equal(deserialize.Id, NewtonsoftJsonObjectIdId.Empty);
-    }


I think this should still be something we support shouldn't it? 🤔

andrewlock · 2022-05-24T21:35:33Z

Nice work on the Mongo2Go @marspion. I was just looking through the serializer changes, and I'm not entirely sure whether the serializers are completely consistent? Should we handle nulls explicitly or not worry about them? At the moment I'm concerned we might get null references when deserializing "invalid" values. I need to look through properly, but just to want make sure we're handling all the cases that we could hit 🙂

Added MongoObjectIdSerializer for String and NullableString

e90315a

Marcin Rogoz added 2 commits April 4, 2022 11:49

Refactored MongoSerializer and implemented it for all templates

96e24fa

Implemented ObjectId backing type.

88be638

marspion changed the title ~~Added MongoObjectIdSerializer for String and NullableString backingType~~ Added MongoSerializer and Mongo ObjectId backing type Apr 4, 2022

andrewlock reviewed Apr 5, 2022

View reviewed changes

Marcin Rogoz added 2 commits May 6, 2022 23:40

Added some integration tests for ObjectId backing type.

2d2b2e0

Implemented MongoSerializer tests for all backing types

608aae8

Added missing link folder in nuget integration tests and simplified c…

60bb3e6

…ode based on review suggestion.

andrewlock reviewed May 9, 2022

View reviewed changes

Marcin Rogoz added 3 commits May 12, 2022 00:19

Resolved discrepancy of handling null/empty values in serializers.

625a115

Reverted CanSerializeToNullable test.

de0c801

Added missing empty line to ObjectId_Base template and generated snap…

f2d63d1

…hot files.

Remove extra spaces

6fdaeab

CodeReview suggestion about returing TESTID.Empty

a6d0b1b

Overrode Mongo2Go binaries for Linux platform.

6854795

andrewlock reviewed May 24, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added MongoSerializer and Mongo ObjectId backing type #62

Added MongoSerializer and Mongo ObjectId backing type #62

marspion commented Mar 29, 2022

andrewlock commented Mar 29, 2022

marspion commented Apr 4, 2022

andrewlock Apr 5, 2022

marspion May 9, 2022

andrewlock commented Apr 5, 2022

marspion commented May 9, 2022

andrewlock left a comment

andrewlock May 9, 2022

marspion May 9, 2022

andrewlock May 15, 2022

marspion May 16, 2022

andrewlock May 9, 2022

marspion May 9, 2022 •

edited

Loading

andrewlock May 11, 2022

marspion May 11, 2022

marspion commented May 9, 2022

marspion commented May 9, 2022

andrewlock commented May 11, 2022

marspion commented May 15, 2022

andrewlock commented May 15, 2022

marspion commented May 16, 2022

marspion commented May 20, 2022 •

edited

Loading

andrewlock May 24, 2022

andrewlock commented May 24, 2022

Added MongoSerializer and Mongo ObjectId backing type #62

Are you sure you want to change the base?

Added MongoSerializer and Mongo ObjectId backing type #62

Conversation

marspion commented Mar 29, 2022

andrewlock commented Mar 29, 2022

marspion commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewlock commented Apr 5, 2022

marspion commented May 9, 2022

andrewlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marspion May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marspion commented May 9, 2022

marspion commented May 9, 2022

andrewlock commented May 11, 2022

marspion commented May 15, 2022

andrewlock commented May 15, 2022

marspion commented May 16, 2022

marspion commented May 20, 2022 • edited Loading

Choose a reason for hiding this comment

andrewlock commented May 24, 2022

marspion May 9, 2022 •

edited

Loading

marspion commented May 20, 2022 •

edited

Loading