HTTP Query API Support - #1319
Conversation
The HTTP Query API will send `null` for notifications without a position. To make the experience the same between using bolt and HTTP, the driver drops `null` positions via HTTP to make them absent, just like they are via bolt.
* Skip non-applicable tests * Always specify a database name * Expect preview warning
| { | ||
| "testkit": { | ||
| "uri": "https://github.com/neo4j-drivers/testkit.git", | ||
| "ref": "6.x" |
There was a problem hiding this comment.
TODO: revert once TestKit PR has been merged:
|
|
||
|
|
||
| class _Http(_Direct): | ||
| _default_port = 7687 |
There was a problem hiding this comment.
_Http uses _default_port = 7687, the bolt port, so http://localhost with no port goes there rather than on 7474:
driver = neo4j.GraphDatabase.driver("http://localhost", auth=AUTH)
driver.address.port # -> 7687
is that intended, or does https want its own default (7473) as well?
| raise | ||
| if protocol_error or not connection_failed: | ||
| raise | ||
| for request in self._requests: |
There was a problem hiding this comment.
_CommitRequest never looks constructed anywhere (i think), so this isinstance can't match. the loop also scans the queued _requests, and send_all() has already popped the one in flight (the bolt version looks at self.responses for CommitResponse instead i believe).
i pointed the driver at a stub that drops the connection on the commit POST and answers the retry, and the write went in twice:
write query submitted : 2 time(s)
commit attempts : 2
commits dropped : 1
execute_write returned ['Alice']
ServiceUnavailable.is_retryable() is True where IncompleteCommit's is False, so the transaction function replays a commit with an outcome we don't know. am i reading that right? happy to hand over the stub i used to test it
There was a problem hiding this comment.
raised this on testkit#720 too, with a test that catches it, so no need for the stub now
| BoundedSemaphore: t.TypeAlias = threading.BoundedSemaphore | ||
|
|
||
|
|
||
| def acquire_bounded_semaphore( |
There was a problem hiding this comment.
semaphore.acquire(timeout=...) returns False on timeout rather than raising, and the result isn't kept here, so with max_connection_pool_size=1 i get a second connection out of the sync pool:
conn 0: acquired
conn 1: acquired <- over the limit
close: ValueError: Semaphore released too many times
the async twin does block, but comes out as a bare TimeoutError rather than ConnectionAcquisitionTimeoutError like the bolt pool. should this be checking the return value, or is this a none issue?
| async def next_id(self) -> int: | ||
| async with self._lock: | ||
| current_id = self._next_id | ||
| self._next_id = min((self._next_id + 1) % IdGenerator._MAX, 1) |
There was a problem hiding this comment.
min keeps this at 1 forever, so every http connection logged for me as [#0001]. did you mean max, to skip 0 on wrap?
| self._next_id = min((self._next_id + 1) % IdGenerator._MAX, 1) | |
| self._next_id = max((self._next_id + 1) % IdGenerator._MAX, 1) |
| @dataclasses.dataclass(slots=True, frozen=True, kw_only=True) | ||
| class HTTPServerInfo: | ||
| neo4j_version: str | ||
| parsed_neo4j_version: tuple[int, int] | None = dataclasses.field( |
There was a problem hiding this comment.
i've got a half-memory of a call about parsing the server version and rejecting the older ones that send values as a flat list rather than a list of records, so users get a clear message instead of a parse error. parsed_neo4j_version is computed here but i couldn't find anything reading it, so i may have the wrong end of this.
a stub advertising 5.19.0 with the flat shape gives:
QueryApiHttpError: protocol error: expected list, got: 1
and that class's docstring points people at filing a driver bug. can't recall if I'm remembering this correctly, and whether it still needs changed?
| Async Driver Construction | ||
| ========================= | ||
| AsyncDriver Construction | ||
| ======================== |
There was a problem hiding this comment.
async_api.rst doesn't look like it picked up the http changes that api.rst got: no http/https in the valid-URI list or the scheme table, no AsyncHttpDriver autoclass section, and line 454 still reads "the URI scheme is bolt:// or neo4j://". AsyncHttpDriver is exported and carries the whole "Unavailable Features" docstring, so i think none of it renders for async users. are the async docs to do at a later time?
| * :attr:`.ResultSummary.result_available_after` and | ||
| :attr:`.ResultSummary.result_consumed_after` will always be | ||
| :data:`None` because this information is not provided by the server. | ||
| TODO: check whether None or 0!! | ||
| * :attr:`.ServerInfo.agent` is being computed from the DBMS's | ||
| advertised version. Further, it is being cached to reduce | ||
| round-trips and overloading the DBMS's HTTP endpoints. | ||
| * :attr:`.ResultSummary.query_type` will always be :data:`None`. | ||
|
|
||
| * Transmitting and receiving :class:`Vector` values is currently not | ||
| supported. | ||
|
|
||
| * The only supported auth scheme (see :ref:`auth-ref`) is ``"basic"`` and | ||
| ``"bearer"``. |
There was a problem hiding this comment.
| * :attr:`.ResultSummary.result_available_after` and | |
| :attr:`.ResultSummary.result_consumed_after` will always be | |
| :data:`None` because this information is not provided by the server. | |
| TODO: check whether None or 0!! | |
| * :attr:`.ServerInfo.agent` is being computed from the DBMS's | |
| advertised version. Further, it is being cached to reduce | |
| round-trips and overloading the DBMS's HTTP endpoints. | |
| * :attr:`.ResultSummary.query_type` will always be :data:`None`. | |
| * Transmitting and receiving :class:`Vector` values is currently not | |
| supported. | |
| * The only supported auth scheme (see :ref:`auth-ref`) is ``"basic"`` and | |
| ``"bearer"``. | |
| * :attr:`.ResultSummary.result_available_after` and | |
| :attr:`.ResultSummary.result_consumed_after` will always be | |
| :data:`None` because this information is not provided by the server. | |
| * :attr:`.ServerInfo.agent` is being computed from the DBMS's | |
| advertised version. Further, it is being cached to reduce | |
| round-trips and overloading the DBMS's HTTP endpoints. | |
| * :attr:`.ResultSummary.query_type` will always be :data:`None`. | |
| * Transmitting and receiving :class:`Vector` values is currently not | |
| supported. | |
| * The only supported auth schemes (see :ref:`auth-ref`) are ``"basic"`` and | |
| ``"bearer"``. |
There was a problem hiding this comment.
is -> are, and removed the TODO as I "think" it's None - worth checking
| @property | ||
| @abc.abstractmethod | ||
| def supports_notification_filtering(self) -> bool: | ||
| """Whether the connection version supports re-authentication.""" |
There was a problem hiding this comment.
| """Whether the connection version supports re-authentication.""" | |
| """Whether the connection version supports notification filtering.""" |
| A string value must be provided connected via | ||
| ``http://`` or ``https://`` scheme. |
There was a problem hiding this comment.
| A string value must be provided connected via | |
| ``http://`` or ``https://`` scheme. | |
| A string value must be provided when connected via | |
| ``http://`` or ``https://`` scheme. |
| return await self._fetch_unguarded(query_api) | ||
| except Exception as e: | ||
| log.warning( | ||
| "[#%04X] _: Failed to fetch server info: %r", |
There was a problem hiding this comment.
| "[#%04X] _: Failed to fetch server info: %r", | |
| "[#%04X] _: Failed to fetch server info: %r", |
| def _compute_repr(self) -> str: | ||
| fields_repr = ( | ||
| f"{key!r}: {value!r}" for key, value in self._iter_sanintized() | ||
| ) | ||
| return f"{{{', '.join(fields_repr)}}}" | ||
|
|
||
| def _iter_sanintized(self) -> t.Generator[tuple[str, str]]: |
There was a problem hiding this comment.
| def _compute_repr(self) -> str: | |
| fields_repr = ( | |
| f"{key!r}: {value!r}" for key, value in self._iter_sanintized() | |
| ) | |
| return f"{{{', '.join(fields_repr)}}}" | |
| def _iter_sanintized(self) -> t.Generator[tuple[str, str]]: | |
| def _compute_repr(self) -> str: | |
| fields_repr = ( | |
| f"{key!r}: {value!r}" for key, value in self._iter_sanitized() | |
| ) | |
| return f"{{{', '.join(fields_repr)}}}" | |
| def _iter_sanitized(self) -> t.Generator[tuple[str, str]]: |
| .. _http-driver-ref: | ||
|
|
||
| HttpDriver | ||
| =========== |
There was a problem hiding this comment.
| =========== | |
| ========== |
StephenCathcart
left a comment
There was a problem hiding this comment.
Other than open questions looks good to me 👍
Neo4j servers recently introduced a new HTTP API for querying the database (called Query API): https://neo4j.com/docs/query-api/current/
This PR add support for configuring the driver with a HTTP URL (e.g.
https://neo4j.example.com:7474/), which will make the driver run queries and transactions via the HTTP Query API instead of Bolt.Notes
Please note that:
urllib3for sync andaiohttpfor async) are required. Installpip install neo4j[http]instead ofpip install neo4j.basicneo4j.ResultSummary.server.protocol_info) might be missing or inaccurate.UnsupportedTypetype, andUUIDs cannot be exchanged with the DBMS.fetch_sizehas not effect, there is no backpressure mechanism.notifications_min_severity,notifications_disabled_categories,notifications_disabled_classifications) have no effect.Preview
This feature is in preview. This means that it does not follow semver. It might be changed (or in rare cases be removed) at any time without following the normal deprecation cycle.
Depends on:
Closes: DRIVERS-118