bump-pydantic icon indicating copy to clipboard operation
bump-pydantic copied to clipboard

Potential Inconsistency with Inner Config Migration

Open acostapazo opened this issue 1 year ago • 2 comments

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.

https://github.com/alice-biometrics/petisco-fastapi-example/pull/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.

acostapazo avatar Jul 04 '23 06:07 acostapazo

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 avatar Jul 04 '23 10:07 Kludex

I'll fix this soon.

Kludex avatar Jul 05 '23 17:07 Kludex