fix: unexpected commit causes pytest failure

[zhaoyongjie]

SUMMARY

The latest SQLAlchemy upgrade seems to be changing the autoflush rule, the full background at here. It causes test_fetch_metadata_for_updated_virtual_table failure.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Pure backend changes, so no screenshot.

TESTING INSTRUCTIONS

should pass following command in local and passes CI.

pytest --disable-warnings -s tests/integration_tests/sqla_models_tests.py -k test_fetch_metadata_for_updated_virtual_table

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #20780 (47b519b) into master (81bd496) will decrease coverage by 0.77%.
The diff coverage is 100.00%.

❗ Current head 47b519b differs from pull request most recent head 083adcf. Consider uploading reports for the commit 083adcf to get more accurate results

@@            Coverage Diff             @@
##           master   #20780      +/-   ##
==========================================
- Coverage   66.28%   65.51%   -0.78%     
==========================================
  Files        1756     1756              
  Lines       66756    66758       +2     
  Branches     7049     7049              
==========================================
- Hits        44252    43738     -514     
- Misses      20708    21224     +516     
  Partials     1796     1796              

Flag	Coverage Δ
hive	53.24% <100.00%> (-0.01%)	⬇️
mysql	?
postgres	?
presto	53.10% <100.00%> (-0.01%)	⬇️
python	79.92% <100.00%> (-1.60%)	⬇️
sqlite	79.55% <100.00%> (+<0.01%)	⬆️
unit	50.24% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/utils/database.py	85.71% <100.00%> (+0.86%)	⬆️
superset/datasets/commands/update.py	25.30% <0.00%> (-68.68%)	⬇️
superset/datasets/commands/create.py	29.41% <0.00%> (-68.63%)	⬇️
superset/datasets/commands/delete.py	38.63% <0.00%> (-54.55%)	⬇️
superset/datasets/commands/bulk_delete.py	33.33% <0.00%> (-53.34%)	⬇️
superset/datasets/columns/commands/delete.py	44.11% <0.00%> (-52.95%)	⬇️
superset/datasets/metrics/commands/delete.py	44.11% <0.00%> (-52.95%)	⬇️
superset/sql_validators/postgres.py	50.00% <0.00%> (-50.00%)	⬇️
superset/datasets/commands/samples.py	41.86% <0.00%> (-46.52%)	⬇️
superset/datasets/dao.py	47.79% <0.00%> (-46.33%)	⬇️
... and 29 more

[zhaoyongjie]

Although the Select can also be placed in a transaction, it relies on the subsequent commit(), which is not useful.

[michael-s-molina]

Pinging @betodealmeida for awareness

[geido]

Hello @zhaoyongjie thanks for the PR!

The latest SQLAlchemy #19890 seems to be changing the autoflush rule

Can you provide more context about this? How is the autoflush rule changed?

[zhaoyongjie]

Can you provide more context about this? How is the autoflush rule changed?

The test case tests/integration_tests/sqla_models_tests.py::TestDatabaseModel::test_fetch_metadata_for_updated_virtual_table has a line as following, the fetch_metadata(commit=False) means that don't perform commit immediately in the function call.

superset/tests/integration_tests/sqla_models_tests.py

Lines 454 to 456 in 81bd496

	# make sure the columns have been mapped properly
	assert len(table.columns) == 4
	table.fetch_metadata(commit=False)

but this transaction was interrupted by get_or_create_db, ---- the commit() would be executed whenever call the get_or_create_db.

superset/superset/utils/database.py

Lines 34 to 64 in 81bd496

	def get_or_create_db(
	database_name: str, sqlalchemy_uri: str, always_create: Optional[bool] = True
	) -> Database:
	# pylint: disable=import-outside-toplevel
	from superset import db
	from superset.models import core as models
	database = (
	db.session.query(models.Database)
	.filter_by(database_name=database_name)
	.autoflush(False)
	.first()
	)
	# databases with a fixed UUID
	uuids = {
	"examples": EXAMPLES_DB_UUID,
	}
	if not database and always_create:
	logger.info("Creating database reference for %s", database_name)
	database = models.Database(
	database_name=database_name, uuid=uuids.get(database_name)
	)
	db.session.add(database)
	if database:
	database.set_sqlalchemy_uri(sqlalchemy_uri)
	db.session.commit()
	return database

The strange thing is that this only happens under Postgres, and only after the SQLAlchemy update.

You can observe that L44 has a call autoflash(False). This means that the select statement is also executed inside the transaction.

Sorry I didn't delve into SQLAlchmey upgrade and PG dialects to continue research, ----- just remove autoflash(False) and prevent execute commit() if no need.