Deprecating hstore meta_data fields


(Sam Saffron) #1

Continuing the discussion from Custom user fields:

I am thinking of removing topic.meta_data and all other uses of hstore and replacing with the same pattern I created for User.custom_fields. Keeping naming and implementation consistent for Post,User,Topic

Its easier to index, has better auditing (timestamps) and thins out our tables cause data is stored in an external table. So, when you do Topic.first you are not yanking out meta_data hstore columns, can use the same implementation on Post,User,Topic with the same interface, overall this is all win.

Also, I kind of like the name custom_fields as its fairly descriptive of the role here.

Alternatively if we REALLY want hstore lets rename this from the awkward meta_data name and use an external table for the hstore column with has_one back to the original table.

I want the code centralized, names consistentaized and data extracted to an aux table (my main priorities). I feel hstore is a bit overkill here, but could accept it if you feel strongly you really want it.

Thoughts ?

@neil @eviltrout @zogstrip @lightyear @radq anyone?


(Benjamin Kampmann) #2

That sounds good to me. Very nice and flexible system and allows for easy extending through plugins. So, I do understand correctly that after, instead of using the plugin-store in a weird way my shared edits plugin could just use post.custom_fields["shared_edit_authors"], right?

Yes, sounds way more practical and if having the proper relationships on the database also prevents from old state to unrelated objects in the database. One question about that though: would you have one custom_fields table for all the three types or one per type? Just asking because I’d be curious how you’d solve data consistency issues with first approach.

Generally, I am all up for it. Not seeing any specific reason why one would require the hstore. Unless it is way faster or easier to handle.


(Sam Saffron) #3

Actually, with very little modification we could support

post.custom_fields["shared_edit_authors"] = ["bob","bill"]

Stored in:

post_custom_fields

post_id, name,         value
-------  ----          -----
1        edit_authors  bob
1        edit_authors  bill

This leaves the data easily queryable

-- all bobs edited posts
select * from posts p
join post_custom_fields on
    post_id = p.id and 
    name = 'edit_authors' and
    value = 'bob'

I would have 3 tables, one per type, it makes querying simpler and we end up storing less duplicate data. Not a huge fan of polymorphic relationships in ActiveRecord.


Add UI for editing custom per-topic fields
(Benjamin Kampmann) #4

Oh, that’d be nice indeed!


(Benjamin Kampmann) #5

@sam, can I ask to have the same on categories? And maybe groups, but I am not sure whether that is a use-case for anyone.

For category I do have a Use case on my todo-list for a plugin that need to extend the “metadata” for categories. One important field is that if you post in certain categories (like health or legal), we are legally required to show a custom message, stating this is not a replacement for asking a lawyer/doctor respectively. This could be handled with these fields easily - assuming they allow texts long enough.


(Johan Jatko) #6

I had a pull request up with a similar system, but closed it as there was not much interest at the time. Glad to see that its active now :slight_smile:

Technically this could be implemented for all models, but i think the main ones: Topic, Post, Category, User, Group should be enough for most plugins.


(Neil Lalonde) #7

I agree. We don’t need hstore. I’ve always looked for ways to use hstore, but end up choosing simpler solutions like custom_fields.

Also, “consistentaized”. Nice.


(Robin Ward) #8

I agree this is the right direction. I think HSTORE is awesome but I think I always wanted it to be a bigger part of our stack than it ended up being. If it’s not solving this problem for us let’s switch.


(Benjamin Kampmann) #9

@sam, let us know if you need/want/accept any help on this.


(Sam Saffron) #10

If you feel like giving this a shot feel free, I can skip working on this for say a week but want to make sure its in fairly soon

Would split to 3 pieces

  1. extract concern has_custom_fields, extract specs, add concern to categories and other tables that don’t have meta_data with migrations for the tables
  2. build a migration that moves meta_data to custom_fields (can only use SQL or standalone ruby - no models), nuke meta_data from topics
  3. add the fancy array support I demonstrated earlier in topic

(Benjamin Kampmann) #11

I was mainly offering in order to get it in rather sooner than later :wink: . But sure, I’d love to take care of this – already started implementing. You’ll have a PR early next week. Have a great weekend.


(Benjamin Kampmann) #12

@sam, I’d need your feedback.

I was able to implement the CustomFields as discussed, including the meta-data-migration-support for Topics (and backwards compatibility using topic.meta_data). On the way I found a few bugs I was able to fix including tests (saving a second time didn’t save), but after implementing the Array-Support I have a few concerns and I’d like your opinion about them before submitting for Review:

  1. The whole part is – as it is and was – not transaction save: between reading the database and creating the values by us is a gap that could potentially lead to inconsistent data which wasn’t possible with meta_data before. Do we want to take the risk it or shall we wrap it into a transaction potentially slowing down the access?

  2. We are only checking against our copy of custom fields. If fields have changed in the database during the lifetime of our instance we don’t look at that at the moment meaning that even after calling save on the source we can’t be sure the model is reflecting the current database state. Making its behaviour a little fuzzy.

And a last remark about the Array-Support: in order to not have it recreate the items every time while also ensure the order of the Array (which is bound to the primary key ordering), I have to collect all items first and compare them to the Array given. Which is better than recreating the Array every time on save, but makes the code appear much more complex for such a tiny feature. We could also drop that and only support sets (not a fan of that proposal though).


(Sam Saffron) #13

AR by design on #save keeps before_save and after_save in the same transaction. It should be safe.

This can be fixed if we super insist, but seems very very complicated for very little gain. I would not worry about this for now.


(Benjamin Kampmann) #14

Oh, didn’t know. Awesome. Good then.

Agreed. We don’t have long-living objects anyhow so this is very unlikely to be a problem. Just wanted to ensure we are on the same page for that.

Here is the PR for it : Custom fields for Topic, Category, Post and Group by gnunicorn · Pull Request #2297 · discourse/discourse · GitHub


(Sam Saffron) #15

Thanks heaps, looks good.

One concern we need to sort out is a clean pattern for returning back data from serializers (optionally, if a plugin opts in) without an N+1 query. I would think plugins will opt to return a field or two on, perhaps post (for custom rendering or something).


(Benjamin Kampmann) #16

Not sure I understand this entirely, do you mean that you want to prevent loading all CustomFields in the serializer (per object) or do you want only one request for all custom fields of a list of models at the time of the serialization to prevent stair case DB-Requests?


(Sam Saffron) #17

I want a pattern for loading a custom field or 2 per post on a topic page … and per topic on a topic list page. (in which only a single db call is made to get all custom fields required)


(Jens) #18

Hello,

great, as far as I can tell, custom fields are implemented!? Is it correct that, for example when creating a post, topic, category… I can then pass an additional parameter like:

"custom_fields":[{"mykey":myvalue"}]

when creating a category/topic/post, and then these custom fields will be persisted?

I tried it, but it seems it did not work when creating at topic, thats the reason I am asking (and just wanted to make shure this is implmented and if the syntax is correct).

thanks