ovsx-client: support multiple registries #12040

paul-marechal · 2023-01-04T22:13:21Z

Refactors the OVSXClient into a simple interface that can easily be implemented by components such as OVSXHttpClient or others.

Add a new OVSXRouterClient that may route OpenVSX queries to different registries based on an OVSXRouteConfig configuration. The path to the configuration file can be specified when starting the backend using the --ovsx-router-config CLI param. When this configuration is not specified, we default to the previously configured OpenVSX registry.

The goal of these changes was to minimally impact Theia's plugin code base by simply replacing the client interfaces we use to communicate with an OpenVSX registry. This proved more difficult than expected, especially when merging search results: pagination may currently be broken when dealing with multiple registries.

Add a sample mock OpenVSX server running as part of Theia's backend. You can find the plugins served by this mock server under examples/api-samples/plugins-db. This mock is there to allow easier testing of the multi-registry feature.

How to test

Starting the backend without the --ovsx-router-config CLI argument should just query open-vsx.org for plugins.
Search extensions for sample-namespace: It should only emit a request to the mock OpenVSX server and show its results
Search for sample-namespac: It should emit the request to both (mock, openvsx) registries.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

paul-marechal · 2023-01-04T22:34:46Z

While there's benefits to having our main OpenVSX client do the routing between multiple registries, the "biggest" problem ended up to find a way to properly merge the search results, as the order somewhat matters. In its current shape this PR doesn't do a great merging job: I just interleave results from each registries. In most situations this is less than perfect, but I'm not sure what we should do.

Surprisingly, it doesn't seem to be that big of a deal in practice. Using the mock OpenVSX server it seems relatively easy to list plugins from both registries, and the more specific your search term is and the more relevant the list seems to become.

Tell me what you think of the current state of this feature, and please suggest improvements if you have any!

paul-marechal · 2023-01-04T22:39:22Z

While this could serve us for now, I think Theia's plugin system needs to be refactored in a more plugin-type-agnostic way while supporting multiple plugin providers. The extension view would list extensions based on the provider, as this will avoid the issues I had here when trying to find a way to merge everything into a single meaningful list.

This can be done later as it will be much more involving.

msujew · 2023-03-07T14:45:32Z

Note to self (and Paul): I'm looking at this today 👍

msujew

The change looks mostly good to me. What I couldn't figure out was whether the merging of results deals with duplicate entries (so multiple registries yielding the same extension) as expected. Would the extension view should them as duplicates or is only one shown?

dev-packages/ovsx-client/src/ovsx-api-filter.ts

MatthewKhouzam · 2023-04-17T18:48:16Z

I have tested this, it brings value to Theia. The order of plug-ins is not logical, but this is an issue that I can live with. Patches are welcome on that.

msujew · 2023-04-18T10:05:49Z

I'm fine with the change, but I still have an open question.

paul-marechal · 2023-04-18T14:03:23Z

@msujew Application makers have to make sure to write their configuration in such a way that there are no duplicates returned by the router client. I don't know what the @theia/vsx-registry would do if the client were to return duplicates.

MatthewKhouzam · 2023-04-19T14:57:31Z

I am dragging @ghuntley in... This is something that may be of interest for https://github.com/coder/code-marketplace

Also vscode has the similar issue 21839

kkistm · 2023-05-10T15:50:48Z

dev-packages/ovsx-client/README.md

+        // Filters applied to the request before being issued to the registry/ies.
+        "requests": [
+            {
+                "someConditionName": "condition expression",


It will be nice to have a description of supported conditions.

dev-packages/ovsx-client/src/ovsx-types.ts

packages/vsx-registry/src/common/vsx-environment.ts

packages/vsx-registry/src/node/vsx-cli.ts

packages/vsx-registry/src/node/vsx-extension-resolver.ts

kkistm · 2023-05-16T12:26:00Z

packages/vsx-registry/src/browser/vsx-extension.tsx

+        const registryUri = new URI(await this.registryUri);
+        if (this.downloadUrl) {
+            const downloadUri = new URI(this.downloadUrl);
+            if (downloadUri.authority !== registryUri.authority) {


This is not clear for me, sorry. Consider a case when two repositories are configured, one which URL is the same as configured registryUri, and the other repository is from different URL. All the extensions for the second registry will generate the error on this method, and it is not clear why error should be generated for them.

kkistm · 2023-05-16T12:30:14Z

packages/vsx-registry/src/browser/vsx-extensions-contribution.ts

@@ -48,7 +48,8 @@ export class VSXExtensionsContribution extends AbstractViewContribution<VSXExten
    @inject(LabelProvider) protected readonly labelProvider: LabelProvider;
    @inject(ClipboardService) protected readonly clipboardService: ClipboardService;
    @inject(PreferenceService) protected readonly preferenceService: PreferenceService;
-    @inject(OVSXClientProvider) protected readonly clientProvider: OVSXClientProvider;
+    @inject(OVSXClientProvider) protected clientProvider: OVSXClientProvider;


It is not clear why clientProvider and vsxApiFilter are not read-only. Are they going to be changed somewhere?

I somewhat accidentally removed the readonly modifier as I was dealing with these fields.

FWIW I think it's a mistake to make injected fields readonly. I don't see how it prevents any kind of issue. It actually prevents manual field injection when testing as we cannot assign values to the readonly fields.

I'll remove the readonly modifiers on injected fields in this class to make it more consistent then.

As an old server-side developer I would prefer to have readonly everywhere it could be placed :) I would propose to do vice versa, to add readonly and configure the tests via tiny Inversify snippets. What do you think about it?

Not to be mean but I don't see how being a "server-side dev" is relevant to this? I consider myself a code rat and I might care too much about code quality but I still think that the readonly keyword is wrong when used for injected fields. It doesn't fix any kind of issue and actually prevent valid re-assignments by downstream users should they want to override those fields with custom logic.

I like immutability as much as the next functional dev but classes as we're using it are meant to be mutable, even more so with the property injection pattern.

If you're still not convinced then I can add readonly everywhere even if I disagree.

To add to my case, consider the following snippet:

interface MyImmutableService { readonly fieldA: object readonly fieldB: object someMethod(): void otherMethod(): void } declare const service: MyImmutableService service.fieldA = {} // error as expected service.someMethod = () => {} // TS is fine here!

If you want the methods to be readonly too you need to write things like:

interface MyImmutableService { readonly fieldA: object readonly fieldB: object readonly someMethod: () => void readonly otherMethod: () => void }

But at this point you really denature service interface definition...

Long story short: I don't think we should care about making readonly fields in services while it may still make sense for "data-objects" that may be returned and passed around by APIs.

I wrote about server-side for exact reasons you mentioned above: more immutability is better. If something could be made immutable, or at least declared immutable, I consider it is good. readonly, as far as I understand, should prevent somebody from assigning a value to a field accidentally in the code. And also it says to the reader "this field is not supposed to be changed after it is assigned". But please take all my arguments with a grain of salt, my Theia and TS experience should be considered as basic at this stage.
And just as a side note, for me the ability to create an instance of interface and start assigning its fields is quite crazy :) I understand why it works like this, but still...
As for readonly for DI fields, I would propose to make it a bit wider topic than a particular review. If there are any coding guidelines for Theia, what do they say about it? If nothing, I would propose to decide in a group (next meeting, maybe?) and then make it a guideline. And then write new code and rework old one, when there is an opportunity, according to the guideline. What do you think about it?

dev-packages/cli/src/theia.ts

kkistm · 2023-05-16T13:39:37Z

About presenting the results of search from several repositories, why not to add a feature to a repository to expose some metadata about itself? For example, a display name and icon. Then the extension list could show an icon of the repository it originates from as a decoration in some corner or something. A sorting of the results according to the repository of the origin could also be possible. OpenVSX-style repository could implement it as /about endpoint or something like this.
What do you think about it?

kkistm

LGTM
The question about readonly could be addressed separately, I think.

msujew

Alright, looks good to me 👍 Don't forget to rebase before merging :)

packages/vsx-registry/src/browser/vsx-extensions-model.ts

Refactors the `OVSXClient` into a simple interface that can easily be implemented by components such as `OVSXHttpClient` or others. Add a new `OVSXRouterClient` that may route OpenVSX queries to different registries based on an `OVSXRouteConfig` configuration. The path to the configuration file can be specified when starting the backend using the `--ovsx-router-config` CLI param. When this configuration is not specified, we default to the previously configured OpenVSX registry. The goal of these changes was to minimally impact Theia's plugin code base by simply replacing the client interfaces we use to communicate with an OpenVSX registry. This proved more difficult than expected, especially when merging search results: pagination may currently be broken when dealing with multiple registries. Add a sample mock OpenVSX server running as part of Theia's backend. You can find the plugins served by this mock server under `examples/api-samples/plugins-db`. This mock is there to allow easier testing of the multi-registry feature.

paul-marechal force-pushed the mp/ovsx-router-client branch 2 times, most recently from 47afd7a to 88e688f Compare January 4, 2023 22:24

paul-marechal force-pushed the mp/ovsx-router-client branch from 46c0e0d to d8793f9 Compare January 6, 2023 11:07

paul-marechal marked this pull request as ready for review January 6, 2023 11:07

msujew self-requested a review January 9, 2023 15:08

vince-fugnitto added proposal feature proposals (potential future features) vsx-registry Issues related to Open VSX Registry Integration labels Jan 11, 2023

msujew reviewed Mar 8, 2023

View reviewed changes

dev-packages/ovsx-client/src/ovsx-api-filter.ts Outdated Show resolved Hide resolved

paul-marechal force-pushed the mp/ovsx-router-client branch from d8793f9 to ff90526 Compare April 13, 2023 19:50

paul-marechal force-pushed the mp/ovsx-router-client branch from ff90526 to 2f04717 Compare April 19, 2023 19:22

vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023

kkistm reviewed May 16, 2023

View reviewed changes

paul-marechal force-pushed the mp/ovsx-router-client branch from 2f04717 to d35da04 Compare May 18, 2023 02:03

MatthewKhouzam mentioned this pull request May 25, 2023

Can I configure a private and an open vsx marketplace? EclipseFdn/open-vsx.org#1849

Closed

kkistm approved these changes Jun 5, 2023

View reviewed changes

paul-marechal force-pushed the mp/ovsx-router-client branch from d35da04 to d668563 Compare June 8, 2023 20:51

msujew approved these changes Jun 13, 2023

View reviewed changes

packages/vsx-registry/src/browser/vsx-extensions-model.ts Outdated Show resolved Hide resolved

paul-marechal force-pushed the mp/ovsx-router-client branch 4 times, most recently from 42c2e18 to a9394a8 Compare June 15, 2023 15:32

paul-marechal force-pushed the mp/ovsx-router-client branch from a9394a8 to 7fea930 Compare June 15, 2023 16:05

paul-marechal merged commit 8831c9d into master Jun 15, 2023

paul-marechal deleted the mp/ovsx-router-client branch June 15, 2023 17:24

github-actions bot added this to the 1.39.0 milestone Jun 15, 2023

paul-marechal mentioned this pull request Jun 30, 2023

It should be possible to configure multiple OpenVSX registries as VSX source #11876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ovsx-client: support multiple registries #12040

ovsx-client: support multiple registries #12040

paul-marechal commented Jan 4, 2023

paul-marechal commented Jan 4, 2023

paul-marechal commented Jan 4, 2023 •

edited

Loading

msujew commented Mar 7, 2023

msujew left a comment

MatthewKhouzam commented Apr 17, 2023

msujew commented Apr 18, 2023

paul-marechal commented Apr 18, 2023

MatthewKhouzam commented Apr 19, 2023

kkistm May 10, 2023

kkistm May 16, 2023

kkistm May 16, 2023

paul-marechal May 17, 2023

kkistm May 23, 2023

paul-marechal May 23, 2023

paul-marechal May 23, 2023

kkistm May 24, 2023

kkistm commented May 16, 2023

kkistm left a comment

msujew left a comment

ovsx-client: support multiple registries #12040

ovsx-client: support multiple registries #12040

Conversation

paul-marechal commented Jan 4, 2023

How to test

Review checklist

Reminder for reviewers

paul-marechal commented Jan 4, 2023

paul-marechal commented Jan 4, 2023 • edited Loading

msujew commented Mar 7, 2023

msujew left a comment

Choose a reason for hiding this comment

MatthewKhouzam commented Apr 17, 2023

msujew commented Apr 18, 2023

paul-marechal commented Apr 18, 2023

MatthewKhouzam commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkistm commented May 16, 2023

kkistm left a comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

paul-marechal commented Jan 4, 2023 •

edited

Loading