Fix: Fail early when database cluster does not respond

[amotl]

Problem

@shraik started using sqlalchemy-cratedb and reported that its behaviour deviates from other vendors by not failing on engine.connect() when the database server is not available.

• https://github.com/crate/sqlalchemy-cratedb/issues/218

We found this is not actually on the SQLAlchemy dialect, but on the DBAPI driver already, which exhibits the same behaviour.

Solution

The patch extends the _lowest_server_version method to re-raise ~the last ConnectionError when no connection can be made to any configured server node~ a ConnectionError when connecting to all server nodes fails, including all error messages.

By doing it this way, we didn't need to submit a dummy SQL command like originally planned. We think it is much better this way because it does not pollute the server-side statement log.

Details

As an additional benefit, the software tests in test_connection.py are now actual integration tests.

/cc @mfussenegger, @seut, @surister

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

"""

Walkthrough

The changes update the connection logic to fail early when the database server is unresponsive, by raising the last encountered connection error if no valid server version is found. The changelog is updated to reflect this. Additionally, a new test is introduced to verify that connection errors are properly raised for invalid server addresses. The client connection documentation was simplified by removing an interactive example. The test suite registration for connection tests was moved to the integration test layer.

Changes

File(s)	Change Summary
CHANGES.rst	Added changelog entry about early failure on unresponsive database server.
src/crate/client/connection.py	Modified _lowest_server_version to raise last ConnectionError if no server is reachable, collecting all connection errors.
tests/client/test_connection.py	Added test to assert that a ConnectionError is raised for invalid server addresses; replaced hardcoded server address with variable.
tests/client/tests.py	Moved ConnectionTest registration from unit to integration test suite with test layer.
docs/by-example/client.rst	Simplified client connection docs by removing interactive example and verification output.
setup.py	Added minimum version requirement verlib2>=0.3 in install_requires; updated license metadata.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Connection
    participant Server

    Client->>Connection: connect()
    loop For each server
        Connection->>Server: request server version
        alt Server responds with version
            Connection->>Connection: store version if valid
        else Server not available (ConnectionError)
            Connection->>Connection: store last ConnectionError
        else Invalid version (ValueError/InvalidVersion)
            Connection->>Connection: ignore and continue
        end
    end
    alt No valid server version found and ConnectionError occurred
        Connection-->>Client: raise last ConnectionError
    else Valid server version found
        Connection-->>Client: proceed with connection
    end

Poem

In tunnels deep, connections seek,
To servers strong or servers weak.
Now if a host just will not play,
We fail up front—no more delay!
A test ensures the right alarm,
No silent errors, just clear charm.
🐇✨ """

✨ Finishing touches

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment
• [ ] Commit unit tests in branch fail-on-connect

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[amotl]

We just published a pre-release package including those updates per crate-2.1.0.dev0 and notified @shraik about it at https://github.com/crate/sqlalchemy-cratedb/issues/218#issuecomment-2866518582.

[amotl]

Hi @surister. The tests are failing now after rebasing on top of the recent test suite refactorings. Can I humbly ask you to look into it? Maybe you can spot the flaw quickly on the interface of both?

[surister]

Hi @surister. The tests are failing now after rebasing on top of the recent test suite refactorings. Can I humbly ask you to look into it? Maybe you can spot the flaw quickly on the interface of both?

looking

[surister]

@amotl It's very positive that many unit tests fail now :) since this new PR introduces breaking changes and in the modernization I wrote many new tests (it's actually worrysome that it didn't in the past). So whoever picks this up will have to adapt the tests to the new behavior. It's nothing inherently 'wrong' about the modernization itself.

For example, before connect() would just silently log and ignore connection error, your PR now raises an exception:

        if lowest is None and last_connection_error is not None:
            raise last_connection_error

That breaks this tests for example:

def test_connection_closes_context_manager():
    """Verify that the context manager of the client closes the connection"""
    with patch.object(connect, "close", autospec=True) as close_fn:
        with connect():
            pass
        close_fn.assert_called_once()

Which expects that when creating a Connection object and no CrateDB is found, no error is raised, ideally this test isolates the intended tested behavior enough so it does not fail but that's a lot more work.