django-fsm-log

django-fsm-log copied to clipboard

Ran into this issue when attempting to log state changes on a model that uses Postgres uuid4 fields for the primary key.

Thanks for the report @destos. :smiley:

Looking through https://docs.djangoproject.com/en/1.8/ref/contrib/contenttypes/#generic-relations in the section titled "Primary key type compatibility" I found this:

There is no one-size-fits-all solution for which field type is best. You should evaluate the models you expect to be pointing to and determine which solution will be most effective for your use case.

Perhaps the solution is something along the lines of using class inheritance from the StateLog model and overriding the object_id field, then allow passing statelog_model='myapp.FooStateLog' or similar to your FSMField.

Did you want to start a PR?

I'm also affected by this issue. Still I'm not sure how to solve it since I'm using FSM with numeric and non-numeric ids.

I can probably take a look at a fix today per your suggestion.

The CharField is flexible enough to take UUIDs. Primary Keys as UUIDs are very useful in systems using DB replication.

I tend to lean towards a similar proposal, maybe with a swappable Model ? At least we shouldn't force existing users to migrate/change type of object_id field.

So glad this repo is getting some love @ticosax Am really hoping that UUID support for PKs can be added, in whatever form; this is really become the norm these days.

Happy New Year! What is the status of this issue? Do you need additional feedback on it @ticosax? I'm still using a fork but would love to get back on the main repo.

@dbinetti Pull requests are welcome. Unfortunately, I don't have myself the need for this use case, so I don't have economical incitement to do it. Suggestions of a suitable approach has been made by @tysonclugg and me. I will help as much as I can once a PR with decent coverage is made.

Very well. My fork just runs a migration to CharField; I'm afraid creating swappable models is beyond my skills. I'll just pull from upstream. Thanks!

Also affected by this issue(we have UUID's as ids). Will copy-paste model to the project with field type changed. Would be great to just transition to CharField intead, should work seamlessly for anyone.

I tend to lean towards a similar proposal, maybe with a swappable Model ? At least we shouldn't force existing users to migrate/change type of object_id field.

Please, don't do a swappable model. They're literally more hassle than they're worth based on my experience with django-cities. You have to make the decision to use a swapped model from the start of the project or you're totally stuck, unable to migrate to/away from it. Either way it's a migration and if done correctly, should be seamless the way it's been suggested above.

Given that the issue at hand here can be resolved so simply by using a CharField to allow integer and uuid primary keys, the thought of using a swappable model is pure overkill. Also, I keep coming back to this issue every so often to see if anyone has just merged PR #48.

Given that the issue at hand here can be resolved so simply by using a CharField to allow integer and uuid primary keys, the thought of using a swappable model is pure overkill. Also, I keep coming back to this issue every so often to see if anyone has just merged PR #48.

Not so simple when you consider that using a CharField would require all users to migrate.

Also, quoting the Django docs again:

There is no one-size-fits-all solution for which field type is best.

Please, don't do a swappable model.

Thanks for your feedback. What are the alternatives ? given that asking everyone else to migrate to a CharField is a no go in my opinion. The solution shouldn't be intrusive.

How is the migration to a Charfield for the statelog table intrusive? All users will have to migrate anyway if you start using a Swappable field, which in my project has been nothing but pain. I get constantly asked to migrate my project and every time I run makemigrations it creates another migration that does the exact same no-op each time. Packages create new migrations for their users all the time, so I'm unsure where the intrusion is here?

by intrusive I meant changing the type of the DB column for projects that doesn't want/care about. You shared your experience with swappable models and explain it is not a good idea. Fine, how do we move forward ? There is probably other solutions out there ?

A swappable model would be just as intrusive since it also would require a migration with all the attendant issues. So the question isn't swappable v. non-swappable, it's "should we support non-number PKs? " If you do, then it becomes intrusive for all regardless of whether or not people want them. Thus, the underlying issue is: at what point does the degree of demand for non-numeric PKs exceed the intrusion on those who don't want it? To go to the absurd, if 99% of the users don't care about non-numeric PKs, it probably isn't worth it. If conversely 99% want non-numeric PKs, then probably worth it.

The middle ground is what's happening now: people fork the library and do it themselves. So perhaps the number of forks should be your guide?

On Mon, Jul 15, 2019 at 1:38 PM Nicolas Delaby [email protected] wrote:

by intrusive I meant changing the type of the DB column for projects that doesn't want/care about. You shared your experience with swappable models and explain it is not a good idea. Fine, how do we move forward ? There is probably other solutions out there ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gizmag/django-fsm-log/issues/34?email_source=notifications&email_token=AABHPOQTJZNYNMDPI6SXKT3P7TN3VA5CNFSM4CEDIQJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ64WWQ#issuecomment-511560538, or mute the thread https://github.com/notifications/unsubscribe-auth/AABHPOQPX7LNA2CWFYCKCWLP7TN3VANCNFSM4CEDIQJQ .

People who doesn't want to migrate, doesn't fork. We can't measure how many they are. If your suggestion would be merged, I (as an example) would fork the project to keep the FK as an Integer. Either way, we stay divided.

We're divided now and will always be; that isn't going to change. It's about what the priority will be moving forward. So perhaps another guidepost would be if and when the django project moves to a UUID as default PK.

On Mon, Jul 15, 2019 at 2:08 PM Nicolas Delaby [email protected] wrote:

People who doesn't want to migrate, doesn't fork. We can't measure how many they are. If your suggestion would be merged, I (as an example) would fork the project to keep the FK as an Integer. Either way, we stay divided.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gizmag/django-fsm-log/issues/34?email_source=notifications&email_token=AABHPOSSFXTQIJQ6ORHLVHLP7TRMBA5CNFSM4CEDIQJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ67FRI#issuecomment-511570629, or mute the thread https://github.com/notifications/unsubscribe-auth/AABHPOSFDMHFHFQ32VRLFQTP7TRMBANCNFSM4CEDIQJQ .

What nobody has yet been able to explain is how a weak referencing generic foreign key (GFK) link changing type and being supplied with a migration for the field is of any inconvenience at all. Either there is an implementation detail which is non-obvious or you're all forgetting that GFK's don't require you to change the type of the fields they reference, and that they coerce ints to string nicely.

https://stackoverflow.com/a/37472408

On Mon, 15 Jul 2019, 22:18 David Binetti, [email protected] wrote:

We're divided now and will always be; that isn't going to change. It's about what the priority will be moving forward. So perhaps another guidepost would be if and when the django project moves to a UUID as default PK.

On Mon, Jul 15, 2019 at 2:08 PM Nicolas Delaby [email protected] wrote:

People who doesn't want to migrate, doesn't fork. We can't measure how many they are. If your suggestion would be merged, I (as an example) would fork the project to keep the FK as an Integer. Either way, we stay divided.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/gizmag/django-fsm-log/issues/34?email_source=notifications&email_token=AABHPOSSFXTQIJQ6ORHLVHLP7TRMBA5CNFSM4CEDIQJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ67FRI#issuecomment-511570629 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AABHPOSFDMHFHFQ32VRLFQTP7TRMBANCNFSM4CEDIQJQ

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gizmag/django-fsm-log/issues/34?email_source=notifications&email_token=ALEDBXDC5AWQA3Y653AQMETP7TSRDA5CNFSM4CEDIQJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ676BY#issuecomment-511573767, or mute the thread https://github.com/notifications/unsubscribe-auth/ALEDBXEBYE44KHHY3M6BJADP7TSRDANCNFSM4CEDIQJQ .

No one is suggesting that the reference fields need to be changed. @ticosax 's point is that mandating a migration is itself an intrusion. Now we can argue about whether or not that rises to the level of an impassable burden (I personally think not) but it is certainly an imposition on existing users who have no need to support non-numeric PKs.

but it is certainly an imposition on existing users who have no need to support non-numeric PKs.

How. What use case, supported by this library, are we breaking by introducing these changes.

Mandating a migration is itself an imposition. It might be slight, but it is one nonetheless.

But more to your point, if a user were sorting their logs by object_id and depending on those being chronologically sequential then this would break that dependency. Yes, they can re-cast to integer and sort, and yes, they shouldn't be doing that anyway, and yes, yes, yes to whatever your counter-point will be. All that I'm saying is that the impact is non-zero and so the real discussion shouldn't be "there is no imposition" but instead "how much imposition should we tolerate?"

Look, I'm not trying to be unreasonable. I just couldn't see a usecase where someone would genuinely care about typing on a generic foreign key which could be one of any content_types.

What is a major imposition is having to keep remembering to blacklist every model with both UUID pk and a fsm field, update it as we refactor our codebase into model packages and get surprised every now and again when a new developer joins because they don't have the institutional knowledge that fsm log doesn't support UUID and all because of what initially seemed like an unwillingness to ask existing users to run python manage.py migrate and post a depreciation notice/warning.

On Wed, 17 Jul 2019, 00:43 David Binetti, [email protected] wrote:

Mandating a migration is itself an imposition. It might be slight, but it is one nonetheless.

But more to your point, if a user were sorting their logs by object_id and depending on those being chronologically sequential then this would break that dependency. Yes, they can re-cast to integer and sort, and yes, they shouldn't be doing that anyway, and yes, yes, yes to whatever your counter-point will be. All that I'm saying is that the impact is non-zero and so the real discussion shouldn't be "there is no imposition" but instead "how much imposition should we tolerate?"

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gizmag/django-fsm-log/issues/34?email_source=notifications&email_token=ALEDBXEUXEDKS43ES2BKTL3P7ZMI5A5CNFSM4CEDIQJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2CSNFI#issuecomment-512042645, or mute the thread https://github.com/notifications/unsubscribe-auth/ALEDBXECFYVEY7YMMK6FMULP7ZMI5ANCNFSM4CEDIQJQ .

Yep, sounds totally reasonable.

What say ye @ticosax? Are you persuaded by this discussion? I think we can put together a PR if we believed it had a chance of being merged.

At this point, I'm no longer interested into maintaining this library. I'd like to step down from my maintainer and releaser role. It doesn't mean I will stop contributing, but this decision is a burden I'm not willing to take.

Any candidate ?

@jacobh @tysonclugg, please, take it over from there. I don't have permission to invite new maintainers.

no need to be so dramatic, luckily there is other maintainers.

Dramatic?

Dramatic ?

I was referring to my previous comment. Luckily, there is other people who could step in.

My tenure at Gizmag is drawing to a close within the next month, as is the tenure of the Gizmag Django codebase. I'm happy enough to hand this project over to new maintainers, and move to a new organisation. I'll raise a new issue asking for new maintainers.

I've made a PR to cover these changes as #97.