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

Add an overload to the exec method with _Executable statement for update and delete statements #909

Open
joachimhuet opened this issue Apr 26, 2024 · 5 comments

Comments

@joachimhuet
Copy link

I think we should add an overload to the exec method to still have the possibility of passing an _Executable statement:

    @overload
    def exec(
        self,
        statement: _Executable,
        *,
        params: Optional[Union[Mapping[str, Any], Sequence[Mapping[str, Any]]]] = None,
        execution_options: Mapping[str, Any] = util.EMPTY_DICT,
        bind_arguments: Optional[Dict[str, Any]] = None,
        _parent_execute_state: Optional[Any] = None,
        _add_event: Optional[Any] = None,
    ) -> TupleResult[_TSelectParam]:
        ...

Originally posted by @joachimhuet in #831 (comment)

@federicober
Copy link

This is also valid for insert statements

@viaSeunghyun
Copy link

viaSeunghyun commented Jul 10, 2024

I agree.

The exec() method of Session allows Executable as a parameter for the statement, as shown in the code block below.

Yes, Session's method exec() has Executable as its type.

class Session(_Session):
    def exec(
        self,
        statement: Union[
            Select[_TSelectParam],
            SelectOfScalar[_TSelectParam],
            Executable[_TSelectParam],            # Here
        ],
        *,
        params: Optional[Union[Mapping[str, Any], Sequence[Mapping[str, Any]]]] = None,
        execution_options: Mapping[str, Any] = util.EMPTY_DICT,
        bind_arguments: Optional[Dict[str, Any]] = None,
        _parent_execute_state: Optional[Any] = None,
        _add_event: Optional[Any] = None,
    ) -> Union[TupleResult[_TSelectParam], ScalarResult[_TSelectParam]]:

When using direct SQL statements, especially when preparing user input, it needs to be wrapped with the text() function.
The text() returns a TextClause, which inherits from Executable through multiple inheritance.

To safely perform SQL queries, you need to wrap them in text(),

@_document_text_coercion("text", ":func:`.text`", ":paramref:`.text.text`")
def text(text: str) -> TextClause:   # Return Hint is TextClause
class TextClause(
    roles.DDLConstraintColumnRole,
    roles.DDLExpressionRole,
    roles.StatementOptionRole,
    roles.WhereHavingRole,
    roles.OrderByRole,
    roles.FromClauseRole,
    roles.SelectStatementRole,
    roles.InElementRole,
    Generative,
    Executable,                        # TextClause inherits Executable
    DQLDMLClauseElement,
    roles.BinaryElementRole[Any],
    inspection.Inspectable["TextClause"],
):

However, the problem arises because Executable is missing from the @overload in Session.
I also received a deprecated warning and tried to transition, but most situations where I use execute are not Select statements. Therefore, I think the current situation of annotating the existing execute() function with @deprecated and only allowing Select in the exec() function is misleading.

In other words, at least in the 0.110 version I tested, it's clearly a bad idea to display a @deprecated warning when calling the execute() method.

If only select statements are allowed, execute and exec should be considered as functions with different characteristics.
This inconsistency causes issues when working with non-Select SQL statements and limits the functionality of the Session.exec() method. It also creates confusion for users trying to follow best practices and handle deprecation warnings appropriately.

Steps to Reproduce:

  1. Attempt to use Session.exec() with a non-Select SQL statement wrapped in text().
  2. Observe the error or unexpected behavior.

Expected Behavior:
Session.exec() should accept all types of SQL statements wrapped in text(), as TextClause inherits from Executable.

Actual Behavior:
Session.exec() fails or produces unexpected results with non-Select SQL statements, despite them being valid Executable objects.

I've been checking and testing this and it does indeed output an error.

  • Exception Type: AttributeError
  • Exception.e: 'Session' object has no attribute 'exec'

Additional Notes:
This issue not only affects current usage but also complicates the transition process for users responding to deprecation warnings. It's important to maintain consistency in function behavior and support for different SQL statement types.

@sandangel
Copy link

I run into this issue to. Do we have any update?

@viaSeunghyun
Copy link

viaSeunghyun commented Jul 22, 2024

@sandangel
Unofficially, don't use exec, ignore the warning, use execute.
This is the cleanest way to do it without customising the FastAPI, otherwise a new @overload would need to be added in the new version.

@jtseven
Copy link

jtseven commented Aug 13, 2024

@sandangel Unofficially, don't use exec, ignore the warning, use execute. This is the cleanest way to do it without customising the FastAPI, otherwise a new @overload would need to be added in the new version.

But execute is marked as deprecated by the type checker. It says:

You probably want to use session.exec() instead of session.execute().

This is the original SQLAlchemy session.execute() method that returns objects of type Row, and that you have to call scalars() to get the model objects.

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

5 participants