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

Added set support for arrays #117

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xshiki
Copy link

@xshiki xshiki commented Dec 11, 2024

No description provided.

@dokempf dokempf linked an issue Dec 11, 2024 that may be closed by this pull request
@dokempf
Copy link
Member

dokempf commented Dec 11, 2024

The code looks good! Please add a test case in tests/schemas with a valid and an invalid document.

@dokempf
Copy link
Member

dokempf commented Dec 11, 2024

Also, for the future: If you write "This fixes #107" into the PR description, GitHub will automatically link it to the issue.

@xshiki
Copy link
Author

xshiki commented Dec 12, 2024

I've noticed that set is not a standard JSON schema validation keyword but uniqueItems with the same function already exists. But changing set to unique_items throws an error in the pydantic model.

from typing import List
class ListUnique(BaseModel):
    ids: List[str] = Field(unique_items=True)

form = Form(ListUnique.model_json_schema())
form.show()
PydanticUserError: `unique_items` is removed, use `Set` instead(this feature is discussed in https://github.com/pydantic/pydantic-core/issues/296)

For further information visit https://errors.pydantic.dev/2.9/u/removed-kwargs

For the test case so far

{
  "default": [],
  "schema": {
    "description": "Bla bla",
    "items": {
      "default": "foo",
      "type": "string"
    },
    "uniqueItems": true,
    "type": "array"
  },
  "valid": [
    [
      "foo"
    ],
    [
      "1",
      "2",
      "3"
    ]
  ],
  "invalid": [
    [
      "1",
      "1",
      "3"
    ]
  ]
}

So either the pydantic model throws an error or the test case fails when I use "set": true,
What's your opinion about this?

Copy link
Member

Choose a reason for hiding this comment

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

This should not be part of the PR.

Copy link
Member

Choose a reason for hiding this comment

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

same.

result = [h.getter() for h in elements[:element_size]]
if schema.get("set", False):
if len(result) != len(set(result)):
raise FormError("Array elements are not unique")
Copy link
Member

Choose a reason for hiding this comment

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

Looking at what pydantic does, I think it would also be acceptable to make the array unique instead of throwing an error:

import pydantic
from typing import Set

class Foo(pydantic.BaseModel):
    ids: Set[str]

Foo(ids=["a", "a"])

above code outputs Foo(ids={'a'})

@@ -931,12 +931,19 @@ def _resetter():
# Initially call the resetter
_resetter()

def _getter():
result = [h.getter() for h in elements[:element_size]]
if schema.get("set", False):
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a mix-up here of set and uniqueItems. uniqueItems is deprecated in a pydantic field definition, but it is still very much in the generated JSONSchema. set on the other hand does not exist in the JSONSchema spec - but uniqueItems does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement set Support for Arrays
2 participants