Upgraded models to SQLAlchemy 2 - #167
Conversation
Replaces 1.x Query API with 2.0 select() across integrated test files for models, services, and resource workflow steps. Production code paths unchanged; legacy .query() still works under SQLA 2.0 but is no longer the canonical pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- base.py: declarative_base() → class AppUsersBase(DeclarativeBase) - All app_users + file_database + controller_metadata models now use Mapped[T] annotations with mapped_column(...) instead of Column(...). - backref= replaced with explicit back_populates= on both sides; reverse relationships now declared on each model (Resource.workflows, AppUser.workflows, ControllerMetadata.step/result, Organization.consultant, ResourceWorkflowStep.workflow/parents, ResourceWorkflowResult.workflow/steps, ResultsResourceWorkflowStep.source, Resource.parents). - session.query() migrated to session.execute(select(...)) in production code paths whose tests don't mock the model class through patching. - Several controllers (add_existing_user, manage_*, modify_user, mixins, resource_condor_workflow service, app_users decorators) intentionally left on session.query() because their unit tests mock get_app_user_model() with a MagicMock that select() rejects — converting them requires test rework beyond the scope of this commit. All 202 unit tests + 1115/1118 integrated tests pass; the 3 remaining failures are pre-existing baseline (condor sequence x2, pandas string/float). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Completes the SQLAlchemy 2 migration for the controllers/services that were left behind in 3bc1bc4 because their tests mocked model classes with MagicMock (which select() rejects with ArgumentError). Production migrations (10 files): controllers/app_users/{add_existing_user, manage_organization_members, manage_organizations, manage_resources, manage_users, mixins, modify_user}, controllers/resource_workflows/mixins, services/app_users/decorators, services/resource_condor_workflow. Test fixes (9 files): updated mocks to either patch select/delete in the controller's namespace (so MagicMock model classes don't trip ORM validation) or rewire the mock chain from session.query().get() to session.get(), and from session.query().X to session.execute().scalars().X. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…get() Cleans up the last live session.query() calls in test files: - file_collection_mixin_tests: count() queries → select(func.count()) - resource_workflows/base: get_app_user_model/get_resource_model tests use select(); test_get_resource mock chain switched to session.get - set_status_wv_tests, xms_tool_wv_tests, workflows_tab_tests: filter().one() patterns migrated to select().where().scalar_one() Only remaining session.query references in the codebase are inside commented-out code blocks. All 202 unit + 1115/1118 integrated tests pass (3 pre-existing baseline failures unchanged), flake8 clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- table_input_wv: NODATA-filled optional columns were being cast back to the template's object dtype, which pandas 3.0+ stringifies. Track the filled columns and force float64 dtype for them. - condor_workflow_manager_tests: test_run_job_prepared and test_run_prepare_with_callback_function asserted hardcoded CondorWorkflow ids (str(2), 3) that assumed PostgreSQL sequence values carried over between tests. Django's TestCase resets the sequence in this environment, producing id=1 every time. Replaced the brittle hardcoded ids with consistency checks against the actual workflow id assigned at runtime. All 202 unit + 1118 integrated tests pass; flake8 clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Migrates ATCore to SQLAlchemy 2.0 by updating ORM models to modern typed declarative mappings and refactoring query/engine usage across services, controllers, job scripts, and tests.
Changes:
- Updated models to SQLAlchemy 2.0 typed mappings (
Mapped[],mapped_column) and explicitback_populates. - Refactored ORM/query patterns to
Session.get()andsession.execute(select(...))style. - Updated tests/fixtures/mocks for SQLAlchemy 2.0 execution patterns and improved a few brittle test assertions.
Reviewed changes
Copilot reviewed 75 out of 76 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tethysext/atcore/tests/unit_tests/services/model_db_spatial_manager.py | Updates engine mocks to use connect().__enter__().execute() call path. |
| tethysext/atcore/tests/integrated_tests/services/spatial_reference.py | Uses engine.begin() + text() for PostGIS extension DDL. |
| tethysext/atcore/tests/integrated_tests/services/resource_condor_workflow_tests.py | Updates mocked ORM usage from query().get() to get(). |
| tethysext/atcore/tests/integrated_tests/services/model_database.py | Updates MockEngine to support connect() context manager and URL handling via make_url. |
| tethysext/atcore/tests/integrated_tests/services/file_database/file_database_client_tests.py | Migrates test queries/counts to SQLA 2.0 select()/execute() APIs. |
| tethysext/atcore/tests/integrated_tests/services/file_database/file_collection_client_tests.py | Migrates test queries/counts to SQLA 2.0 select()/execute() APIs. |
| tethysext/atcore/tests/integrated_tests/services/app_users/permissions_manager.py | Replaces ORM query with select() + scalar_one(). |
| tethysext/atcore/tests/integrated_tests/services/app_users/condor_workflow_manager_tests.py | Makes workflow-id assertions robust to sequence state; updates helper signature. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/table_input_rws_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/spatial_rws_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/spatial_input_rws_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/spatial_dataset_rws_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/spatial_condor_job_rws_test.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/spatial_attributes_rws_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/resource_workflow_steps/results_rws_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/initializer.py | Migrates query to select() + scalar_one_or_none(). |
| tethysext/atcore/tests/integrated_tests/models/files_database/file_database_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/files_database/file_collection_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/app_users/spatial_resource_tests.py | Updates geometry and ORM queries to SQLA 2.0 select()/execute(). |
| tethysext/atcore/tests/integrated_tests/models/app_users/resource_workflow_step_tests.py | Switches to Session.get() for load-by-id. |
| tethysext/atcore/tests/integrated_tests/models/app_users/resource_tests.py | Migrates counts/listing to SQLA 2.0 select()/execute(). |
| tethysext/atcore/tests/integrated_tests/models/app_users/organization_tests.py | Migrates queries to select() + scalar_one_or_none(). |
| tethysext/atcore/tests/integrated_tests/models/app_users/app_user_tests.py | Migrates many queries/counts to SQLA 2.0 select()/execute() APIs. |
| tethysext/atcore/tests/integrated_tests/mixins/file_collection_mixin_tests.py | Migrates count queries to select(func.count()). |
| tethysext/atcore/tests/integrated_tests/controllers/rest/spatial_reference.py | Uses engine.begin() + text() for PostGIS extension DDL. |
| tethysext/atcore/tests/integrated_tests/controllers/resources/tabs/workflows_tab_tests.py | Migrates workflow lookup to select() and Session.get(). |
| tethysext/atcore/tests/integrated_tests/controllers/resource_workflows/workflow_views/xms_tool_wv_tests.py | Migrates query to select() + scalar_one(). |
| tethysext/atcore/tests/integrated_tests/controllers/resource_workflows/workflow_views/set_status_wv_tests.py | Migrates query to select() + scalar_one(). |
| tethysext/atcore/tests/integrated_tests/controllers/resource_workflows/workflow_view_mixins_tests.py | Updates mocks/expectations from query().filter().one() to execute().scalar_one(). |
| tethysext/atcore/tests/integrated_tests/controllers/resource_workflows/base.py | Migrates list queries and resource lookup mocking to SQLA 2 patterns. |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/modify_user.py | Updates tests to patch select in controller namespace and mock execute().scalar_one(). |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/mixins.py | Updates mocks from query().get() to get() and exception paths accordingly. |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/manage_users_tests.py | Updates exception mock target from query to get. |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/manage_resources.py | Updates mocks to use Session.get() path. |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/manage_organizations.py | Updates mocks/patching for select() and execute().scalars().all(). |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/manage_organization_members.py | Updates mocks to use Session.get() path. |
| tethysext/atcore/tests/integrated_tests/controllers/app_users/add_existing_user.py | Updates tests to patch select and align with execute(select(...)) usage. |
| tethysext/atcore/services/spatial_reference.py | Refactors engine execution to SQLAlchemy 2 connection context + text(). |
| tethysext/atcore/services/resource_workflows/decorators.py | Replaces deprecated .query().get() calls with Session.get(). |
| tethysext/atcore/services/resource_condor_workflow.py | Replaces deprecated .query().get() with Session.get(). |
| tethysext/atcore/services/model_db_spatial_manager.py | Refactors raw SQL execution to connection context + text(). |
| tethysext/atcore/services/model_database.py | Refactors raw SQL execution to connection context + text() and ensures disposal in finally. |
| tethysext/atcore/services/file_database.py | Migrates .query().get() and .count() patterns to Session.get() and select(func.count()). |
| tethysext/atcore/services/app_users/decorators.py | Migrates user lookup query to select() + scalar_one_or_none(). |
| tethysext/atcore/models/types/guid.py | Handles native uuid.UUID values returned by drivers in process_result_value. |
| tethysext/atcore/models/resource_workflow_steps/results_rws.py | Updates relationships to typed mapping and explicit back_populates; adds source relation. |
| tethysext/atcore/models/file_database/file_database.py | Converts model to typed mapping; adds explicit relationship typing. |
| tethysext/atcore/models/file_database/file_collection.py | Converts model to typed mapping; adds explicit relationship typing. |
| tethysext/atcore/models/controller_metadata.py | Converts model to typed mapping; replaces backrefs with explicit reverse relationships. |
| tethysext/atcore/models/app_users/user_setting.py | Converts model to typed mapping. |
| tethysext/atcore/models/app_users/spatial_resource.py | Converts extent to typed mapping and migrates spatial SQL usage to select() + execute(). |
| tethysext/atcore/models/app_users/resource_workflow_step.py | Converts model to typed mapping; replaces backrefs with explicit parents/workflow/result relationships. |
| tethysext/atcore/models/app_users/resource_workflow_result.py | Converts model to typed mapping; adds explicit reverse relationships (workflow, steps). |
| tethysext/atcore/models/app_users/resource_workflow.py | Converts model to typed mapping; replaces backrefs with explicit back_populates. |
| tethysext/atcore/models/app_users/resource.py | Converts model to typed mapping; adds explicit parents/workflows relationships. |
| tethysext/atcore/models/app_users/organization.py | Converts model to typed mapping; replaces consultant backref with explicit relationships; wraps raw SQL in text(). |
| tethysext/atcore/models/app_users/initializer.py | Migrates staff-user lookup to select() + scalar_one_or_none(). |
| tethysext/atcore/models/app_users/base.py | Switches declarative base to SQLAlchemy 2 DeclarativeBase. |
| tethysext/atcore/models/app_users/app_user.py | Converts model to typed mapping; migrates multiple queries to select() + execute(). |
| tethysext/atcore/job_scripts/update_resource_status.py | Replaces deprecated .query().get() with Session.get(). |
| tethysext/atcore/handlers.py | Replaces deprecated .query().get() with Session.get(). |
| tethysext/atcore/controllers/resources/tabs/workflows_tab.py | Migrates query building to select() and result consumption via execute().scalars(). |
| tethysext/atcore/controllers/resource_workflows/workflow_views/table_input_wv.py | Fixes dtype coercion for NODATA-filled optional columns to preserve float dtype. |
| tethysext/atcore/controllers/resource_workflows/mixins.py | Migrates lookups to select() + scalar_one(). |
| tethysext/atcore/controllers/app_users/modify_user.py | Migrates lookups to select() + scalar_one() and Session.get(). |
| tethysext/atcore/controllers/app_users/modify_resource.py | Migrates resource/org lookups to Session.get() and list queries to select() + scalars(). |
| tethysext/atcore/controllers/app_users/modify_organization.py | Migrates org/resource lookups to select() + scalar_one() and Session.get(). |
| tethysext/atcore/controllers/app_users/mixins.py | Migrates resource lookups to Session.get(). |
| tethysext/atcore/controllers/app_users/manage_users.py | Migrates list/delete operations to select() and Session.get(). |
| tethysext/atcore/controllers/app_users/manage_resources.py | Migrates resource lookups to Session.get(). |
| tethysext/atcore/controllers/app_users/manage_organizations.py | Migrates list/delete operations to select() and Session.get(). |
| tethysext/atcore/controllers/app_users/manage_organization_members.py | Migrates org/user lookups to Session.get(). |
| tethysext/atcore/controllers/app_users/add_existing_user.py | Migrates org/user listing to Session.get() and select() + scalars(). |
| install.yml | Bumps SQLAlchemy requirement to >=2, adds GeoAlchemy2 >=0.13 and condorpy. |
| README.md | Updates dev/test setup docs (venv-based install and Docker PostGIS workflow). |
| .gitignore | Ignores local venv and .remember artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Retrieve a list of SRIDs from database | ||
| get_spatial_ref_list = "SELECT * FROM spatial_ref_sys WHERE ({0} = @srid)".format(srid) | ||
|
|
||
| spatial_ref_object_result = self.db_engine.execute(get_spatial_ref_list) | ||
| with self.db_engine.connect() as connection: | ||
| spatial_ref_object_result = connection.execute(text(get_spatial_ref_list)) |
There was a problem hiding this comment.
The SQL built for SRID lookup is invalid for PostgreSQL/SQLAlchemy and bypasses parameter binding: it formats srid directly into the string and compares it to @srid (which is not a valid bind parameter in SQLAlchemy/Postgres). This will produce a syntax error (or at best a query that can’t use indexes) and also creates SQL injection risk. Use a text() statement with a :srid bind param (e.g., ... WHERE srid = :srid) and pass {"srid": srid} to execute(); ideally coerce/validate srid to int first.
| # Retrieve a list of SRIDs from database | ||
| get_spatial_ref_list = "SELECT srtext FROM spatial_ref_sys WHERE ({0} = @srid)".format(srid) | ||
|
|
||
| spatial_ref_object_result = self.db_engine.execute(get_spatial_ref_list) | ||
| with self.db_engine.connect() as connection: | ||
| spatial_ref_object_result = connection.execute(text(get_spatial_ref_list)) |
There was a problem hiding this comment.
Same issue as above in get_wkt_by_srid(): the query interpolates srid into the SQL string and still uses @srid, which isn’t a valid SQLAlchemy bind parameter on Postgres. Switch to a text() query with :srid and pass the parameter dict to execute() (and validate/coerce srid to an int).
| # Retrieve a list of SRIDs from database | ||
| get_spatial_ref_list = "SELECT * FROM spatial_ref_sys " \ | ||
| "WHERE to_tsvector('english', srtext) @@ " \ | ||
| "to_tsquery('english', '{0}');".format(sql_query_input) | ||
|
|
||
| spatial_ref_object_result = self.db_engine.execute(get_spatial_ref_list) | ||
|
|
||
| # Parse out the wanted items into the list for the select input | ||
| for spatial_reference in spatial_ref_object_result: | ||
| spatial_ref_list.append( | ||
| { | ||
| "text": "{0} {1}".format(spatial_reference[0], spatial_reference[3].split('"')[1]), | ||
| "id": str(spatial_reference[0]) | ||
| } | ||
| ) | ||
| spatial_ref_object_result.close() | ||
| with self.db_engine.connect() as connection: | ||
| spatial_ref_object_result = connection.execute(text(get_spatial_ref_list)) | ||
|
|
There was a problem hiding this comment.
get_spatial_reference_system_by_query_string() builds a raw SQL string via .format(sql_query_input) and then executes it as text(...). This is SQL-injection prone (query terms come from user input) and also prevents the DB from caching query plans. Use bind parameters for the tsquery input (e.g., to_tsquery('english', :q)) and pass the value via execute(..., {"q": sql_query_input}); if you need operators like &, build the query string separately but still bind it.
| sql = "SELECT srid, proj4text FROM spatial_ref_sys WHERE srid = {}".format(srid) | ||
| ret = db_engine.execute(sql) | ||
| with db_engine.connect() as connection: | ||
| ret = connection.execute(text(sql)) |
There was a problem hiding this comment.
The SRID is interpolated directly into the SQL string (... WHERE srid = {}) and then executed via text(sql). Even if srid is typically an int, this bypasses bind parameters and is injection-prone if srid ever comes from request/user input. Prefer text('... WHERE srid = :srid') and pass {"srid": srid} to execute() (or build the statement with SQLAlchemy Core).
| if proj_format is self.PRO_WKT: | ||
| sql = "SELECT srtext AS proj_string FROM spatial_ref_sys WHERE srid = {}".format(srid) | ||
| else: | ||
| sql = "SELECT proj4text AS proj_string FROM spatial_ref_sys WHERE srid = {}".format(srid) | ||
|
|
||
| ret = db_engine.execute(sql) | ||
| projection_string = '' | ||
| with db_engine.connect() as connection: | ||
| ret = connection.execute(text(sql)) |
There was a problem hiding this comment.
Same parameter-binding issue in get_projection_string(): SRID is formatted into the SQL string before executing. Use a bind param (:srid) rather than string formatting so the DB can safely parameterize/cache the statement and avoid injection risk.
| kwargs: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | ||
| http_methods: Mapped[Optional[list]] = mapped_column(PickleType, default=['get', 'post', 'delete']) |
There was a problem hiding this comment.
These columns use mutable objects as default values (default={} and default=['get', ...]). With SQLAlchemy this can lead to shared mutable state between instances/rows and surprising mutations. Prefer callables for per-row defaults (e.g., default=dict and default=lambda: ['get', 'post', 'delete']).
| kwargs: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | |
| http_methods: Mapped[Optional[list]] = mapped_column(PickleType, default=['get', 'post', 'delete']) | |
| kwargs: Mapped[Optional[dict]] = mapped_column(PickleType, default=dict) | |
| http_methods: Mapped[Optional[list]] = mapped_column( | |
| PickleType, | |
| default=lambda: ['get', 'post', 'delete'], | |
| ) |
| _options: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | ||
| _attributes: Mapped[Optional[str]] = mapped_column(String) | ||
| _parameters: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | ||
| _active_roles: Mapped[Optional[list]] = mapped_column(PickleType, default=[]) |
There was a problem hiding this comment.
mapped_column(..., default={}) / default=[] uses mutable objects as defaults for _options, _parameters, and _active_roles. This can cause shared state across instances and hard-to-debug behavior when these dicts/lists are mutated. Use callable defaults instead (e.g., default=dict and default=list).
| _options: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | |
| _attributes: Mapped[Optional[str]] = mapped_column(String) | |
| _parameters: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | |
| _active_roles: Mapped[Optional[list]] = mapped_column(PickleType, default=[]) | |
| _options: Mapped[Optional[dict]] = mapped_column(PickleType, default=dict) | |
| _attributes: Mapped[Optional[str]] = mapped_column(String) | |
| _parameters: Mapped[Optional[dict]] = mapped_column(PickleType, default=dict) | |
| _active_roles: Mapped[Optional[list]] = mapped_column(PickleType, default=list) |
| _data: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | ||
| _options: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) |
There was a problem hiding this comment.
_data and _options are declared with default={}. Using a mutable object as a column default can lead to shared state across instances when mutated. Prefer default=dict (callable) so each row gets a fresh dict.
| _data: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | |
| _options: Mapped[Optional[dict]] = mapped_column(PickleType, default={}) | |
| _data: Mapped[Optional[dict]] = mapped_column(PickleType, default=dict) | |
| _options: Mapped[Optional[dict]] = mapped_column(PickleType, default=dict) |
|
@copilot rebase with master and resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/Aquaveo/tethysext-atcore/sessions/94d5033c-8d26-4d39-8aa3-818992ec56b4 Co-authored-by: swainn <5123221+swainn@users.noreply.github.com>
Merged master into the branch in commit ada074b. The only conflict was in |
Summary
Migrates tethysext-atcore to SQLAlchemy 2.0, including the modern typed-mapping syntax (Mapped[T] + mapped_column) and select()
query API. All tests pass on the new stack.
Primary changes
controller_metadata, and the resource_workflow hierarchy.
Resource.workflows/parents, AppUser.workflows, Organization.consultant, ControllerMetadata.step/result,
ResourceWorkflowStep.workflow/parents, ResourceWorkflowResult.workflow/steps, and ResultsResourceWorkflowStep.source.
(controllers, services, mixins, decorators, handlers, job_scripts) and integrated tests.
SQLA 2's ORM validation.
dtype, fixing a real bug exposed by pandas 3.0's stringify-on-object-cast behavior.
a manager share the same dynamically-assigned id.
PostGIS test-DB workflow that mirrors CI.
Please review the checklist before submitting the Pull Request:
Explain deviations from original design if applicable: