Introduction of `updated_timestamp_column` throws an exception when using the Postgres JSON column type for metadata

[mbaird]

With v3.4.0, the addition of https://github.com/gocardless/statesman/pull/306 makes transitions dependent on Statesman::Adapters::ActiveRecordTransition, so the advice given in the README when using a PostgreSQL JSON column type to remove the include causes the following exception:

NoMethodError:
       undefined method `updated_timestamp_column' for #<Class:0x00007fdd2ade07b0>

a workaround is obviously to now to explicitly set the updated_at in the transition class, which isn't ideal.

class_attribute :updated_timestamp_column
self.updated_timestamp_column = :updated_at

[timrogers]

Thanks for reporting this - it’s a shame we didn’t have specs for this! Do you know why we advise to remove the include in that case? (I’m out and about right now so I can’t check for myself.) On Tue, 13 Feb 2018 at 15:22, Michael Baird notifications@github.com wrote:

With v3.4.0, the addition of #306 https://github.com/gocardless/statesman/pull/306 makes transitions dependent on Statesman::Adapters::ActiveRecordTransition, so the advice given in the README when using a PostgreSQL JSON column type https://github.com/gocardless/statesman#using-postgresql-json-column to remove the include causes the following exception:

NoMethodError: undefined method `updated_timestamp_column' for #

a workaround is obviously to now to explicitly set the updated_at in the transition class, which isn't ideal.

class_attribute :updated_timestamp_columnself.updated_timestamp_column = :updated_at

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gocardless/statesman/issues/309, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHFplyZLJ2t-fRo4vAU84m-cQomZbnlks5tUai5gaJpZM4SD6zR .

[dmagliola]

@timrogers The Statesman::Adapters::ActiveRecordTransition include basically just did this in the past: base.send(:serialize, :metadata, JSON) Which (I presume, but i'm not 100% confident) would not be necessary if the field is of json type.

However, now it also defines updated_timestamp_column and sets it, by default, to :updated_at: https://github.com/gocardless/statesman/pull/306/files#diff-10e9e39475eb4deba3d30cc473bb8d30

So now we depend on its inclusion, @mbaird is correct.

I'm assuming calling serialize even when not necessary is harmless (but we should test it to make sure!), in which case we could just remove the suggestion from the README, but this is now a breaking change and everyone would have to retroactively include it in their transition classes. I haven't thought about this problem that much, so not sure if there's a better place to define updated_timestamp_column.

Can we use defined? to check for the existence of updated_timestamp_column, and defaulting it to updated_at if it's not defined, instead of defining it for all Transition classes? I know defined? is somewhat frowned upon, but it feels cleaner than breaking backwards compatibility.

[timrogers]

We could do something like that. I think it'd be better though to have everyone include ActiveRecordTransition if that's possible - it may not have been necessary before, but it enables things like this, and the introduction of validations which we've discussed internally.

[dmagliola]

Agreed on the long term benefits. We'd have to upgrade to v4.0, for that, though, and release a v3.4.1 (or v3.5?) which reverts this change and includes some sort of deprecation notice when using Transition classes that don't include it, right?

Or am I doing deprecations wrong?

[timrogers]

The version prior to this one had a bug anyway though, so it's not that clear that you can just revert to that and be in a clean state. I'm not that sure what to do.

@hmac Any thoughts?

[hmac]

I think the best bet here is to release an immediate fix that does something along the lines of what @dmagliola suggested: test for the presence of updated_timestamp_column on the transition class before trying to call it.

I think we should then follow up with a change that makes it possible to include Statesman::Adapters::ActiveRecordTransition even if you're using a json column, so we can remove that bit from the README (we don't have to enforce this necessarily, which means it won't be a breaking change).

I think this will get us into a better place, and then we can think about a more long term plan for what we want to do here.

What do you think?

[timrogers]

That makes sense to me. Working on a fix now.