feat: Refactor SQLConnector connection handling

[qbatten]

This is a draft PR that proposes a solution to issue #1393.

Broad overview:

• We are no longer storing an open connection on this object. Instead, we store an engine. Wherever we were previously making a new engine, we instead access the connector's existing engine. When a connection is needed, we open one at that time.
• When connections are opened, they're opened using self._connect() as a context manager. This rule is put into place where it's possible to do so without making a breaking change.
• Several methods are prepared for deprecation, but their function is preserved.
  • (Breaking change) _connection is removed altogether. The tap no longer accepts that argument on init, and we don't store a single connection at all
    • This is a breaking change for any subclasses that were using this property
    • This is a breaking change for any subclasses that counted on a single connection being reused throughout the lifetime of the instance
    • This is a breaking change for any cases where an external method call counted on a single connection being returned each time that create_sqlalchemy_connection was called (or for anyone who didn't respect the protectedness of _connection).
  • (Slightly breaking) connection still exists, it just calls create_sqlalchemy_connection. So this is a slightly breaking change—- it returns a new connection instead of the _connection that prviously had been preserved on the instance.
  • create_sqlalchemy_engine now just returns self._engine, since we have an engine on this object anyway.
  • create_sqlalchemy_connection now opens a connection using self._engine and returns the open connection.

Detailed changes in functionality:

1. self._connect() is added. This is the correct way to open connections now—- you use it as a context manager, exactly as you would sqlalchemy.engine.connect()... in fact currently it just directly wraps that.
2. We now create and cache a single engine for this instance. It's accessed as a protected property under _engine.
3. create_sqlalchemy_connection calls self._engine now instead of self.create_sqlalchemy_engine, since the connector now has an engine. This and the next item help extricate and disconnect these soon-to-be-deprecated methods.
4. create_sqlalchemy_engine now calls self._engine instead of making the engine itself.
5. Everywhere else in the SQLConnector that connection.execute was called now uses the correct/new _connect strategy.
6. Internal calls to create_sqlalchemy_engine now just call _engine (this is just a change to discover_catalog_entries).

---

📚 Documentation preview 📚: https://meltano-sdk--1394.org.readthedocs.build/en/1394/

[qbatten]

(re: mypy fix; issue was caching the _engine property). I used an lru_cache decorator at first, but there doesn't seem to be any real way to handle typing for a lru_cache-wrapped function that returns something— see here. So instead I just used another property to cache it. Maybe not the most pythonic(?) but it works fine. Another option is to type: ignore that line. I haven't worked much with type checkers, so I'm not sure what the right move is here. Ofc open to suggestions if anyone has an opinion.

[codecov]

Codecov Report

Merging #1394 (050157e) into main (ccdc6a6) will increase coverage by 0.19%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##             main    #1394      +/-   ##
==========================================
+ Coverage   85.19%   85.39%   +0.19%     
==========================================
  Files          54       54              
  Lines        4722     4737      +15     
  Branches      803      808       +5     
==========================================
+ Hits         4023     4045      +22     
+ Misses        507      501       -6     
+ Partials      192      191       -1     

Impacted Files	Coverage Δ
singer_sdk/connectors/sql.py	81.91% <93.54%> (+3.63%)	⬆️

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

[kgpayne]

@qbatten this is great, thanks for all your work on connection handling 🙏 As we are pre-1.0 for the SDK, and especially since there are far fewer users of the SQLConnector at this point, I think the breaking changes are OK. However, it would be good to proactively reach out to the Taps/Targets we know about that would potentially be impacted (using Hub data), to make sure they are pinning their SDK version and are aware of the changes 🙂 I can help with that 👍

[kgpayne]

LGTM 🚢 Minor suggestions around deprecation warnings 👍

[qbatten]

Test coverage! Woohoo!

[edgarrmondragon]

@qbatten this is looking great 😄

[edgarrmondragon]

Thanks @qbatten, this is g2g!

@kgpayne I'll leave this open in case you want to review, otherwise I'll merge first thing on Monday.