Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create an endpoint for listing available ORT plugins #1640

Open
sschuberth opened this issue Dec 13, 2024 · 10 comments
Open

Create an endpoint for listing available ORT plugins #1640

sschuberth opened this issue Dec 13, 2024 · 10 comments
Labels
api Issues related to the API. enhancement New feature or request.

Comments

@sschuberth
Copy link
Contributor

sschuberth commented Dec 13, 2024

In order to dynamically display available ORT plugins and their options in the UI, the API needs an endpoint to list these. Either there should be different (nested) endpoints for the different types of plugins (like /plugins/reporter), or a single endpoint needs to take some query parameter to request e.g. only reporter plugins (like /plugins?tool=reporter).

I believe it makes sense to start with this task even if not all ORT plugins are migrated to KSP yet. Esp. for reporters this API would already be very useful in order to display all reporter-specific options to the user, which otherwise are hard to remember and error-prone to specify.

@sschuberth sschuberth added enhancement New feature or request. api Issues related to the API. labels Dec 13, 2024
@mnonnenmacher
Copy link
Contributor

I wonder, is it even required to request the plugin types separately or would one endpoint be sufficient that returns all plugin information? Because the trigger run page needs all of them anyway.

@sschuberth
Copy link
Contributor Author

Does the plugin information contain for which ORT tool the plugin is? If yes, we could also filter by post-processing. Anyway, I would not want to rule out that filtering by ORT tool could be required somewhere at some point. So that's probably an argument for using a single / non-nested API endpoint, first without a query option, but add it later if needed.

@mnonnenmacher
Copy link
Contributor

So that's probably an argument for using a single / non-nested API endpoint, first without a query option, but add it later if needed.

That sounds good to me, a single endpoint is less work and adding filtering should be easy. The plugin descriptor has a property like this:

"factoryClass": "org.ossreviewtoolkit.advisor.AdviceProviderFactory"

So we could either return a list and filter based on that, or we could group them in the endpoint with custom more readable keys, for example:

{
  "advisors": [...],
  "package-managers": [...]
}

Any preference?

@mnonnenmacher
Copy link
Contributor

For the response itself I would just use the PluginDescriptor object, for example for the OSV advisor:

{
    "descriptor": {
        "id": "OSV",
        "displayName": "OSV",
        "description": "An advisor that retrieves vulnerability information from the Open Source Vulnerabilities database.",
        "options": [
            {
                "name": "serverUrl",
                "type": "STRING",
                "description": "The base URL of the OSV REST API. If undefined, default is the production endpoint of the official OSV.dev API.",
                "default": "https://api.osv.dev",
                "aliases": [],
                "isRequired": false
            }
        ]
    },
    "configClass": "org.ossreviewtoolkit.plugins.advisors.osv.OsvConfiguration",
    "factoryClass": "org.ossreviewtoolkit.advisor.AdviceProviderFactory"
}

We could consider leaving out the configClass and factoryClass values as implementation details.

@sschuberth
Copy link
Contributor Author

Any preference?

Hmm. I'd vote for a solution that avoids to hard-code any mappings. I.e. IMO we should not have code that manually maps "org.ossreviewtoolkit.advisor.AdviceProviderFactory" to "advisors" just to perform some grouping. Instead, maybe we could just use the direct parent package of the factory class for grouping? However, that would group package manager under "analyzer" instead of "package-managers". So maybe rather use the factory class's name without the "Factory" for grouping?

But then again, if thinking about grouping already causes hassle, maybe indeed do not group at all in the API, but leave filtering to the front-end. What do you think @mmurto?

@mmurto
Copy link
Contributor

mmurto commented Dec 14, 2024

Generally I think that we should hand-craft opinionated forms instead of dynamically creating the forms based on the available plugins. I think that would allow for more polished UX instead of just exposing ORT over the browser. The example OSV configuration is a good example - IMO configuring which OSV endpoint the run uses should not be configurable for runs but it is for the instance administrator to configure, likely with environment variables.

If you want to go this route, I think it would be best for the endpoint to return all relevant information that's needed to build the form. Some additional information that comes to mind would be at least maybe a description for the suggested grouping, so the form doesn't just show title "Advice Provider" under which there's a toggle to enable or disable "OSV" - the "Advice Provider" et al likely needs some explanation as well. Another useful piece of information would likely also be the order of plugins/groupings so they're not show in a random order in the form.

Maybe it makes sense to start with creating a rough mock-up of the forms you want to achieve with this to figure out what data needs to be returned to display the form.

@sschuberth
Copy link
Contributor Author

[...] instead of just exposing ORT over the browser. [...] it is for the instance administrator to configure

I basically agree to this as a mid-term goal. However, for the short term I'm currently "blocked" by not being able to enable some reports that I need via the UI. The quickest way to elegantly resolve this is by dynamically creating the forms. To that logic later some filter could be applied, configured at the administrator level.

@mmurto
Copy link
Contributor

mmurto commented Dec 14, 2024

[...] instead of just exposing ORT over the browser. [...] it is for the instance administrator to configure

I basically agree to this as a mid-term goal. However, for the short term I'm currently "blocked" by not being able to enable some reports that I need via the UI. The quickest way to elegantly resolve this is by dynamically creating the forms. To that logic later some filter could be applied, configured at the administrator level.

It shouldn't take too many lines of code to add the fields that you're missing to the forms.

But looking forward to the implementation! I've never seen or done dynamic forms myself, but quick googling reveals stuff like this. You could also think whether something like JSON schema would be a good data format to return from the API, if there are some libraries that would make form creation easier that way.

@sschuberth
Copy link
Contributor Author

It shouldn't take too many lines of code to add the fields that you're missing to the forms.

True, but I decided against hard-coding yet another format just because I happen to need it now, as I believe that to be wrong approach.

@mnonnenmacher
Copy link
Contributor

mnonnenmacher commented Dec 14, 2024

Generally I think that we should hand-craft opinionated forms instead of dynamically creating the forms based on the available plugins. I think that would allow for more polished UX instead of just exposing ORT over the browser.

That's true, but users could add custom ORT plugins to the server and the UI should support configuring them as well, in this case the forms need to be generated. If the plugin information is not sufficient to build nice forms, we can also extend it (for example, currently it does not support enum types for plugin options which still needs to be implemented).

IMO configuring which OSV endpoint the run uses should not be configurable for runs but it is for the instance administrator to configure

That's covered by #501, but designing this feature will take some time, in the meantime I think generated forms will help a lot, and they will propably be reusable when implementing #501.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API. enhancement New feature or request.
Projects
None yet
Development

No branches or pull requests

3 participants