HTTP/Query API tests - #720
Conversation
Also test non-auto-commit transactions
Add integration tests
| def with_commit( | ||
| self, | ||
| bookmarks: list[str] | None = None, | ||
| errors: list[dict[str, object]] | None = None, | ||
| status_code: int | None = None, | ||
| ) -> t.Self: | ||
| self._finishing_handler = HttpTxCommitEndpoint( | ||
| HttpTxCommitEndpoint.RequestData( | ||
| db=self._db, tx_id=self._tx_id, auth=self._auth | ||
| ), | ||
| HttpTxCommitEndpoint.ResponseData( | ||
| bookmarks=bookmarks, errors=errors, status_code=status_code | ||
| ), | ||
| ) | ||
| return self | ||
|
|
||
| def with_rollback( | ||
| self, | ||
| *, | ||
| legacy_response: bool = False, | ||
| errors: list[dict[str, object]] | None = None, | ||
| status_code: int | None = None, | ||
| ) -> t.Self: | ||
| self._finishing_handler = HttpTxRollbackEndpoint( | ||
| HttpTxRollbackEndpoint.RequestData( | ||
| db=self._db, tx_id=self._tx_id, auth=self._auth | ||
| ), | ||
| HttpTxRollbackEndpoint.ResponseData( | ||
| legacy=legacy_response, | ||
| errors=errors, | ||
| status_code=status_code, | ||
| ), | ||
| ) | ||
| return self |
There was a problem hiding this comment.
with_query passes _verify_affinity_header in but commit and rollback don't, though both endpoint classes take it. fed the matchers a request each way:
query endpoint without the header -> no match
commit endpoint without the header -> still matches
so test_affinity_header_* passes either way. docs say it's needed on commit/rollback too, and a real server 404s the commit without it. added the check to both and the suite still passed
| def with_commit( | |
| self, | |
| bookmarks: list[str] | None = None, | |
| errors: list[dict[str, object]] | None = None, | |
| status_code: int | None = None, | |
| ) -> t.Self: | |
| self._finishing_handler = HttpTxCommitEndpoint( | |
| HttpTxCommitEndpoint.RequestData( | |
| db=self._db, tx_id=self._tx_id, auth=self._auth | |
| ), | |
| HttpTxCommitEndpoint.ResponseData( | |
| bookmarks=bookmarks, errors=errors, status_code=status_code | |
| ), | |
| ) | |
| return self | |
| def with_rollback( | |
| self, | |
| *, | |
| legacy_response: bool = False, | |
| errors: list[dict[str, object]] | None = None, | |
| status_code: int | None = None, | |
| ) -> t.Self: | |
| self._finishing_handler = HttpTxRollbackEndpoint( | |
| HttpTxRollbackEndpoint.RequestData( | |
| db=self._db, tx_id=self._tx_id, auth=self._auth | |
| ), | |
| HttpTxRollbackEndpoint.ResponseData( | |
| legacy=legacy_response, | |
| errors=errors, | |
| status_code=status_code, | |
| ), | |
| ) | |
| return self | |
| def with_commit( | |
| self, | |
| bookmarks: list[str] | None = None, | |
| errors: list[dict[str, object]] | None = None, | |
| status_code: int | None = None, | |
| ) -> t.Self: | |
| self._finishing_handler = HttpTxCommitEndpoint( | |
| HttpTxCommitEndpoint.RequestData( | |
| db=self._db, tx_id=self._tx_id, auth=self._auth | |
| ), | |
| HttpTxCommitEndpoint.ResponseData( | |
| bookmarks=bookmarks, errors=errors, status_code=status_code | |
| ), | |
| extra_header_verification=(self._verify_affinity_header,), | |
| ) | |
| return self | |
| def with_rollback( | |
| self, | |
| *, | |
| legacy_response: bool = False, | |
| errors: list[dict[str, object]] | None = None, | |
| status_code: int | None = None, | |
| ) -> t.Self: | |
| self._finishing_handler = HttpTxRollbackEndpoint( | |
| HttpTxRollbackEndpoint.RequestData( | |
| db=self._db, tx_id=self._tx_id, auth=self._auth | |
| ), | |
| HttpTxRollbackEndpoint.ResponseData( | |
| legacy=legacy_response, | |
| errors=errors, | |
| status_code=status_code, | |
| ), | |
| extra_header_verification=(self._verify_affinity_header,), | |
| ) | |
| return self |
| class _FailPoint(enum.Enum): | ||
| PRE_TX_CREATION = enum.auto() | ||
| PRE_QUERY_HEADER = enum.auto() | ||
| PRE_QUERY_RECORDS = enum.auto() | ||
| POST_QUERY_RECORDS = enum.auto() |
There was a problem hiding this comment.
this happens before the query finishes, so the driver always gets a proper error body and knows the commit didn't land. couldn't see anything for a commit that gets no answer at all
test_auth_token_manager does put errors in with_commit / with_rollback, but that's a response rather than a dropped connection. looks like you had in the TODO.md "amend: tx commit & rollback" unticked so you might already be aware?
bolt has test_retry.py::test_disconnect_on_commit for this. works here too with a drop_connection flag on HttpTxCommitEndpoint.ResponseData that streams a body shorter than its Content-Length. tried it and it caught a real retry:
FAIL: test_no_retry_on_commit_disconnect
AssertionError: DriverError not raised
can you double check this? im assuming it wants the java/dotnet skip that the bolt one has
| def _get_counters(self, req: Request) -> t.Any: | ||
| if isinstance(self.counters, AutoRespond): | ||
| body = req.get_json(force=True) | ||
| if body.get("include_counters") is True: |
There was a problem hiding this comment.
i think the real field is includeCounters, so AutoRespond never fires. same in _tx_query.py:135 and _tx.py:177, which also read query instead of statement (their own matchers below use statement).
before: includeCounters: true -> no counters
after: includeCounters: true -> counters
after: includeCounters absent -> no counters
probs not a problem right now since the counters tests pass an explicit CountersMap, but might be an issue later? fixed all three and the suite still passed.
| if body.get("include_counters") is True: | |
| if body.get("includeCounters") is True: |
| STATUS_CODES = [ | ||
| None, # auto | ||
| # Some early server versions sometimes return errors with 2xx status codes. | ||
| # Drivers should pick up the error anyway and throw. | ||
| 200, | ||
| 400, |
There was a problem hiding this comment.
had a poke at a 2025.04.0 server to see what it sends with errors: runtime ones (RETURN 1/0, or after some records) are 202 with data + errors, syntax errors and bad bodies are 400, missing db / unknown tx are 404. same inside a tx.
so 400's fine, but the default error here is ArithmeticError "/ by zero" which is a 202 case, and 202 isn't in the list. added it and test_errors still passed. does 200 cover it fine?
| STATUS_CODES = [ | |
| None, # auto | |
| # Some early server versions sometimes return errors with 2xx status codes. | |
| # Drivers should pick up the error anyway and throw. | |
| 200, | |
| 400, | |
| STATUS_CODES = [ | |
| None, # auto | |
| # Some early server versions sometimes return errors with 2xx status codes. | |
| # Drivers should pick up the error anyway and throw. | |
| 200, | |
| # What current servers return for errors raised during execution. | |
| 202, | |
| 400, |
| if protocol_version == ProtocolVersion.V1_0: | ||
| return content_type in { | ||
| "application/vnd.neo4j.query", | ||
| "application/vnd.neo4j.query.v1.0", | ||
| } | ||
| elif protocol_version == ProtocolVersion.V1_1: | ||
| return content_type == "application/vnd.neo4j.query.v1.1" | ||
| else: | ||
| NotImplementedError("TODO") | ||
|
|
||
| @classmethod | ||
| def _version_as_header( | ||
| cls, | ||
| protocol_version: ProtocolVersion, | ||
| ) -> dict[str, str]: | ||
| if protocol_version == ProtocolVersion.V1_0: | ||
| return { | ||
| "Content-Type": "application/vnd.neo4j.query", | ||
| } | ||
| elif protocol_version == ProtocolVersion.V1_1: | ||
| return { | ||
| "Content-Type": "application/vnd.neo4j.query.v1.1", | ||
| } | ||
| else: | ||
| NotImplementedError("TODO") |
There was a problem hiding this comment.
do the errors need raised?
| if protocol_version == ProtocolVersion.V1_0: | |
| return content_type in { | |
| "application/vnd.neo4j.query", | |
| "application/vnd.neo4j.query.v1.0", | |
| } | |
| elif protocol_version == ProtocolVersion.V1_1: | |
| return content_type == "application/vnd.neo4j.query.v1.1" | |
| else: | |
| NotImplementedError("TODO") | |
| @classmethod | |
| def _version_as_header( | |
| cls, | |
| protocol_version: ProtocolVersion, | |
| ) -> dict[str, str]: | |
| if protocol_version == ProtocolVersion.V1_0: | |
| return { | |
| "Content-Type": "application/vnd.neo4j.query", | |
| } | |
| elif protocol_version == ProtocolVersion.V1_1: | |
| return { | |
| "Content-Type": "application/vnd.neo4j.query.v1.1", | |
| } | |
| else: | |
| NotImplementedError("TODO") | |
| if protocol_version == ProtocolVersion.V1_0: | |
| return content_type in { | |
| "application/vnd.neo4j.query", | |
| "application/vnd.neo4j.query.v1.0", | |
| } | |
| elif protocol_version == ProtocolVersion.V1_1: | |
| return content_type == "application/vnd.neo4j.query.v1.1" | |
| else: | |
| raise NotImplementedError("TODO") | |
| @classmethod | |
| def _version_as_header( | |
| cls, | |
| protocol_version: ProtocolVersion, | |
| ) -> dict[str, str]: | |
| if protocol_version == ProtocolVersion.V1_0: | |
| return { | |
| "Content-Type": "application/vnd.neo4j.query", | |
| } | |
| elif protocol_version == ProtocolVersion.V1_1: | |
| return { | |
| "Content-Type": "application/vnd.neo4j.query.v1.1", | |
| } | |
| else: | |
| raise NotImplementedError("TODO") |
| @dataclasses.dataclass | ||
| class MaybeNull(t.Generic[T]): | ||
| """ | ||
| Instruct a matcher to accept requests with this value missing/bein None. |
There was a problem hiding this comment.
| Instruct a matcher to accept requests with this value missing/bein None. | |
| Instruct a matcher to accept requests with this value missing/being None. |
| return self | ||
|
|
||
| def build(self) -> HttpEndpoint: | ||
| assert self._sequential_handlers, "initialized non-empty int __init__" |
There was a problem hiding this comment.
| assert self._sequential_handlers, "initialized non-empty int __init__" | |
| assert self._sequential_handlers, "initialized non-empty in __init__" |
StephenCathcart
left a comment
There was a problem hiding this comment.
Mainly open questions - looks good! 👍
Closes: DRIVERS-326