Extension configuration is broken about pigallery2

[mblythe86]

It looks like loadExtensionList in ExtensionManager.ts used to add any directories it found into Config.Extensions.extensions, but it doesn't do that anymore. This feels like the root cause of these issues I mentioned:

When starting from an empty config, the admin page doesn't populate anything.

it still wouldn't load the extension. In fact, it didn't even recognize that a new extension had been added at all!

I haven't examined the most recent refactor of the extension loading code, so maybe the process to discover new extensions happens somewhere else now?

[mblythe86]

That seems to fix it, thanks!

[bpatrik]

I had a look. Unfortunately it not simple to fix so it will take a while.

I had to restructure the config lib to have UI generation support for extensions too, but that apparently broke some config loading.

[mblythe86]

I think I found the problem in this change.

Previously, __loadJSONObject() was called to populate the config values into the newly-created extension configuration object. After the change, the extension configuration object is placed as-is (i.e. with the default values) into the passed-in PrivateConfigClass object.

I think the fix is to load() or loadSync() the config again afterwards. I don't know whether this is best done within the loadToConfig() function itself, or if it should be done by the calling function. It looks like ExtensionConfigWrapper::original() and ExtensionConfig::setTemplate() are the only places where loadToConfig() is called, and original() already calls load() afterwards. With that in mind, in my local codebase, I added a loadSync() to setTemplate(). It seems to fix it.

I'll put together a pull request with my fix shortly.

[mblythe86]

Hmm, my simple change only fixes the case where some extension config already exists in the config.json file. If that file is deleted, or if an extension is added to an existing install, then the extension config is not created.

[mblythe86]

I might know what's going on now with regards to the empty/new extension config. I think it will need to be fixed in typeconfig.

This is where the extension configurations are added - an array of ServerExtensionsEntryConfig.

This is the code responsible for filling arrays from JSON. It has a special case to try JSON-decoding a string within an array, but aside from that, it will end up calling set(). In this case, it will end up clobbering the array of FIXME with null or something.

We're basically guaranteed that the config.json file exists by the time we initialize the extensions (and they call setTemplate()). So, even if the extension config objects are added to the global Config, they'll be removed the next time Config.load() is run.

[mblythe86]

It looks like typeconfig doesn't have this problem when a key is a nested @SubConfigClass because it should enter this condition, recursively call itself, and immediately return without clobbering anything (and I assume it uses the default values at that point).

Along that line, is there a way to dynamically add a @ConfigProperty to a @SubConfigClass? If so, instead of pushing extension configs into an array, we could add it as a new key (using the extension_id?) directly in ServerExtensionsConfig (or a nested @SubConfigClass that we use only as a container for extensions).

If that's not possible, then (and I'll be the first to admit this is an ugly hack), then maybe we could add a bunch of these to the ServerExtensionsConfig class:

  @ConfigProperty({
    tags: {
      name: $localize`Installed extension 1`,
      priority: ConfigPriority.advanced
    }
  })
  extension1?: ServerExtensionsEntryConfig;

  @ConfigProperty({
    tags: {
      name: $localize`Installed extension 2`,
      priority: ConfigPriority.advanced
    }
  })
  extension2?: ServerExtensionsEntryConfig;

...and so on...

[bpatrik]

added d4d8dcf that hopefully fixes the issue.

Long story short, with the nice UI support I had to make fundametnal changes to the config library that created a circular depdenency between the config and extensions. I need to major code rafactor around the extensions handling to undo the circular dependency. I found it hard to test so far, so its just "best effort now".

Also note the extension interface changed. There is now a separate function to set the config template. See example.

[mblythe86]

I tried d4d8dcf, and got an error. The next commit without the error is 7f65dfd, and extension configuration is still broken. When starting from an empty config, the admin page doesn't populate anything. When starting with a known-good config, the admin page shows the extension location, and that's it:

It also doesn't seem to make the config available to the extension:

pigallery2_dev  | 3/31/2024, 6:58:52 AM[SILLY][Extension][pigallery2-extension-digikam-connector] extension.config.getConfig(): undefined

Can you re-open this? Or should I open a new issue?

[bpatrik]

Ahh annoying.

I got quiet busy with life until like July, but I'll try to fix it.

Let's reopen this.

[bpatrik]

tagging #743 and #784 as the main issue

[bpatrik]

I tried d4d8dcf, and got an error. The next commit without the error is 7f65dfd, and extension configuration is still broken. When starting from an empty config, the admin page doesn't populate anything. When starting with a known-good config, the admin page shows the extension location, and that's it: It also doesn't seem to make the config available to the extension:

pigallery2_dev  | 3/31/2024, 6:58:52 AM[SILLY][Extension][pigallery2-extension-digikam-connector] extension.config.getConfig(): undefined

Can you re-open this? Or should I open a new issue?

@mblythe86
Did you update your extension after the commit? Setting the config template changed with that and also the extension pigallery2-extension-kit

[mblythe86]

Did you update your extension after the commit? Setting the config template changed with that and also the extension pigallery2-extension-kit

Yes, I updated my package.json to depend on "pigallery2-extension-kit": "2.0.3-edge6", and I implemented the initConfig function.

I realized today that my issue might have been that I didn't run npm install after updating package.json, but that didn't have any effect.

Just to check that my initConfig function is getting called at all, I added a console.log(), but I don't see that printing out anywhere.

I also tried the sample extension. The first issue I hit with that was this:

> npm run build

> [email protected] build
> tsc

server.ts:19:36 - error TS2307: Cannot find module 'pigallery2-extension-kit/src/backend/model/extension/IExtension' or its corresponding type declarations.

19 import {IExtensionConfigInit} from 'pigallery2-extension-kit/src/backend/model/extension/IExtension';
                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error in server.ts:19

After fixing that locally, it still wouldn't load the extension. In fact, it didn't even recognize that a new extension had been added at all!

[mblythe86]

I have done some debug, and I've made some local changes that fix most of the problems. I'll file another pull request soon.

The one thing I haven't been able to fix is the case of adding an extension to an existing install (i.e. when config.json already exists). This seems to be a limitation of typeconfig. I'll raise an issue there describing the problem, and you can evaluate whether it's something that should be fixed or not.

[bpatrik]

I made #885 (also some commits to typeconfig) that brings us one step closer to the solution

[bpatrik]

The last commit should (mostly) fix this issue.

There are still 2 outstanding TODOs to fully fix it:

1. If the extension order in the config changes (like you install a new one), that will mess things up:
   

   pigallery2/src/backend/model/extension/ExtensionConfigTemplateLoader.ts

   Lines 112 to 114 in 055862f

   	// TODO: this does not hold if the order of the extensions mixes up.
   	// TODO: experiment with a map instead of an array
   	config.Extensions.extensions.push(c);

2. If the json config does not contain a extension, it wont load it (basiacally the empty or partial array, overrides the given config):
   

   pigallery2/src/backend/model/extension/ExtensionConfigWrapper.ts

   Lines 23 to 30 in 055862f

   	// TODO: remove this once typeconfig is fixed and can properly load defaults in arrays
   	if (!fs.existsSync((pc.__options as ConfigClassOptions<ServerConfig>).configPath)) {
   	await pc.save();
   	}
   	await pc.load(); // loading the basic configs, but we do not know the extension config hierarchy yet
   	// TODO make sure that all extensions are present even after loading them from file

+1 there is some issue with the default values if the config is not loaded form JSON. And default values can mess up for extension.

Default values are only used for making the UI bold blue, so not a big issue.

Solving 1. by using an Map instead of an array, should also solve 2.

[bpatrik]

I had to change the extension interface again:

config has now a separate config.js file to set it up. (I had to separate it, so just setting up the config does not need running npm install an all extensions. This would have delayed the app startup).

See sample extension.

[bpatrik]

@mblythe86 Made some comments here and also at one of your PR and your comment in the other repo.
Sorry, but I might end up rejecting most of your PR. :/

The not-so-technical root cause of the whole issue is that:

1. typeconfig is overcomplicated. If it works its awesome, because it validates input config and provides a lot of metadata to the UI. (like the default value of the config, so the UI can show if it has been changed)
2. seting up config is the least fun of this project, so it hard to find motivation to working on it
3. setting up this extension config takes waay more time than I expected (at lest 3-4 month more)
4. I'm getting overwhelmed in private life, so I just have less time recently to this project.
   1. and typeconfig is a complex, generic lib, that needs significant focus time to untangle all the dependencies.

[mblythe86]

Sorry, but I might end up rejecting most of your PR. :/

No worries! It sounded from a previous comment like you were pretty busy in your private life, so I was just trying to do what I could.

If you have an idea about what still needs to change in typeconfig and/or pigallery2 to get this fully working, please share. I'm happy to learn more about the codebase and give it a try. It sounds like the GenericConfig type in typeconfig is the direction you want to go?

[bpatrik]

I think now extension config is more or less working for majority of the cases.

To fix the outstanding issues, I think the best approach would be to actually use a Map like object then a array. But as I mentioned in typeconfig that is not really supported, so just changing to it might not work out of the box.

If that is the case, I plan to add a "addNewProperty" to the generic configtype that does all the magic.

[bpatrik]

The latest commit should fix this issue. Please confirm.

[mblythe86]

I still need this tweak in order for pigallery2 to find my extensions at all:

> git diff
diff --git a/src/common/config/private/Config.ts b/src/common/config/private/Config.ts
index 7a14e5c0..1c1ccc85 100644
--- a/src/common/config/private/Config.ts
+++ b/src/common/config/private/Config.ts
@@ -10,6 +10,6 @@ try {
   pre.loadSync({preventSaving: true});
 } catch (e) { /* empty */
 }
-ExtensionConfigTemplateLoader.Instance.init(path.join(__dirname, '/../../../../', pre.Extensions.folder));
+ExtensionConfigTemplateLoader.Instance.init(pre.Extensions.folder);
 
 export const Config = ExtensionConfigWrapper.originalSync(true);

I'm not sure if this is due to some oddity in how I'm building my Docker image for testing (I'm using a slightly modified version of docker/debian-buster/selfcontained/Dockerfile) or if it's a real bug.

Aside from that, it's working well! Naturally, my settings didn't transfer from a previous config.json where extensions were still in an array, but once I made some changes to the values and saved, it updated config.json to the new map-based structure.

I also tested with a non-existent config.json file, and it correctly creates the file without any problem.

Also, I tested adding & removing extensions, and it works as expected - showing the default values for any newly-added extensions, not showing settings for deleted extensions, and preserving values for unchanged extensions.

[mblythe86]

I'm not sure if this is due to some oddity in how I'm building my Docker image for testing (I'm using a slightly modified version of docker/debian-buster/selfcontained/Dockerfile) or if it's a real bug.

I think it's a real bug. I just pulled the bpatrik/pigallery2:edge-debian-bullseye image from hub.docker.com, and it has the same issue. I was able to work around it by duplicating my config volume:

    volumes:
      - "./pigallery2/config:/app/data/config"
      - "./pigallery2/config:/app/app/data/config" # This one is necessary to work around the issue

I'll submit a pull request with the fix from my previous comment.

[bpatrik]

Can you try with 278eb86?

copying from the #897 review:

yeah so the docker used absolute path, while my test setup used a relative one.
Your PR fixes it for absolute path (eg the docker build), but breaks it for absolute.
My last commit should fix it for both.