[base + modules] Issue #670 - Remove side effects in manage_schema functions by robvandenbogaard · Pull Request #671 · driebit/ginger

robvandenbogaard · 2020-07-07T14:28:24Z

To avoid concurrency problems in loading data models between dependent modules, remove (database) side effects from manage_schema/2 functions, because in its current form the intended use is to return the data model declaration and let Zotonic take care of installing/updating in dependency order.

rl-king · 2020-07-07T14:30:28Z

ping @mworrell

robvandenbogaard · 2020-07-07T14:33:33Z

modules/mod_ginger_base/mod_ginger_base.erl

        ]
-    },
-    z_datamodel:manage(?MODULE, Datamodel, Context),
-    schema:create_identity_if_not_exists(editor_dev, "redacteur", "redacteur", Context).


Proposal to remove the automatic creation of editor_dev. If we need one we should provide it in fixtures.

Ok, I've never used it, maybe @loetie has

There is also an manage_data/2 function. That is called after the manage_schema/2 has been handled.

See https://test.zotonic.com/page/1353/modules

manage_data(_Version, Context) -> %% Whatever data needs to be installed after the datamodel %% has been installed. ok.

Good point, thanks.

BTW schema is a dangerous name for a module. I can imagine that other projects will also try to use this generic name. Better prefix it with ginger_, as it is a Ginger-only module.

Yes, I completely agree, that struck me too.

rl-king · 2020-07-07T14:31:12Z

modules/mod_ginger_base/mod_ginger_base.erl

        ]
-    },
-    z_datamodel:manage(?MODULE, Datamodel, Context),
-    schema:create_identity_if_not_exists(editor_dev, "redacteur", "redacteur", Context).


Where did this go?

It went nowhere, as I think it is not used / we should not use it (here) and rethink it when we need it.

Use manage_data/2, it is called after manage_schema/2

manage_data(_Version, Context) -> %% Whatever data needs to be installed after the datamodel %% has been installed. ok.

rl-king · 2020-07-07T14:31:57Z

modules/mod_ginger_banner/mod_ginger_banner.erl

@@ -14,12 +14,11 @@
 ]).

 manage_schema(_Version, Context) ->


Context -> _Context (unused)

rl-king · 2020-07-07T14:32:27Z

modules/mod_ginger_embed/mod_ginger_embed.erl

@@ -21,7 +21,7 @@
 -include("zotonic.hrl").

 manage_schema(_Version, Context) ->


Context -> _Context (unused)

rl-king · 2020-07-07T14:32:44Z

modules/mod_ginger_message/mod_ginger_message.erl


 manage_schema(install, Context) ->
-    Datamodel = #datamodel{
+    #datamodel{


Context -> _Context (unused)

rl-king · 2020-07-07T14:33:49Z

modules/mod_ginger_tagger/mod_ginger_tagger.erl

@@ -19,18 +19,20 @@
 ]).

 manage_schema(install, Context) ->


Version is install?

Context -> _Context (unused)

Version type signature is:

-type manage_schema() :: install | {upgrade, integer()}.

rl-king · 2020-07-07T14:35:28Z

modules/mod_ginger_base/mod_ginger_base.erl

        ]
-    },
-    z_datamodel:manage(?MODULE, Datamodel, Context),
-    schema:create_identity_if_not_exists(editor_dev, "redacteur", "redacteur", Context).


Ok, I've never used it, maybe @loetie has

robvandenbogaard · 2020-07-07T14:37:03Z

modules/mod_ginger_rdf/mod_ginger_rdf.erl

-    z_datamodel:manage(?MODULE, Datamodel, Context),
-
+    %% TODO: Legacy code ahead! This function must not have side effects;
+    %% it should only return the data model.


We should look into this case to see if it is still necessary to keep this here, or if it could be in a better place.

Use manage_data/2 to update/install data after the schema has been installed.

Issue #670 - Remove side effects in manage_schema functions

09e3eaf

robvandenbogaard added the base label Jul 7, 2020

robvandenbogaard requested review from loetie and rl-king July 7, 2020 14:28

robvandenbogaard self-assigned this Jul 7, 2020

robvandenbogaard commented Jul 7, 2020

View reviewed changes

rl-king suggested changes Jul 7, 2020

View reviewed changes

robvandenbogaard commented Jul 7, 2020

View reviewed changes

Issue #670 - Cleanup unused variables

c98e566

		@@ -21,7 +21,7 @@
		-include("zotonic.hrl").

		manage_schema(_Version, Context) ->

Conversation

robvandenbogaard commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rl-king commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robvandenbogaard commented Jul 7, 2020 •

edited

Loading