tugberkugurlu / AspNetCore.Identity.MongoDB

MongoDB Data Store Adaptor for ASP.NET Core Identity
MIT License
229 stars 69 forks source link

Id as a generic type parameter #42

Open frmokoena opened 7 years ago

frmokoena commented 7 years ago

I think it's nice to provide Id as a generic type parameter (or at least make it to be changed), and provide the current ObjectId (or even Guid) implementation for convenience.

It's currently kind of limiting for people who would want to generate their own Id's.

tugberkugurlu commented 7 years ago

Thnaks for the suggestioin.

Can you tell me about what you want to use for Id and why?

frmokoena commented 7 years ago

I use Guids but customize by stripping -. I use that across all my domains. Others don't even use Mongo store, so I see no reason not to pass that behavior to my identities.

In any case, I might want to change that behavior for my next project to use other data type, say ints, this way I just feel the library is tying my hands. Just my thoughts.

SorenZ commented 7 years ago

@rm2k when we're using mongodb as a store, ObjectIdplaying important role (in case of replication and sharding) more than simple identifier. And it's more mature than GUID (it's holds embedded timespan in itself). although it's easy to implement, I think it's not a good idea ;) waiting to hear from @tugberkugurlu maybe I'm wrong. :)

frmokoena commented 7 years ago

@SorenZ sorry if i put it to you that ObjectId is bad and Giud is good. :) .That's not what i suggest. All I'm saying is:

  1. The current type of id is a string and private set.
  2. The current implementation is: Id = ObjectId.GenerateNewId().ToString()

And my issue is, the library is being harsh to poor minds like me, who would like to do something like this: Id = Guid.NewGuid().ToString()

Thats why i say why not make it generic like its done in Identity itself and provide the current ObjectId as a default implementation or at least allow it to be changed .

SorenZ commented 7 years ago

@rm2k no no, I didn't think so :) Just consider it, we can not simply do Id = Guid.NewGuid().ToString() (at least now), Id is string in C# but it's ObjectId in mongoDB. we should think about mapping too ;)

frmokoena commented 7 years ago

@SorenZ thanks for the clarification. It seems it's just me against the world.

SorenZ commented 7 years ago

I just thought how to implement it (in best way) and eagerly waiting to hear from @tugberkugurlu ...

tugberkugurlu commented 7 years ago

@rm2k This could be a valid feature request to add, especially for migration cases. However, I still believe MongoDB Id should be the suggested way. So, even if we get this in, we should not make the users choose an id type.

@SorenZ can you elaborate what approach you are thinking about? You can PR it if it's easier than explaining 😄

SorenZ commented 7 years ago

PR is really easier than explaining (for me) :)))

tugberkugurlu commented 7 years ago

@SorenZ cool, looking forward to it!

prafsoni commented 7 years ago

@tugberkugurlu @SorenZ I think the solution to this problem is fairly straightforward as in MongoDB _id field can be of any type i.e. ObjectId (default), String, Int, or a JSON Object. So, to fix this problem what needs to be done is when creating Document from the class the _id should be set to the value of the same type as Id field i.e. Suppose Class have String Id then doing new Document("_id", Id) will store it as a String, similarly if Integer Id , ObjectId Id or CustomId Id. Hence, anyone can store any type of Id they want. So @rm2k can use his custom string Id as _id in MongoDB. I haven't looked at the code yet but I guess you guys know where exactly changes should be made, hope this helps.

tugberkugurlu commented 7 years ago

The current type of id is a string and private set. The current implementation is: Id = ObjectId.GenerateNewId().ToString()

I want to clarify something that even if you work with the id as string, it's stored as ObjectId:

image

This is restrictive but the other approach is to introduce a generic identity user type. Let's see how many 👍 we will get on this and decide based on that.