ranked-model icon indicating copy to clipboard operation
ranked-model copied to clipboard

NonNilColumnDefault breaks running app when rank column has a default value

Open vanboom opened this issue 4 years ago • 6 comments

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.

vanboom avatar Oct 10 '21 00:10 vanboom

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? :)

brendon avatar Oct 10 '21 00:10 brendon

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.

vanboom avatar Oct 10 '21 20:10 vanboom

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 avatar Oct 12 '21 03:10 brendon

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

brendon avatar Oct 12 '21 03:10 brendon

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?

vanboom avatar Oct 17 '21 21:10 vanboom

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 avatar Oct 19 '21 20:10 brendon

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

brendon avatar Jun 04 '24 04:06 brendon