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

Reuse identifiers in UniqueIdentifierHelper #1146

Closed
GreenKn1ght opened this Issue Dec 19, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@GreenKn1ght
Contributor

GreenKn1ght commented Dec 19, 2017

Feature description

Current implementation can lead to overflow problem. I know that int.Max (x2 with negative) is large number, but for 24/7 apps will be better to reuse identifiers of unlinked (dead) instances.
ViewModelBase uses it in ctor so I cant override it by myself.

I think would be nice to have some generation strategies for UniqueIdentifierHelper with ability to register it for specific types.

@GeertvanHorrik GeertvanHorrik added this to the 5.3.0 milestone Dec 19, 2017

@GeertvanHorrik

This comment has been minimized.

Show comment
Hide comment
@GeertvanHorrik

GeertvanHorrik Dec 19, 2017

Member

I am not sure whether re-using is a good idea. What if we migrate to using ulong instead of int?

Member

GeertvanHorrik commented Dec 19, 2017

I am not sure whether re-using is a good idea. What if we migrate to using ulong instead of int?

@GreenKn1ght

This comment has been minimized.

Show comment
Hide comment
@GreenKn1ght

GreenKn1ght Dec 19, 2017

Contributor

For me its looks like semi-solution or even not solution at all. We haven't faced with this problem yet, but due to the fact that the application life time isn't deterministic, it can't be said that the limit won't be reached. And if it happens, we don't even know what kind of behavior to expect.
Since UniqueIdentifierHelper is public, I think its a good idea to improve it.

What are the disadvantages in re-using you see?

Contributor

GreenKn1ght commented Dec 19, 2017

For me its looks like semi-solution or even not solution at all. We haven't faced with this problem yet, but due to the fact that the application life time isn't deterministic, it can't be said that the limit won't be reached. And if it happens, we don't even know what kind of behavior to expect.
Since UniqueIdentifierHelper is public, I think its a good idea to improve it.

What are the disadvantages in re-using you see?

@GeertvanHorrik

This comment has been minimized.

Show comment
Hide comment
@GeertvanHorrik

GeertvanHorrik Dec 19, 2017

Member

The idea is to create an ObjectIdGenerator (implementing IObjectIdGenerator) class which will solve #1141 as well. Then we could make it injectable / replaceable.

Re-using will result in the same objects reusing the same unique ids, making them no longer unique. This could be very confusing when going through the logs, etc.

Member

GeertvanHorrik commented Dec 19, 2017

The idea is to create an ObjectIdGenerator (implementing IObjectIdGenerator) class which will solve #1141 as well. Then we could make it injectable / replaceable.

Re-using will result in the same objects reusing the same unique ids, making them no longer unique. This could be very confusing when going through the logs, etc.

@GreenKn1ght

This comment has been minimized.

Show comment
Hide comment
@GreenKn1ght

GreenKn1ght Dec 19, 2017

Contributor

Miss click.

Contributor

GreenKn1ght commented Dec 19, 2017

Miss click.

@GreenKn1ght GreenKn1ght reopened this Dec 19, 2017

@GreenKn1ght

This comment has been minimized.

Show comment
Hide comment
@GreenKn1ght

GreenKn1ght Dec 19, 2017

Contributor

May be I just making a mountain out of a molehill. Probably ulong "long" enough.

Contributor

GreenKn1ght commented Dec 19, 2017

May be I just making a mountain out of a molehill. Probably ulong "long" enough.

@GeertvanHorrik

This comment has been minimized.

Show comment
Hide comment
@GeertvanHorrik

GeertvanHorrik Dec 20, 2017

Member

Guids will be crap to read, making it much harder to find easy unique identifiers. But again, what if we turn the unique identifier into a string and the user can choose whether to return int, long or guid?

Member

GeertvanHorrik commented Dec 20, 2017

Guids will be crap to read, making it much harder to find easy unique identifiers. But again, what if we turn the unique identifier into a string and the user can choose whether to return int, long or guid?

@GreenKn1ght

This comment has been minimized.

Show comment
Hide comment
@GreenKn1ght

GreenKn1ght Dec 20, 2017

Contributor

It make sense.

Contributor

GreenKn1ght commented Dec 20, 2017

It make sense.

@GeertvanHorrik

This comment has been minimized.

Show comment
Hide comment
@GeertvanHorrik

GeertvanHorrik Dec 20, 2017

Member

For this to be a non-breaking change, we'll need to introduce a new class name. My suggestion is ObjectIdGenerator (I believe .NET also used this name for once of their own classes, so it makes sense). The only thing I am not sure about yet is whether we should make it static (easy to use, but hard to customize for end-users) or instance (are we going to resolve by interface?).

Member

GeertvanHorrik commented Dec 20, 2017

For this to be a non-breaking change, we'll need to introduce a new class name. My suggestion is ObjectIdGenerator (I believe .NET also used this name for once of their own classes, so it makes sense). The only thing I am not sure about yet is whether we should make it static (easy to use, but hard to customize for end-users) or instance (are we going to resolve by interface?).

@GreenKn1ght

This comment has been minimized.

Show comment
Hide comment
@GreenKn1ght

GreenKn1ght Dec 20, 2017

Contributor

If UniqueIdentifierHelper was an instance with interface, I would not even create this issue :)

Contributor

GreenKn1ght commented Dec 20, 2017

If UniqueIdentifierHelper was an instance with interface, I would not even create this issue :)

@GreenKn1ght

This comment has been minimized.

Show comment
Hide comment
@GreenKn1ght

GreenKn1ght Dec 20, 2017

Contributor

But I'm not sure that registering the implementation of this interface at the same time for the Catel-types and user-types is a good solution. Perhaps we should somehow split them.

Contributor

GreenKn1ght commented Dec 20, 2017

But I'm not sure that registering the implementation of this interface at the same time for the Catel-types and user-types is a good solution. Perhaps we should somehow split them.

@GeertvanHorrik

This comment has been minimized.

Show comment
Hide comment
@GeertvanHorrik

GeertvanHorrik Jan 19, 2018

Member

We are planning to implement this for 5.3, and we hope to release that before the end of the month. We will try to look into this ticket soon.

Member

GeertvanHorrik commented Jan 19, 2018

We are planning to implement this for 5.3, and we hope to release that before the end of the month. We will try to look into this ticket soon.

alexfdezsauco added a commit that referenced this issue Jan 22, 2018

(+) #1146 Add IObjectIdGenerator interfaces in order to customize the…
… approach to generate id for objects with options to reuse identifiers

(x) Fix ViewModelBase constructor implementation in order to use IntegerObjectIdGenerator by default

GeertvanHorrik added a commit that referenced this issue Jan 25, 2018

Merge pull request #1151 from Catel/feature/1146
#1146 Add IObjectIdGenerator interfaces in order to customize the approach to generate id for objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment