Category serializer modification is behaving differently in 2.4.0

After upgrading to https://github.com/discourse/discourse/releases/tag/v2.4.0.beta2, almost all of our plugins (i.e. topic list previews, events, locations, ratings etc) have been experiencing an issue with the add_to_serializer plugin.rb method.

Existing usage followed this format:

add_to_serializer(:serializer, property) { value }

This no longer works in a production environment. It still works in a development environment.

Initially, I thought it might have something to do with the way plugin enabling is handled. As add_to_serializer’s _include? method uses the enabled? state

if define_include_method
   # Don't include serialized methods if the plugin is disabled
  klass.public_send(:define_method, "include_#{attr}?") { plugin.enabled? }
end

However usage of the enabled_site_setting system doesn’t seem to address the issue. Moreover, enabled? seem to still default to true anyway

def enabled?
  @enabled_site_setting ? SiteSetting.get(@enabled_site_setting) : true
end

To fix the immediate issue we’ve changed the way our plugins are serializing data, however I’d like to get to the bottom of this if possible. Anyone have any ideas on what’s going on here?

cc @merefield, @fzngagan

9 Likes

This is highly unusual since we use this in polls and they are certainly not broken. The only thing I can think of is they were broken in the beta release but fixed in master? Can you try on tests-passed and see if they work?

9 Likes

I’ll test it further today. The usage in polls is slightly different, as the _include? method is always precluded with a third false parameter. I suspect the include? method may be the issue.

add_to_serializer(:post, :polls_votes, false)
2 Likes

It seems this issue is specific to modifications of the category serializers.

https://staging.discourse.angusmcleod.com.au has a single plugin installed, “test-add-to-serializer”

https://github.com/angusmcleod/test-add-to-serializer/blob/master/plugin.rb

As you can see in the plugin.rb, a number of serializer modifications are made.

The modifications to the non-category serializers seem to be working, but the modifications to the basic_category serializer only work in development.

You won’t find the test category properties at

https://staging.discourse.angusmcleod.com.au/categories.json

or

https://staging.discourse.angusmcleod.com.au/c/records-musicians.json

But you will find the other test properties at

https://staging.discourse.angusmcleod.com.au/t/this-is-a-title-of-a-topic/42.json

or

https://staging.discourse.angusmcleod.com.au/latest.json

So the issue seems to be not about the add_to_serializer method per se, but the modification of the basic_category serializer.

6 Likes

@joffreyjaffeux @dan Hey :), it looks like you guys have done some recent work to discourse-voting to address the same issue I’ve outlined above.

https://github.com/discourse/discourse-voting/commit/7e6f27b198416a6ef27f0f0df715f5e6cfc00197

Do you know why it’s no longer sufficient to add properties to the basic_category_serializer?

3 Likes

I can confirm this is affecting tests-passed

1 Like

Dan is out, but essentially there are 3 different places we serialize categories with varying amounts of data. We will see if there is any way to eliminate the mixin here which makes this harder to reason about.

3 Likes

Fixed and backported to stable and beta.

https://github.com/discourse/discourse/commit/bd5fa1737d42a958bb5cda35ae5bab2464d4e084

Dan correctly setup the inheritance structure, it is just that we had a very very very long standing core bug.

9 Likes

Thanks @sam, much appreciated.

4 Likes

Thanks for reporting and pushing on this, it was a very very tricky one to nail down I spend some time reading rails internals to figure this out.

Be sure to check out the new test if you are curious at how we ensure this is good going forward.

5 Likes

Thanks a lot, @sam for fixing this quickly.

@angus. Will our plugin work without the recent changes we made due to this fix?