farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
59 stars 38 forks source link

Create a system for versioning and upgrading field modules #244

Open jgaehring opened 4 years ago

jgaehring commented 4 years ago

I realize this will be a necessary feature to have from the very start, because when we add the logic for filtering logs on both syncing and caching operations, that will mean that some logs won't have gotten the 'my-logs' tag added to their modules field, so they will be inaccessible if they were marked done before the upgrade occurs.

@mstenta and I are going to work out some steps to making sure we can track a schema version for every field module and provide the appropriate hook for upgrading, similar to how Drupal does it...

mstenta commented 4 years ago

I think these are roughly the things we'd need to do in order to replicate how Drupal module updates work:

  1. Create a new database table in IndexedDB to store information about modules, including module name and schema version.
  2. Add a schema version property to each module's module.config.js file, which is incremented whenever a schema version change is needed.
  3. When a module update requires that changes be made to the schema, it should provide a function (somewhere that FK knows to look), which performs the necessary updates. For example, load all logs and add the my-logs tag to their modules field. This function should be tied strictly to a schema version.
  4. When FK loads modules, it should look at the schema version stored in IDB, and also look at the version in module.config.js. If the version in module.config.js is higher than the version in IDB, then it should run ALL the update functions between those two versions to bring things up to date, and then update the IDB version to match.
jgaehring commented 4 years ago

Ok, so I've taken a first pass at this: 1be2e6b.

@mstenta, one thing I recall you mentioning was that the module version and its schema version should not be the same. What was the reason for this? I'd also love to walk through this with you and see if you think it meets the requirements.

mstenta commented 4 years ago

Awesome!! I'd love to walk through it with you to see how it works.

the module version and its schema version should not be the same. What was the reason for this?

So... in Drupal (but the same would apply here), you could conceivably have multiple schema updates in between actual module versions. That's why they are considered separate numbers. Doing this ensures that ALL updates are run as needed, regardless of which commit a user has running.

I'd recommend reading the Drupal documentation for hook_update_N(): https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension%21module.api.php/function/hook_update_N/8.8.x

There are some good things to consider there, which are probably also good to keep in mind for Field Kit updates.

Some choice sections from the "Notes about the function body" on that page:

You do not know when updates will be run: someone could be keeping up with every update and run them when the database and code are in the same state as when you wrote your update function, or they could have waited until a few more updates have come out, and run several at the same time.

Never assume that the database schema is the same when the update will run as it is when you wrote the update function. So, when updating a database table or field, put the schema information you want to update to directly into your function instead of calling your hook_schema() function to retrieve it (this is one case where the right thing to do is copy and paste the code).

Be careful about API functions and especially CRUD operations that you use in your update function. If they invoke hooks or use services, they may not behave as expected, and it may actually not be appropriate to use the normal API functions that invoke all the hooks, use the database schema, and/or use services in an update function -- you may need to switch to using a more direct method (database query, etc.).

They may not apply directly to Field Kit, but the lessons and reasoning are good to think about generally.

TL;DR: be careful about assumptions in update functions. Code may change elsewhere between when you write an update function and when it gets executed by a user.

mstenta commented 4 years ago

(this is one case where the right thing to do is copy and paste the code)

This was something that jumped out at me when I was learning to use that hook in Drupal. It goes against the normal inclination to avoid duplication (DRY). But there's a good reason to repeat code in the case of updates.

jgaehring commented 4 years ago

Great, thanks @mstenta!

I guess I'm still a little unsure about what the difference would be between the module version number and it's schema version number, because unlike Drupal, I don't envision giving field modules the same level of autonomy as Drupal modules, that is I don't really want them intereacting directly with the database if I can help it. That's largely what the filter object is intended to do—to be that layer of abstraction between the field module and the database so it doesn't have to interact directly with the database.

Right now, I've only got the logic for running the upgrades; I haven't written the upgrade for My Logs v1, and I don't think suspect I'll need to write an upgrade for Weather v1. It's hard at this point to imagine what all sorts of edge cases I need to consider. As it stands now, I'm just doing a few basic things:

jgaehring commented 4 years ago

I don't really want them intereacting directly with the database if I can help it.

I realize the one instance where the field modules might need to interact more directly with the database is actually here in the upgrade functions. For instance, I anticipate that My Logs v1 will need to access every log that's currently in the database append the 'my-logs' tag to its modules field.

mstenta commented 4 years ago

I don't really want them intereacting directly with the database if I can help it.

Hmm giving this further thought...

The case we have right now (needing to add my-logs to the modules field) is not technically an upgrade coming from the My Logs module - it's an upgrade coming from Field Kit itself. The My Logs module is not responsible for the database table that needs the update. Field Kit is. So really, in this case, the upgrade logic belongs in Field Kit itself - not in the field module.

Maybe we don't actually need "module upgrade functions" right now. And we just need "Field Kit upgrade functions". Does that make sense?

Field modules don't define their own schemas (yet), so maybe we were jumping the gun by thinking about these upgrades as coming from modules. It's a little muddy in my mind, because in Drupal EVERYTHING is a module - even the core system pieces. So they all use the same update process.

jgaehring commented 4 years ago

The case we have right now (needing to add my-logs to the modules field) is not technically an upgrade coming from the My Logs module - it's an upgrade coming from Field Kit itself.

Oh interesting... I hadn't thought of it that way, but that sounds right. Hmm...

Perhaps this could be handled by an upgrade to IDB. We have some logic for that, although it hasn't been tested yet, since this would be our first upgrade. Perhaps I can apply some of this thinking to setting up that system in a sensible way that will work for this situation.

mstenta commented 4 years ago

Sorry! I hope the work you did wasn't for naught! I do expect that we'll eventually want to let field modules define their own schemas as well, at which point field module update functions will be necessary.

jgaehring commented 4 years ago

We'll also want to make sure we add the version number to the modules section of the /farm.json endpoint when we implement this. See https://github.com/farmOS/farmOS-client/issues/309#issuecomment-590482123.