fix(sql lab): Tracking URL getter/setter

[john-bodley]

SUMMARY

I'm unclear if #20799 ever stored the correct URL in the database, but it seems like the query.tracking_url column is not updated with the transformed URL when the record is persisted to the database.

Although the unit tests illustrate the getter/setter behavior (and thus the link in SQL Lab works) it doesn't include a database write and thus it's unclear if SQLAlchemy invokes the getter when serializing a record to the database. Typically the getter should simply return the value whereas the setter should perform the necessary augmentation which is the logic I've implemented here, i.e., the _tracking_url column always contains the correct transformed value (if appropriate) whereas previous only the tracking_url field (proxy) was correct.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI. Also tested locally and confirmed that the tracking_url which was persisted in the database was correct.

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 #23387 (3982680) into master (da3791a) will increase coverage by 11.29%.
The diff coverage is 100.00%.

❗ Current head 3982680 differs from pull request most recent head 93a3fa5. Consider uploading reports for the commit 93a3fa5 to get more accurate results

@@             Coverage Diff             @@
##           master   #23387       +/-   ##
===========================================
+ Coverage   56.27%   67.57%   +11.29%     
===========================================
  Files        1907     1907               
  Lines       73495    73494        -1     
  Branches     7977     7977               
===========================================
+ Hits        41356    49660     +8304     
+ Misses      30091    21786     -8305     
  Partials     2048     2048               

Flag	Coverage Δ
hive	52.76% <80.00%> (?)
mysql	78.43% <80.00%> (?)
postgres	78.49% <80.00%> (?)
presto	52.69% <100.00%> (-0.01%)	⬇️
python	82.28% <100.00%> (+23.38%)	⬆️
sqlite	76.97% <80.00%> (?)
unit	52.50% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
superset/models/sql_lab.py	74.60% <100.00%> (+4.18%)	⬆️

... and 300 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[michael-s-molina]

Typically the getter should simply return the value whereas the setter should perform the necessary augmentation which is the logic I've implemented here, i.e., the _tracking_url column always contains the correct transformed value (if appropriate) whereas previous only the tracking_url field (proxy) was correct.

@john-bodley Would we need a migration to transform the previously saved values?

[ktmud]

The original intention was to store the raw url and always transform the url at runtime. This was by design.

We need transform at runtime because certain urls expires and we need to return None when they do---this requires runtime information such as current timestamp. Superset administrators can also tweak the transform function from time to time in cases like when they migrate the tracking ui server to a new host name. The original URL would always contain the most original information so it makes more sense to store the original URL in the database.

[john-bodley]

@ktmud if that's the case then rather than augmenting the tracking_url (via a getter/setter method)which corresponds to an actual column in the table (which likely adds to some level of confusion, i.e., I was tripped on this), why not move the logic which resides in,

@property
def tracking_url(self) -> Optional[str]:
 ...

to a different property, maybe tracking_url_transformed and expose that to the frontend for rendering purposes?

[ktmud]

I think I tried that but couldn't figure out how to expose a new property in the FAB API...