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

[BUG] Missing validation on factory endpoints (LLM, embedder, auth) #767

Open
enrichman opened this issue Mar 29, 2024 · 4 comments
Open
Assignees
Labels
bug Something isn't working

Comments

@enrichman
Copy link

Describe the bug
It seems possible to create a completely empty setting (with blank name, blank category and empty value) in the POST /settings endpoint:

curl -X 'POST' \
  'http://localhost:1865/settings/' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -d '{
  "name": "",
  "value": {},
  "category": ""
}'

result:

{
  "setting": {
    "name": "",
    "value": {},
    "category": "",
    "setting_id": "aefed2ee-94ba-4975-9fd2-d37acd68794a",
    "updated_at": 1711723874
  }
}

Expected behavior
Maybe it's the expected behavior but I'm not sure if at least the name should be verified, or all the three fields.

@enrichman enrichman added the bug Something isn't working label Mar 29, 2024
@pieroit
Copy link
Member

pieroit commented Mar 29, 2024

Having a basic validation could be useful
Do you want to make a PR?
The error messages should follow this format

Thanks and welcome ;)

@enrichman
Copy link
Author

Thanks @pieroit for confirming that this is probably not expected. I'm not sure how this settings should be used. Is the name mandatory? What about category or the value?

IMO at least the name and the value should not be empty, but I don't know if also the category should be mandatory.

@pieroit
Copy link
Member

pieroit commented Mar 29, 2024

Thanks @pieroit for confirming that this is probably not expected. I'm not sure how this settings should be used. Is the name mandatory? What about category or the value?

They should be used freestyle, so we can impose a minimal validation without restricting freedom (as a framework sould do).
In the future we may add the possibility to add custom validators via hook, but too soon for that.

IMO at least the name and the value should not be empty, but I don't know if also the category should be mandatory.

let's make all of them non null/empty
Assigned ;)

I may add the tests for this if you don't feel like it, because they are long due (my bad).
BTW they should go here

@matteocacciola
Copy link

matteocacciola commented Oct 19, 2024

Simple pydantic validation rules could be added in the crud model, wdyt?

@pieroit pieroit changed the title [BUG] Missing validation on create /settings endpoint [BUG] Missing validation on factory endpoints (LLM, embedder, auth) Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants