brendon / ranked-model

An acts_as_sortable/acts_as_list replacement built for Rails 4+
https://github.com/mixonic/ranked-model
MIT License
1.09k stars 134 forks source link

NonNilColumnDefault breaks running app when rank column has a default value #180

Closed vanboom closed 3 months ago

vanboom commented 2 years ago

The logic for raising the NonNilColumnDefault breaks a running application from loading when the ranked column has a default value. The exception does not even allow rake to run.

Suggestion: change to something non-destructive to raise awareness of this dependency without breaking the application.

brendon commented 2 years ago

Hi @vanboom, it's intended to be annoying. It's not supported and can cause subtle problems so I think it's best that it fatally fails. I'd assume you'd only find yourself in this position upon upgrading ranked-model or commencing development. In both cases I think it'd be important to know of this problem early on and rectify it.

What was your situation? :)

vanboom commented 2 years ago

In our case, we attempted to add ranked-model to an existing app that already had a position column with a default value.

With the gem installed, the NonNilColumnDefault exception causes us to not be able to run rake to migrate the required database change. In development this is not a big deal, but it forces us to release the production code in two steps:

  1. Migration to remove the default value (potentially breaking existing code)
  2. Merge and release the changes with ranked-model implemented.

We are not able to release to production in this way because removing the default value from the database column would break existing code, and because of the NonNilColumnDefault exception, we are not able to release the change incorporating ranked-model in one release.

brendon commented 2 years ago

Ah I see, yes that would be annoying. Is there a way to prevent a raise in migrations and only have it happen on normal app server boot, perhaps even only in development?

I really want people to see this error as before this we used to get quite a few tickets about strange behaviour, and it wasn't obvious the problem was a column default.

Thanks for persevering with me :)

brendon commented 2 years ago

Another idea is to allow the error raise to be silenced by an initialiser?

vanboom commented 2 years ago

I agree that the error is fairly significant, but do not think that the exception should stop the execution of the app. Possibly print the error as a warning when the rank field is accessed so the developer will see a lot of runtime warnings to get their attention?

brendon commented 2 years ago

Thanks @vanboom. I'd only be ok with that solution if it didn't adversely affect the majority of people using the gem. If there's a way to cache the check when the app boots and then check that cache whenever a ranked-model operation occurs, and that checking doesn't measurably slow things down then that could work.

I just had another thought. In rails when there are migrations to run it throws an error telling the user to run the migrations. The migrations are still able to run. We could look to copy how this works for our purposes?

brendon commented 3 months ago

Closing this for now. See my comment in the PR.