Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ingredients as new content structure #2061

Merged
merged 57 commits into from
Jul 1, 2021

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Apr 8, 2021

What is this pull request for?

This introduces a new model of storing content in Alchemy.

Before we had a mixture of several rather complex objects to store content in Alchemy:

Element has many contents (Alchemy::Content) that belongs to essence (polymorphic Essence classes). That worked well for the last decade but has several issues in large scale applications:

  1. You can't properly eager load or join polymorphic associations
  2. You can't create or destroy a large amount of polymorphic associations
  3. Confusing object tree

Now we make use of Rails' STI to build

Element has many ingredients (Alchemy::Ingredient child classes). That's it 🎉. Now we can

  1. Properly eager load and join all ingredients including their associations
  2. Can create and destroy large amounts of ingredients
  3. Have an understandable object tree

In order to make that work we normalized the alchemy_ingredients table to:

  1. A value column that holds the primary value of an ingredient. (A string of text for the Text ingredient or the date of the Datetime ingredient.)
  2. A data JSONB (JSON in non-postgresql rdbs) column, that holds additional attributes like the link_class and link_title attributes of the Link ingredient.
  3. A related_object belongs_to association for ingredients having related objects like Alchemy::Picture, Alchemy::Page for the Picture and Page ingredients.

We are pretty confident that this is all we need to support the same flexible content structure Alchemy provides you for for over a decade now, but allows us to scale Alchemy for large installations and truly serve as a content API for the JAMStack age.

Upgrading

We introduce this change in parallel without touching the current Content Essence structure at all, making it possible to run a hybrid approach. This is completely opt-in.

I order to upgrade one of your elements just change

 - name: article
-  contents:
-    - name: headline
-      type: EssenceText
-    - name: text
-      type: EssenceRichtext
-    - name: image
-      type: EssencePicture
+  ingredients:
+    - role: headline
+      type: Text
+    - role: text
+      type: Richtext
+    - role: image
+      type: Picture

and run this rake task

rake alchemy:upgrade:6.0:create_ingredients

And you are good to go. 🚀

This task is idempotent, so you can upgrade element by element. It will skip elements already having ingredients (but will destroy all contents and essences of a succesfull migrated element).

During rendering Alchemy will prefer ingredients over contents.

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

@tvdeyen tvdeyen added this to the 6.0 milestone Apr 8, 2021
@tvdeyen tvdeyen self-assigned this Apr 8, 2021
@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch 7 times, most recently from d219ef6 to 66d6f03 Compare April 10, 2021 09:44
@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch 6 times, most recently from 1992e16 to ced45a0 Compare April 15, 2021 08:36
@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch 4 times, most recently from 4d935e6 to 84d1ed3 Compare May 27, 2021 06:52
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6043 lines exceeds the maximum allowed for the inline comments feature.

@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch 3 times, most recently from a5a560a to 48606e0 Compare June 3, 2021 20:25
@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch 2 times, most recently from 49dbe71 to 05ddc0d Compare June 7, 2021 14:40
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6358 lines exceeds the maximum allowed for the inline comments feature.

@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch from 05ddc0d to eb2ffd1 Compare June 7, 2021 15:19
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6681 lines exceeds the maximum allowed for the inline comments feature.

@tvdeyen tvdeyen changed the title Normalize contents and essences Introduce ingredients as new content structure Jun 29, 2021
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, I ran into some issues with actually displaying ingredients rather than contents.
Also, I had to remove a bunch of cache blocks in page and element partials in order to get a debugger into the ElementsViewHelper. I am guessing that we're still missing touching elements and pages when we update ingredients.

This is all pretty small stuff though, I think with these couple of issues fixed, this might be good to go! Hello performance improvement :)

app/helpers/alchemy/elements_block_helper.rb Outdated Show resolved Hide resolved
mamhoff and others added 5 commits June 29, 2021 20:38
Otherwise timestamps will not update in the necessary fashion.
Without this, ingredient-based elements will not be published.
No need to implement this ourselves, Rails has us backed.

Thanks @hmans 🎉
@tvdeyen tvdeyen force-pushed the normalize-contents-and-essences branch from 5d77542 to 42c2e4d Compare June 29, 2021 18:38
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6650 lines exceeds the maximum allowed for the inline comments feature.

Here we know that we want to load all elements including their ingredients.
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6654 lines exceeds the maximum allowed for the inline comments feature.

@tvdeyen tvdeyen marked this pull request as ready for review June 29, 2021 18:58
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6734 lines exceeds the maximum allowed for the inline comments feature.

@tvdeyen tvdeyen removed their assignment Jun 29, 2021
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 6759 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in. We need this in the main branch to be able to work on our dependent libraries.

@tvdeyen tvdeyen merged commit 6552f21 into AlchemyCMS:main Jul 1, 2021
@tvdeyen tvdeyen deleted the normalize-contents-and-essences branch July 1, 2021 11:17
@robinboening
Copy link
Contributor

Wow, thats a big change! I like the approach of supporting both ways and allowing to slowly opt-in to the new structure. ❤️

I am glad we are getting rid of the polymorphic association. Simplifying the associations is a big win! I believe it is the right way. Thanks! 👏 🥳

I'd like to add my 2 cents though as replacing polymorphism with STI comes with trade offs. STI has its very own problems, especially later in the future when it comes to extending the data model. The introduced json(b) column helps with that on the first glance but it comes at a cost and shouldn't be underestimated. I've been bitten by json(b) structures many times by now, up to a point where I promised to not use them again in my life (and I did anyways). Very easily and quickly the data integrity can get compromised and aside from that querying those structures is a real pain (even queries that seem simple at first often turn into monsters).

I know you thought about the pros and cons of the change a lot and in depth and I also believe the trade offs are worth the simplifications and the performance gain.

I am looking forward to trying it out, but I will probably need to wait a bit longer as stuff is piling up here...

Thanks again!

@tvdeyen
Copy link
Member Author

tvdeyen commented Jul 2, 2021

Thanks, Robin.

Yes, we are aware about these potential issues and weighted them out. Ingredients as well as essences always have been and will be "typed single value" records. The additional attributes are secondary options like link titles or css class names. Those will most likely never be queried on collections or used in search.

That said, we should think about adding an indexed column where ingredients like the Richtext can put data that will be queried frequently (ie. full text search).

Thanks for the input.

@robinboening
Copy link
Contributor

robinboening commented Jul 2, 2021

One thing that came to my mind is the https://github.com/ruby-json-schema/json-schema gem, which allows to validate specific json schemas (as the name suggests 😆) on the application layer. In the case of STI it should be possible to validate a schema per inheriting model, I suppose. It might not be needed to validate every model and also not its entire schema, but maybe some things could be useful. It's just a suggestion to look into.

Since you mentioned full text search and indexing the data column, let me add one more thing to keep an eye on and be cautious with.

DISCLAIMER – the following is only postgres and JSONB related as I don't know how other DBMS handle JSON types.

Even with btree/gin indexes there could easily be cases were the query planner won't be able to be as smart as with normalized data. This is because postgres does not collect statistical information about the distribution of values for JSONB types. This can lead to very expensive queries even with proper indexes in place.

I find this article (11/2017) a very good read on that topic as it explains cases where the query planner makes bad estimates, thus bad choices, but it also shows a workaround using functional indexes and mentions the extendability of postgres statistics from v10. https://blog.anayrat.info/en/2017/11/26/postgresql-jsonb-and-statistics/

@tvdeyen
Copy link
Member Author

tvdeyen commented Jul 2, 2021

Yeah, I thought about json schema integrity as well, but the same comment from above applies here as well. The additional attributes in the data JSON are secondary attributes, besides the Richtexts stripped_content (what is used by search indexes), and does not need that complexity IMO.

What do you think about adding a dedicated column for indexed content from the Richtext ingredient (See https://github.com/AlchemyCMS/alchemy_cms/blob/main/app/models/alchemy/ingredients/richtext.rb#L37 for backgorund). Maybe indexable_value?

Thoughts @mamhoff @rmparr ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants