search

gocardless / statesman

A statesmanlike state machine library.
https://gocardless.com/blog/statesman/
MIT License
1.78k stars 163 forks source link

Failure: calling update_most_recents without most_recent_id #408

Closed ccvivien closed 4 years ago

ccvivien commented 4 years ago

I'm moving my rails application from mysql to postgres, and everywhere I make a call to transition_to, it fails with the following error:

 TypeError:
       no implicit conversion of nil into String

the error occurs in statesman-7.2.0/lib/statesman/adapters/active_record.rb

def build_most_recents_update_all_values(most_recent_id = nil) on the line Arel::Nodes::SqlLiteral.new(most_recent_value(most_recent_id))

It seems to come from the version 7.2. I don't encounter such error with version 7.1

In the method def create_transition(from, to, metadata), when not using mysql, it tries to call update_most_recents with no parameters, so that's why it fails building the arel sql literal.

Any idea of why this is happening?

thom-oman commented 4 years ago

Hey @ccvivien sorry for the slow reply, not got round to looking at this properly today. Sounds like you may be in a weird state between PG and mysql. Will try dedicate sometime tomorrow to look at this properly but in the meantime could you tell me what Statesman.mysql_gaplock_protection? returns please?

ccvivien commented 4 years ago

Hey @thom-oman !, no worries.

Statesman.mysql_gaplock_protection? returns nil

Normaly, I should have no trace left of mysql, I even removed mysql2 gem from the Gemfile, and in config/database.yml there is no trace left aswell of the former mysql database.

PS: It should be one of my teammate who replies next as I'm absent for a week.

thom-oman commented 4 years ago

Hey @ccvivien sorry for not getting back to you, something came up on Friday that prevented me from looking into this.

Don't have all the context myself but I've taken a quick look at this this morning and my theory is that the error is due to the default value of the most_recent column not being false. When we generate the migrations here we set this default value to false based on the logic in this method. Which since you've only recently changed to postgres would've resulted in most_recent not having a default.

I will try to confirm this my side later in the day, could you (or your teammate) see if changing this null value locally solves your problem? If this is the case, then we will have to add some documentation on transitioning from MySQL to Postgres to the README.

thom-oman commented 4 years ago

Managed to reproduce this issue by removing the null value for the most_recent column. Will add a short paragraph in the readme for those migrating from mysql to postgres.

Did this fix it your issue?

thom-oman commented 4 years ago

I confused myself on the above. By default value I should've said nullable. Also with second look I realised this was a code issue. I've made a quick PR (https://github.com/gocardless/statesman/pull/409), which should hopefully solve the issue. Will try to get reviewed by some colleagues once I can get tests to pass.

AlSquire commented 4 years ago

Hi @thom-oman, I'm a teammate of @ccvivien

I've just tried your PR and in its current state it seems to solve our issue :)

thom-oman commented 4 years ago

Hey @AlSquire that's great new! Sorry again for the delays, will release a new version today. Also wll try to clean up that file in a follow up PR, it's getting quite messy haha. Will close this issue, lemme know if you have any more issues :)