cheshire-cat-ai / plugins-backend

The backend of The Cheshire Cat plugin directory
GNU General Public License v3.0
7 stars 5 forks source link

Implement stringent validation for individual plugin.json fields #16

Closed EugenioPetulla closed 1 year ago

EugenioPetulla commented 1 year ago

Currently, the fields from various plugin.json files are all taken and included in the response of the different endpoints. This behavior, besides being risky, also leaves room for unintended errors on the part of developers.

We need to devise a method to validate the fields of the various plugins and only display those that are valid. Moreover, we should also consider returning only the plugins that have all the necessary minimal fields (name, description, url, author, tags, thumb, and version).

A mechanism should also be designed to notify any validation errors, perhaps by saving and making a log available through a designated endpoint. In this regard, we could also provide an endpoint to assist plugin authors in automatically validating their plugin.json files through actions (e.g., /validate or /validation).

EugenioPetulla commented 1 year ago

🧠 ⚡ A brainstorming about this topic with someone of you with different experiences will be appreciated @nicola-corbellini @pieroit @Mte90 @zAlweNy26

Mte90 commented 1 year ago

To me all those fields except the thumbnail should be mandatory. JSON validation I am not sure if it is something that should be a script that developers uses for packaging a plugin etc in this way the core it will be free from this things.

zAlweNy26 commented 1 year ago

I agree with Daniele, except that for me also the plugin URL and author URL should not be mandatory. For the validation, I think we should find another way to do that, so without needing a specific endpoint for it. Sorry, but I don't have any idea at the moment about that.

nicola-corbellini commented 1 year ago

I agree with @Mte90 too, all mandatory except the thumbnail. Maybe also tags could be sacrificed despite I think they could be useful.

W.r.t. the validation I like the idea, but honestly I don't have a clear understanding of pros and cons of all the possibilities (i.e. why an endpoint is a good/bad idea, rather than something else). Naively, I'd run a validation script in a GitHub workflow, when devs open a PR to add their plugin to the registry. The same script could be added to the template-repo workflows, if they are using it. Not sure if this makes sense or I'm missing the point.

zAlweNy26 commented 1 year ago

I like your idea about the validation @nicola-corbellini

pieroit commented 1 year ago

About the validation, offering an endpoint here is a good idea and also have an automatic workflow (the first to check validity before submitting, the second after submitting). Maybe an endpoint is more versatile as we'll have a function that can be run from devs externally and also from the registry internally.

For the plugin.json I propose to ask only what is necessary ( name and url ), and impose fields only when there will be strong competition to be on the registry. When registry plugins are going to be shown in the admin, devs will see they have no thumbnail and no description, and will di propria sponte update their plugins.

Having a curated plugin.json is something in the direct interest of a plugin dev, not something we should impose.

Avoid 1) bureaucracy and 2) forcing people to read things, at all costs. Let's welcome experiments! :partying_face:

EugenioPetulla commented 1 year ago

I have to reopen this thread.

We have the "Author" endpoint that filters plugins based on authors and I think it's one of those mandatory field I was talking about.

Can I make at least the author field as required along with title and url?

pieroit commented 1 year ago

Can I make at least the author field as required along with title and url?

Agree, both for the engineering reason and so people has someone to sue in case of explosions XD

EugenioPetulla commented 1 year ago

@zAlweNy26 what happens if there is no thumbnail url in the plugin.json but the key exists (like if it's just generated by our template? Do we fallback to some standard image?

Eg.

{
...
"thumb": "",
...
}
zAlweNy26 commented 1 year ago

@EugenioPetulla the frontend fallbacks to the first letter of the plugin name as thumbnail

EugenioPetulla commented 1 year ago

But that's not happening if the image is broken and returns a 404, right? Maybe we should implement this check if we don't validate the plugin.json prior :(

zAlweNy26 commented 1 year ago

Yeah, I only check if the string is not empty or undefined, without validating it. Plugin dev responsability imho

EugenioPetulla commented 1 year ago

but we are not validating it because we don't want to put pressure on them so we have to make stronger fallback or the risk is that some developer make an error and we have a broken image in the official repository. Not good UX :(

I can write a typescript image check function in less than 5 lines if you want!

function checkImageURL(url: string, callback: (isBroken: boolean) => void) {
  const img = new Image();
  img.onload = function() { callback(false);  };
  img.onerror = function() { callback(true);  };
  img.src = url;
}
zAlweNy26 commented 1 year ago

If you want that this way, ok then, I'll add it :)

EugenioPetulla commented 1 year ago

Yup, I will love to see this kind of validation 🤟🏼

If isBroken -> your fallback with the first letter If not -> use the image

Easy peasy 👯

zAlweNy26 commented 1 year ago

Fixed image validation now on admin-vue main branch :)