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

feat: valid Pydantic #373

Closed
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion core/cat/db/crud.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,7 @@ def upsert_setting_by_name(payload: models.Setting) -> models.Setting:
query = Query()
get_db().update(payload, query.name == payload.name)

return get_setting_by_name(payload.name)
return get_setting_by_name(payload.name)

def validate_presences(required: list, payload: dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the utility of a validate_presences method, but it is not related to DB crud.
I would move it here in the factory, where schemas are defined, or in utilis in case we'll use this validation method also for plugin settings. Also infrastructure could be a place for this method, if we go for a validation set of methods

What do you think?

return len([req for req in required if req not in payload]) > 0
14 changes: 12 additions & 2 deletions core/cat/routes/setting/embedder_setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def get_embedder_settings():
def upsert_embedder_setting(
request: Request,
languageEmbedderName: str,
payload: Dict = Body(example={"openai_api_key": "your-key-here"}),
payload: Dict = Body(..., example={"openai_api_key": "your-key-here"})
):
"""Upsert the Embedder setting"""

Expand All @@ -54,7 +54,17 @@ def upsert_embedder_setting(
status_code=400,
detail=f"{languageEmbedderName} not supported. Must be one of {allowed_configurations}",
)


required = EMBEDDER_SCHEMAS[languageEmbedderName]['required']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there always a required key?

if crud.validate_presences(required, payload):
raise HTTPException(
status_code=405,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's go for 400 (bad request)

detail={
"error": f"The following fields are required: {', '.join(required)}"
}
)


# create the setting and upsert it
final_setting = crud.upsert_setting_by_name(
models.Setting(name=languageEmbedderName, category=EMBEDDER_CATEGORY, value=payload)
Expand Down
19 changes: 16 additions & 3 deletions core/cat/routes/setting/llm_setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def get_llm_settings():
def upsert_llm_setting(
request: Request,
languageModelName: str,
payload: Dict = Body(example={"openai_api_key": "your-key-here"}),
payload: Dict = Body(..., example={"openai_api_key": "your-key-here"})
):
"""Upsert the Large Language Model setting"""

Expand All @@ -54,9 +54,22 @@ def upsert_llm_setting(
if languageModelName not in allowed_configurations:
raise HTTPException(
status_code=400,
detail=f"{languageModelName} not supported. Must be one of {allowed_configurations}",
detail={
"error": f"{languageModelName} not supported. Must be one of {allowed_configurations}"
}
)


# validate configuration
required = LLM_SCHEMAS[languageModelName]['required']
if crud.validate_presences(required, payload):
raise HTTPException(
status_code=405,
detail={
"error": f"The following fields are required: {', '.join(required)}"
}
)


# create the setting and upsert it
final_setting = crud.upsert_setting_by_name(
models.Setting(name=languageModelName, category=LLM_CATEGORY, value=payload)
Expand Down
14 changes: 13 additions & 1 deletion core/tests/routes/setting/test_llm_setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,16 @@ def test_upsert_llm_settings_success(client):
# assert
assert response.status_code == 200
assert json["selected_configuration"] == 'LLMCustomConfig'
assert json["settings"][0]["value"]["url"] == invented_url
assert json["settings"][0]["value"]["url"] == invented_url

def test_upsert_llm_settings_error(client):

# set a different LLM
invented_url = "https://example.com"
payload = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for writing the test

response = client.put("/settings/llm/LLMCustomConfig", json=payload)
json = response.json()

# check immediate response
assert response.status_code == 405
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's go for 400 (bad request)

assert json["detail"]["error"] == "The following fields are required: url"