Bad return type on the execute method

[silentworks]

Bug report

• I confirm this is a bug with Supabase, not with my own application.
• I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

This has been an issue for a long time but it didn't surface until we decided to enforce mypy for type checking in this repo. The .execute method inside of the SyncQueryRequestBuilder and AsyncQueryRequestBuilder is trying to handle too many use cases and in one (csv) of the case it's not even used anymore. The method is currently returning both a APIResponse object and just plain str.

We should extract any code that is returning a str into its own execute method in a different class. This is already done for the .csv method as it uses an execute method from the SyncSingleRequestBuilder and AsyncSingleRequestBuilder class. The same need to be done for the .explain method and it should just return a str and no objects at all.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

1. Go to '…'
2. Click on '…'
3. Scroll down to '…'
4. See error

Expected behavior

A clear and concise description of what you expected to happen.

Screenshots

If applicable, add screenshots to help explain your problem.

System information

• Version of supabase-py: 2.21.1

Additional context

Add any other context about the problem here.

[o-santi]

Agreed, I think the refactor is fairly simple.

[hnykda]

Aye, we have this issue in our prod, caused us to pin postgrest.

[o-santi]

Can you explain what do you mean by having this issue in prod? This should be just a typing issue.

[hnykda]

Sorry, I should have been more clear, it is indeed just a typo issue, we are experiencing on a production code.

We kind of had to to update supabase because there was a weird issue with postgrest (AsyncPostgrestClient.__init__() got an unexpected keyword argument 'http_client'), and to avoid this type issue we did:

    "supabase>=2.22.0",
    "postgrest>=1.0.0,<=2.0.0",

[o-santi]

Sorry, that is not the same issue. That should be because the version of the library you had before did not support the http_client parameter, so it makes sense that you had to update.

[jackwildman]

Expanding upon @hnykda's comment, we've pinned to solve two issues:

• http_client not supported in older versions (<=1.1.1), as mentioned
• breaking changes to the .execute() interface (>=2.0.0), as per (or, at least, in the spirit of) the headline issue

Specifically, 1.1.1 has:

class APIResponse(BaseModel, Generic[_ReturnT]):
    data: List[_ReturnT]

and AsyncQueryRequestBuilder.execute() returning:

async def execute(self) -> APIResponse[_ReturnT]:

while >= 2.0.0 has:

class APIResponse(BaseModel):
    data: List[JSON]

and

async def execute(self) -> APIResponse | str:

This causes us to have to handle:

• execute might returning a str
• execute's .data might be a list of None, bool, etc, rather than a plain dict

Particularly, we often simply know that we're getting list[dict] back because of how we've structured the query e.g.:

result = await supabase.table("my_table")
    .select("id, some_field")
    .execute()

where we know up-front that we're going to be dealing with lists of {"id", "some_field"}, but would, in the new scheme, have to ensure that we're checking for str responses, that we actually have lists of dict, or otherwise cast our way out of the problem.

For now, we've managed to find the sweet spot of version 1.1.1 satisfying everything, but this is clearly a bad solution long term. It sounds like the long-term solution is to simply update all usage of execute to the new return type, which is somewhat painful. I'm curious if any of the following are available/possible:

• simplify the return type (as per OP's suggestion)
• something that aids us inferring the schema of the return type
• something else I'm not aware of

(Greatly appreciate this client overall though. Thanks for all the work that's gone into it.)

[o-santi]

Thank you for your comment @jackwildman, it is very informative.

breaking changes to the .execute() interface (>=2.0.0), as per (or, at least, in the spirit of) the headline issue

Indeed, these would be breaking changes if they were concrete changes, but they're purely typing changes. What actually happened is that before merging to the monorepo, the postgrest library was unsound, in that its typing annotations were wrong, as they were largely unchecked. In order to improve the quality of the libraries, I started enforcing mypy on CI (#1231), which made me change a lot of the types that were there previously.

async def execute(self) -> APIResponse[_ReturnT]:

(the _ReturnT) and

async def execute(self) -> APIResponse | str:

(the | str part) are both examples of wrong typing definitions that were changed after running mypy. The generic part was just a lot of unsound code, in the sense that you could affirm that the result would be of any type and the type checker would accept it, as it boiled down to a json.loads returning Any. In fact, this is exactly the reason why I made all data be JSON, as it is the best thing we can say about it for now. All the generic definitions were purely useless cruft, as in the end it would return the data that it got, and do absolutely no validation on it whatsoever.

The | str part is the core of this issue specifically, and is due to the body of the main execute call being wrongly typed to ignore the cases where it returned a string, which were introduced to special case both .csv and .explain. This is going to be fixed very nearly, as I believe that the work to fix it isn't too difficult.

For now, we've managed to find the sweet spot of version 1.1.1 satisfying everything, but this is clearly a bad solution long term

Do not be fooled by the type definitions, there's virtually no changes in the code between 1.1.1 and now, it is all just typing information being correct now. I would encourage you to use the most up to date version, as at the very least you're aware of what could be going wrong and were. Also, even if you use 1.1.1, do not rely on the generics, as they do no validation whatsoever of the payload, so if your annotation is wrong it will end up causing bugs.

The intended solution for this issue is to make .explain return a different object, that has an execute method returning purely a str, simplifying the more general (main) one to a simple APIResponse.

something that aids us inferring the schema of the return type

Lastly, this is a very requested feature, and already exists for other libraries (TypeScript has it), which is generating structs for the exact schema you have, with the correct types and validations in place. This is planned to be done in the future, when all the libraries in the monorepo are up to a certain level of quality (where I'm not worried about key soundness problems hahaha). Currently, auth is still very much unchecked, as well as supabase, so these two will have a rough review in order to pass a very weak ruleset of mypy (reminder, mypy --strict is not ensured in any library as of right now, but that's a different goal).

After these are fixed, we intend on releasing a major version with very big updates and improvements, in the form of the v3.0 tagline. Feel free to comment on the tracking issue for it, #1251, specially on how these would impact your existing codebase (positively or negatively), it would be extremely helpful. These are not all that is planned, I intend to flesh out the changes more once I get to prioritizing the switch.

Finally, with all that done, I intend to start working on improving the features of the libraries, with the very first one being generating types (most likely pydantic models) from the schemas of the database for this library. With this, it would be much simpler to ensure that the code is correct, while still maintaining type correctness.