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.

• No exception when trying to connect to server sqlalchemy-cratedb#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]

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

Loading

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]

There is no server at http://127.0.0.1:4200. This test case just didn't fail because connect() did not raise an exception up until now.

Now, there is no longer a way to validate connecting to the default address per doctest file, because there is no server listening at http://127.0.0.1:4200. Because the core information is still viable, all what's left is pure prose, rephrased a bit.

[surister]

LGTM

[seut]

Lgtm

[amotl]

str(connection_errors) uses an opaque way to serialize the list of exception instances into a string, but I think it is viable and does not cause too many surprises for callers that expect exception.message to be of str type. Do you have any objections or suggestions to do it differently, like using JSON instead? ¹

Footnotes

1. In general, raise ConnectionError(connection_errors) is also possible, but the caller then needs to handle the exception value as a list type, which is semantically different on some occasions like "Server not available" in ex.exception.message would no longer be True, bearing potential breaking changes, at least for a few test cases of the test suite already. ↩

[amotl]

[...] suggestions to do it differently, like using JSON instead?

The procedure now uses JSON instead of an opaque Python-serialized string per e818221.

For a single element, it looks like this now:

 crate.client.exceptions.ConnectionError: ["Server not available, exception: HTTPConnectionPool(host='127.0.0.1', port=44209): Max retries exceeded with url: / (Caused by ReadTimeoutError(\"HTTPConnectionPool(host='127.0.0.1', port=44209): Read timed out. (read timeout=0.01)\"))"]

-- https://github.com/crate/crate-python/actions/runs/14928213165/job/41937814085?pr=711#step:5:357

[seut]

👍

[amotl]

We just published a pre-release package including those updates per crate-2.1.0.dev0 and notified @shraik about it at crate/sqlalchemy-cratedb#218 (comment).

[matriv]

@amotl Thank you! Is there a way to test that we don't fail the connection attempt, but only if only all servers provided fail?
Can we also test the exception when multiple nodes fail?

[surister]

why not just raise ConnectionError(error_list)?

[amotl]

I've elaborated about it here, but I am also not sure, that's why I am also asking about your opinion.

[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.