pallets-eco / flask-sqlalchemy

Adds SQLAlchemy support to Flask
https://flask-sqlalchemy.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
4.18k stars 896 forks source link

The 'db.first_or_404(db.select(User))' method cannot return None when 'User.query()' is empty. #1313

Closed YlguYtrid closed 4 months ago

YlguYtrid commented 4 months ago

I tried to rewrite the legacy query method to new api. Then I run coverage and find 'db.first_or_404(db.select(User))' still cause 404 Error.

I read the 'first_or_404' function code.It's same as:

    def first_or_404(
        self, statement: sa.sql.Select[t.Any], *, description: str | None = None
    ) -> t.Any:
        """Like :meth:`Result.scalar() <sqlalchemy.engine.Result.scalar>`, but aborts
        with a ``404 Not Found`` error instead of returning ``None``.

        :param statement: The ``select`` statement to execute.
        :param description: A custom message to show on the error page.

        .. versionadded:: 3.0
        """
        value = self.session.execute(statement).scalar()

        if value is None:
            abort(404, description=description)

        return value

I think the code is wrong, then I rewrite it as:

    def first_or_404(
        self, statement: sa.sql.Select[t.Any], *, description: str | None = None
    ) -> t.Any:
        """Like :meth:`Result.scalar() <sqlalchemy.engine.Result.scalar>`, but aborts
        with a ``404 Not Found`` error instead of returning ``None``.

        :param statement: The ``select`` statement to execute.
        :param description: A custom message to show on the error page.

        .. versionadded:: 3.0
        """
        value = self.session.execute(statement).scalar()

        if value == "<NotFound '404: Not Found'>":
            abort(404, description=description)

        return value

Now I retest coverage. All tests passed.

Environment:

davidism commented 4 months ago

I can't reproduce this issue. It sounds like you have a different error. The tests pass currently, the suggested change would break them.

YlguYtrid commented 4 months ago

I can't reproduce this issue. It sounds like you have a different error. The tests pass currently, the suggested change would break them.

I read the docstring again. I realized I made a mistake. This method is replace None to 404 error. I thought it is replace 404 error to None.