Connor/eng 1914 ensure select works with all ways of querying sqlalchemy#15
Conversation
ENG-1914 Ensure `Select` works with all ways of querying `SQLAlchemy`, even ones we don't support
i.e. they can't use our |
|
Since this is basically just an extension of the above quoted PR, I'm going to hold this off in draft state until I see notes on that one! |
conniedonahue
force-pushed
the
connor/eng-1914-ensure-select-works-with-all-ways-of-querying-sqlalchemy
branch
from
68a4c01 to
d1fa03a
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds type-safe overloads for SQL execution and selection to support various query forms with the custom Select and Session.execute.
- Extends
Session.executewith overloads for 1–4 tuple returns and a fallback for other cases. - Makes
Selectgeneric and provides overloads on theselect()factory for 1–4 entities plus a varargs fallback.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/sqlalchemy_oso_cloud/session.py | Imported Result and TypedReturnsRows, added multiple execute overloads; overridden execute calls super with filtering support. |
| src/sqlalchemy_oso_cloud/select_impl.py | Made Select generic, added overloads for the select() helper and updated its varargs implementation. |
Comments suppressed due to low confidence (2)
src/sqlalchemy_oso_cloud/session.py:84
- Consider adding unit tests for each
executeoverload (1–4 columns and the fallback) to verify that authorization filtering is applied correctly across all cases.
# Execute overloads for our custom Select class
src/sqlalchemy_oso_cloud/session.py:9
- TypeVars T2, T3, and T4 are used in the
executeoverloads but not declared. AddT2 = TypeVar("T2"),T3 = TypeVar("T3"), andT4 = TypeVar("T4")at the top of the file.
T1 = TypeVar("T1")
|
|
||
| # Fallback for 5+ entities or any other Select | ||
| @overload | ||
| def execute( |
There was a problem hiding this comment.
is there a reason this is a duplicate header of the below function?
There was a problem hiding this comment.
Good catch! This is a remnant of some slimming down I did of this section. I cut out this repeat!
There was a problem hiding this comment.
Ah! I misspoke! Instead of deleting this overload, I need to adjust the statement to Any to work as a true fallback.
| When used with our custom Select class or Query class that have been filtered | ||
| with .authorized(), the authorization filtering will be applied. | ||
| """ | ||
| return super().execute( # type: ignore[misc] |
There was a problem hiding this comment.
is there a reason we can't set the execution_options type to OrmExecuteOptionsParameter = EMPTY_DICT like how it is in the super class? then we don't need the type ignores
|
i believe this pr also wraps up this ticket, but cmiiw |
| if TYPE_CHECKING: | ||
| from sqlalchemy.orm._typing import OrmExecuteOptionsParameter | ||
| from sqlalchemy.util._collections import EMPTY_DICT | ||
| else: | ||
| try: | ||
| from sqlalchemy.orm._typing import OrmExecuteOptionsParameter | ||
| from sqlalchemy.util._collections import EMPTY_DICT | ||
| except ImportError: | ||
| OrmExecuteOptionsParameter = Mapping[str, Any] # type: ignore[misc] | ||
| EMPTY_DICT = {} # type: ignore[assignment] | ||
|
|
There was a problem hiding this comment.
I'm not sure if this is the right move, but I wanted to write a little defensively around OrmExecuteOptionsParameter and EMPTY_DICT since they are both pulled from SQLAlchemy's internal collections. With this logic, if they make some sort of breaking change that causes an ImportError, we have fallbacks.
…-ways-of-querying-sqlalchemy
|
I'm just going to close this, not worth expending the time on pushing through a review. |
Essentially this adds the same sort of overrides to
select()andexecute()as those we have inQuery