feat(apps): plugin source in its own model #9825

mariusandra · 2022-05-17T20:54:39Z

Problem

If we want to offer apps, we will need to store more than just one source text field on the plugin. This PR makes that possible. It also paves the wave for generic multi-file plugins, if we ever want them.

Changes

Create a new PluginSource model for storing source for a plugin
Deprecate the existing source field, and create a migration to move old data over. Remove the field from the serializer.
Add API endpoints to get/update the plugin source in a compact way (the frontend won't deal with separate PluginSource objects, but will get all files and send all changes to plugins/:id/source in one object {"index.ts": "code", "plugin.json": "{}"}).
Replaced editing of just the config json with editing of the full plugin.json. This means that now the plugin's name is also editable inside the JSON, not as a separate field. This greatly simplified the code loading paths in the plugin server -> archive, source or local -> they all work very similarly now.

How did you test this code?

Wrote tests for the new parts in django and the plugin server
Tried a lot of changes in the interface myself

Next steps

Add frontend.tsx and transpiling support.
Load that on the frontend.

macobo · 2022-05-18T08:00:14Z

Q: What is the operational effect of this to plugin-server? How does it affect mean-time-to-ingest events?

yakkomajuri

Left some comments.

I'm also curious about the answer to @macobo's question.

Another thing I'd really love in general is if we could split the behavioral changes from the scaffolding (models, migration, etc).

Why? That makes it so much easier to revert if things go sour. We first migrate the data, check things work as expected, then merge the behavioral changes, which we can revert independently.

Although as long as this is well tested we should be good 👍

frontend/src/scenes/plugins/source/createDefaultPluginSource.ts

yakkomajuri · 2022-05-18T09:47:27Z

posthog/api/plugin.py

@@ -176,6 +175,15 @@ def get_queryset(self):
                return queryset
        return queryset.none()

+    def get_plugin_with_permissions(self, reason="installation"):
+        plugin = self.get_object()


nit: small thing but we probably don't need to call this if the validation below fails i.e. this can be moved all the way down.

Hmm.. not sure what do you mean?

def get_plugin_with_permissions(self, reason="installation"): plugin = self.get_object() # move this down? but then `plugin.organization` won't work...? organization = self.organization if plugin.organization != organization: raise NotFound() if not can_install_plugins(self.organization): raise PermissionDenied(f"Plugin {reason} is not available for the current organization!") return plugin

ah brain 💨

posthog/api/plugin.py

yakkomajuri · 2022-05-18T09:52:43Z

posthog/api/plugin.py

+            if sources.get(key):
+                if sources[key].source != value:
+                    performed_changes = True
+                    if value is None:


do we want to allow this? can't we prevent breaking a plugin at an earlier stage (here) vs. finding out a file is missing when loading the plugin in the plugin server?

Or what's the legitimate reason for this?

The interface doesn't support it yet, but as we basically allow creating and editing any source file now, you may also want to delete the files. Thus if you pass {"frontend.tsx": null} (or undefined), that source file will be removed, instead of being rewritten to "". Just pass "" if you want an empty file.

Also, you need to explicitly send a file as a key with null/undefined/None to delete it. If you omit the key altogether in the update call, the source file won't be touched at all.

posthog/api/plugin.py

posthog/models/plugin.py

mariusandra · 2022-05-18T15:58:04Z

Q: What is the operational effect of this to plugin-server? How does it affect mean-time-to-ingest events?

I have not done measurements, but it should be virtually negligible.

The only thing that changed is the way we load code for "source code" plugins (written with the editor). Now instead of getting the source from the Plugin model (that's already fetched by this point), we make two extra async database query per plugin to fetch its source and possibly fetch and transpile the frontend source.

That may sound like a lot (N+1 queries anyone?), but:

Most plugins are installed from the plugin repository, and nothing changed from them. For example we still extract the index.ts from a zip when loading those, or load a local file for local plugins. Only source plugins changed.
I was contemplating using a join/subquery to add the source to the fetched plugin model, to mimic the current API and remove the extra query, but decided against it. Basically the complexity we get with adding new fields to the query that differ from the model, keeping those in sync whenever plugins reload, or using some separate source caching layer, is not really worth the complexity, as the win is likely measured in milliseconds. Without these "optimizations", the codepath for loading a plugin is now exactly the same for local, remote and source plugins, with one changed "getFile" function.

mariusandra · 2022-05-18T16:04:55Z

Another thing I'd really love in general is if we could split the behavioral changes from the scaffolding (models, migration, etc).

Why? That makes it so much easier to revert if things go sour. We first migrate the data, check things work as expected, then merge the behavioral changes, which we can revert independently.

Generally agree, but here we can't really only migrate the data, without migrating all the other code paths as well. Otherwise the migrated data itself will grow stale, if someone changes the old data through the unchanged interface... 🤔

The scaffolding needed to support that will turn complex itself.

mariusandra · 2022-05-18T19:41:13Z

Ready for a review again.

macobo

Added some comments for the python/frontend side of things.

Is it at all possible to separate out the non-plugin-server changes on this? As-is if this introduces a subtle bug this is not at all revertable, which is a no-go for the plugin-server.

macobo · 2022-05-19T06:39:24Z

posthog/api/plugin.py

+            response[source.filename] = source.source
+
+        # Update values from plugin.json
+        plugin_json = json.loads(response.get("plugin.json") or "{}")


Suggested change

plugin_json = json.loads(response.get("plugin.json") or "{}")

plugin_json = json.loads(response.get("plugin.json", "{}"))

Relying on behavior of falsy values is generally an antipattern in python. .get() already has a default value argument.

This was explicit. The plugin.json string may contain "", and response.get("plugin.json") will then return "", while I want to get None instead

That's the kind of thing sure to will lead to bugs as things get refactored, especially when not covered by separate tests. Could we make the intent for "" vs None behavior more explicit via a model property or something?

macobo · 2022-05-19T06:40:14Z

posthog/api/plugin.py

+        if plugin_json["name"] != plugin.name:
+            plugin.name = plugin_json["name"]
+            performed_changes = True
+        if json.dumps(plugin_json.get("config") or []) != json.dumps(plugin.config_schema or []):


Same - use dict.get second argument here.

Same reasoning here. plugin_json.get("config") will return None, but I want that to be comparable to [] in this line, if that's what plugin.config_schema contains

posthog/api/plugin.py

macobo · 2022-05-19T06:43:51Z

posthog/api/test/test_plugin.py

+        self.assertEqual(response.json(), {"index.ts": "'hello again'", "plugin.json": '{"name":"my plugin"}'})
+        self.assertEqual(mock_reload.call_count, 3)
+
+        response = self.client.patch(


Q: Why do we have a test that tests N different scenarios? This makes for a hard to understand and debug test suite. Could we give each scenario a name and test separately?

I decided against that initially to avoid needless scaffolding. The test essentially sets up a story, and then tests based on that story, in 5 steps. I can change maximally optimise this to be three tests with 3, 4 and 4 steps respectively. This seemed wasteful, so I opted to have one 5-step story test the entire flow.

I added helpful hints to make it easier to follow, but can break it up if you think this makes sense.

macobo · 2022-05-19T06:44:55Z

posthog/models/plugin.py

+
+    plugin: models.ForeignKey = models.ForeignKey("Plugin", on_delete=models.CASCADE)
+    filename: models.CharField = models.CharField(max_length=200, blank=False)
+    source: models.TextField = models.TextField(blank=True, null=True)


Why can this be null?

When the source is actually in a .zip file, and we use this model to cache the transpiled output.

Let's add a comment to that effect.

posthog/models/plugin.py

mariusandra · 2022-05-19T07:11:08Z

Thanks for the review @macobo. Left some answers, not sure if good answers or not.

Is it at all possible to separate out the non-plugin-server changes on this? As-is if this introduces a subtle bug this is not at all revertable, which is a no-go for the plugin-server.

Everything's possible, but I'm worried about the complexity this brings. Right now, we:

Create a new model and copy over the data
Start using the new model immediately
Profit

If I split the PRs for data + code/UI,

Create a new model and copy over the data
Keep using the old model, and watch the new data go ever more stale
Finally migrate the UI, hoping the old copied data is still valid.

I could of course move the "copy the data" from point 1 into a second migration in the second PR, but this all seems... excessive.

So not sure how to proceed. 🤔

macobo · 2022-05-19T09:05:27Z

Goal: Make it so we can roll back if something goes wrong in the plugin server.

Proposal: Scope down the plugin-server.ts change to db.ts only where you now do a JOIN but assume that there's only a single file. Then a follow-up PR does the rest of the vm and other changes.

db.ts change should be relatively safer than vm changes so we achieve our goal of rollbackability in the most critical area.

mariusandra · 2022-05-19T10:41:46Z

Going to close this as well in favour of the forks:

mariusandra added 10 commits May 17, 2022 14:00

create plugin source model

33ab6d2

edit source via plugin_source model

cd41f1f

deprecate "source"

3792778

test plugin source updates

cdfdd18

add support for index.ts and index.js

87e418c

refactor plugin loading, support plugin sources from db

3d3f7c4

fix source code in tests

c6112de

remove transpilation code

22f893a

reload plugin after saving

5e3036e

store defaults in the db instead of persisting in form

b20e318

mariusandra requested review from macobo and yakkomajuri May 17, 2022 20:54

remove fields that don't exist

0c06e25

mariusandra mentioned this pull request May 17, 2022

feat(apps): transpile frontend.tsx #9828

Merged

mariusandra mentioned this pull request May 18, 2022

feat(apps): frontend apps #9831

Merged

yakkomajuri reviewed May 18, 2022

View reviewed changes

mariusandra added 3 commits May 18, 2022 17:38

Merge branch 'master' into plugin-source-db-model

16ed033

remove unused fields

ef0b545

commit suggestion

67da1d2

rename to PluginSourceFile

e1d4801

mariusandra requested a review from yakkomajuri May 18, 2022 19:36

macobo reviewed May 19, 2022

View reviewed changes

mariusandra added 4 commits May 19, 2022 08:55

fix code feedback

faa6a8f

add comments

5896017

make it safer to call

d2df123

convert to upsert

efc253e

convert to upsert

17bc497

comment on the null

b927c02

This was referenced May 19, 2022

feat(apps): plugin source in its own model, part 1 #9853

Merged

feat(apps): plugin source in its own model, part 2 #9854

Merged

mariusandra closed this May 19, 2022

	plugin_json = json.loads(response.get("plugin.json") or "{}")
	plugin_json = json.loads(response.get("plugin.json", "{}"))

feat(apps): plugin source in its own model #9825

feat(apps): plugin source in its own model #9825

Uh oh!

Conversation

mariusandra commented May 17, 2022

Problem

Changes

How did you test this code?

Next steps

Uh oh!

macobo commented May 18, 2022

Uh oh!

yakkomajuri left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

mariusandra commented May 18, 2022

Uh oh!

mariusandra commented May 18, 2022

Uh oh!

mariusandra commented May 18, 2022

Uh oh!

macobo left a comment

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!

Uh oh!

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!

Uh oh!

mariusandra commented May 19, 2022

Uh oh!

macobo commented May 19, 2022

Uh oh!

mariusandra commented May 19, 2022

Uh oh!

Uh oh!