search

andrewlock / StronglyTypedId

A Rosyln-powered generator for strongly-typed IDs
MIT License
1.52k stars 80 forks source link

Added MongoSerializer and Mongo ObjectId backing type #62

Open marspion opened 2 years ago

marspion commented 2 years ago

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

andrewlock commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

marspion commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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:

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

marspion commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

marspion commented 2 years ago

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 commented 2 years ago

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 🙂