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: Validators on an inherited field cause a bug #19

Open
awgymer opened this issue Jan 16, 2025 · 1 comment
Open

bug: Validators on an inherited field cause a bug #19

awgymer opened this issue Jan 16, 2025 · 1 comment
Assignees
Labels
unconfirmed This bug was not reproduced yet

Comments

@awgymer
Copy link

awgymer commented Jan 16, 2025

Description of the bug

I believe if you create a validator method on a field called id that it will cause an error:

.../lib/python3.10/site-packages/griffe_pydantic/common.py", line 72, in <listcomp>
    targets = [cls.members[field] for field in fields]
KeyError: 'id'

Docs were building fine until I added the following validator to my model:

@field_validator("id", mode="before")
@classmethod
def convert_null_id_to_empty(cls, v: Any) -> Any:
    return v if v is not None else ""

Since the cls.members is coming from a griffe.Class object I'm not sure if this is a bug there rather than in this extension, but I suspect that it's much more likely to crop up here as typically you would avoid using id as an object attribute.

UPDATE: I loaded in my package using griffe in my interpreter and I found the real cause of the bug. It seems that the field doesn't exist on the Class object because I guess griffe does not include the inherited attributes from the base class (where id is defined)?

So I think MRE:

from typing import Any
from pydantic import BaseModel, field_validator

class MyBase(BaseModel):
    id: str


class AllowNones(MyBase):

    @field_validator("id", mode="before")
    @classmethod
    def convert_null_id_to_empty(cls, v: Any) -> Any:
        return v if v is not None else ""

I would guess a fix might be to use cls.all_members?

@awgymer awgymer added the unconfirmed This bug was not reproduced yet label Jan 16, 2025
@awgymer awgymer changed the title bug: Validators on an id field cause an error bug: Validators on an inherited field cause a bug Jan 16, 2025
@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

Thanks for the report @awgymer 🙂

I would guess a fix might be to use cls.all_members?

Yep, seems like the solution!

However! During dynamic analysis, common.process_function will run while the package is not entirely loaded. Using all_members would be bad: it computes inherited members, and if Griffe didn't yet see the base classes, some inherited members will be missing, and we'll likely get a key error again. If we want to use all_members safely, then we have to refactor the extension to use on_package_loaded for dynamic analysis as well. Which means the current hooks (on_class_instance, on_attribute_instance, on_function_instance, on_class_members) should only record the objects somewhere (in a dict, self._objects[obj.path] = obj). Then we would iterate on those in on_package_loaded. Something like that 😅

mkdocstrings/griffe#347 could alleviate the need to wait for the whole package to be loaded, but this is a consequent effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unconfirmed This bug was not reproduced yet
Projects
None yet
Development

No branches or pull requests

2 participants