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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Inconsistency with Inner Config Migration #50

Open
acostapazo opened this issue Jul 4, 2023 · 2 comments
Open

Potential Inconsistency with Inner Config Migration #50

acostapazo opened this issue Jul 4, 2023 · 2 comments

Comments

@acostapazo
Copy link

First of all, I want to express my gratitude for developing this incredible tool. It has greatly assisted me in adopting Pydantic v2, and I have achieved exceptional results in my projects. Thank you for your valuable contribution! 馃殌

I have encountered an issue where the bump-pydantic tool is exhibiting unexpected behavior by replacing the inner Config in non-Pydantic objects.

alice-biometrics/petisco-fastapi-example#11

To reproduce this behaviour you can use the following code (pip install petisco):

from meiga import BoolResult, Failure, Success
from petisco import Controller, DomainError


class IsNotPositive(DomainError):
    ...


class MyController(Controller):
    class Config:
        success_handler = lambda _: print("The result is positive")
        failure_handler = lambda _: print("The result is negative")

    def execute(self, value: int) -> BoolResult:
        if value < 0:
            return Failure(IsNotPositive())
        return Success(True)


result_success = MyController().execute(1)
result_failure = MyController().execute(-1)

print(result_success.transform())
print(result_failure.transform())

The Controller inherits from a metaclass class Controller(metaclass=MetaController) and this metaclass gets information from config

class MetaController(type, ABC):
    middlewares: list[Middleware] = []

    def __new__(
        mcs, name: str, bases: tuple[Any], namespace: dict[str, Any]
    ) -> MetaController:
        config = namespace.get("Config")

        mapper = get_mapper(bases, config)

        if "execute" not in namespace:
            raise NotImplementedError(
                "Petisco Controller must implement an execute method"
            )

        new_namespace = {}
        for attributeName, attribute in namespace.items():
            if isinstance(attribute, FunctionType) and attribute.__name__ == "execute":
                if iscoroutinefunction(attribute):
                    attribute = async_wrapper(attribute, name, config, mapper)
                else:
                    attribute = wrapper(attribute, name, config, mapper)
            new_namespace[attributeName] = attribute

        return super().__new__(mcs, name, bases, new_namespace)

    @abstractmethod
    def execute(self, *args: Any, **kwargs: Any) -> AnyResult:
        return NotImplementedMethodError

I guess bump-pydantic only have to update BaseModels objects. To ensure that it would be beneficial to introduce an additional check in the replace_config.py file.

@Kludex
Copy link
Member

Kludex commented Jul 4, 2023

Hmm... I think I actually can't transform to model_config when I have attributes that I "don't know" e.g. success_handler, and failure_handler.

I can also make sure it's a BaseModel based class, but I'd like to avoid this kind of logic - even if I already have it in place.

@Kludex
Copy link
Member

Kludex commented Jul 5, 2023

I'll fix this soon.

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

No branches or pull requests

2 participants