refactor: rest client#217
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the write path to use a new synchronous RestClient (urllib3-based) instead of the previous generated client/write service wiring, and updates InfluxDBClient3 construction accordingly.
Changes:
- Refactors
WriteApito issue HTTP requests via a newRestClient, adds gzip decision/compression utilities, and adds endpoint/exception translation logic. - Introduces
influxdb_client_3/write_client/_sync/rest_client.pyto encapsulate urllib3 request/response handling. - Updates
InfluxDBClient3initialization to pass write connection details (base_url/auth/gzip) directly intoWriteApi.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
influxdb_client_3/write_client/client/write_api.py |
Major refactor of write implementation to use a new REST client and custom request building/serialization paths. |
influxdb_client_3/write_client/_sync/rest_client.py |
Adds a new urllib3-based synchronous REST client abstraction used by WriteApi. |
influxdb_client_3/__init__.py |
Wires new write API constructor parameters (auth/gzip/base_url) and updates public kwargs documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2b26e5c to
7d74438
Compare
52cee20 to
a7076f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:251
WriteApi.write()takes parameters in(bucket, org, record, ...)order, but this test passes(org, bucket, record). This makes the test less representative of real usage and can mask issues in how query parameters are constructed.
tests/test_write_api.py:293- This call passes
(org, bucket, record)instead of(bucket, org, record), which makes the timeout test less representative of real usage and inconsistent with other tests in this file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
influxdb_client_3/write_client/client/write_api.py:1171
__getstate__deletes_write_service, butWriteApino longer defines that attribute. This will raiseKeyErrorduring pickling (anddel state['_subject']/del state['_disposable']can also KeyError depending on instance state). Usestate.pop(..., None)and remove the stale_write_servicereference.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
influxdb_client_3/write_client/client/write_api.py:1181
__setstate__callsself.__init__(self._write_options, self._point_settings, ...), butWriteApi.__init__requirestoken,bucket, andorgas the first arguments. As written, unpickling will raiseTypeError. Recreate the Rx batching pipeline based on the restored_write_optionsinstead of re-calling__init__with the wrong signature.
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
tests/test_write_api.py:252
WriteApi.write()expects positional arguments in the order(bucket, org, record). This call passes org first, which swaps the query parameters and makes the test less representative of real usage. Use keyword arguments (or correct positional order) to avoid accidental swaps.
tests/test_write_api.py:293WriteApi.write()positional argument order is(bucket, org, record). This test passes org then bucket, which is easy to get wrong and can hide routing/query-param bugs. Prefer keyword arguments to make the intent explicit (and keep_request_timeoutbehavior under test).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
tests/test_write_api.py:29
mock_urllib3_timeout_requestis used as aside_effectforurllib3._request_methods.RequestMethods.request, which passes theRequestMethodsinstance as the first positional arg. The helper currently omits that parameter, somethod/urlare shifted and the test can behave incorrectly.
tests/test_write_api.py:252WriteApi.write()takesbucketas the first positional argument andorgas the second. This call passes them in the opposite order, so the request routing/query params will be wrong.
tests/test_write_api.py:293WriteApi.write()takesbucketas the first positional argument andorgas the second. This call passes them in the opposite order, so it won't exercise the intended timeout behavior for the right endpoint params.
influxdb_client_3/write_client/client/write_api.py:1171__getstate__deletesstate['_write_service'], but this class no longer defines_write_service. Pickling will raiseKeyErrorhere.
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
influxdb_client_3/write_client/client/write_api.py:1181
__setstate__callsself.__init__(self._write_options, self._point_settings, ...), but__init__now requirestoken,bucket,org, and other constructor args. This will break unpickling; it should just recreate the Rx batching pipeline based on the restored_write_options.
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tests/test_write_api.py:252
WriteApi.write()takes positional arguments in the order(bucket, org, record). This call passes org first and bucket second, which swaps routing parameters and can hide bugs in header/query handling. Prefer keyword arguments (or swap the positionals) to make the intent unambiguous.
tests/test_write_api.py:293- Same positional-argument issue as above: this passes org first and bucket second to
WriteApi.write(bucket, org, record), which inverts query parameters. Using keywords also makes the_request_timeoutintent clearer.
influxdb_client_3/write_client/client/write_api.py:1181 __getstate__/__setstate__are broken after the refactor:_write_serviceis no longer an attribute, sodel state['_write_service']will raiseKeyErrorduring pickling. Additionally,__setstate__calls__init__with the wrong arguments for the new constructor signature (it no longer accepts justwrite_optionsandpoint_settings). This makes pickling/unpickling unusable.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
influxdb_client_3/write_client/client/write_api.py:1181
__getstate__deletesstate['_write_service'], butWriteApino longer defines_write_service, so pickling will raiseKeyError. Additionally,__setstate__calls__init__with the wrong signature (it now requirestoken,bucket,org, etc.), so unpickling will fail even if__getstate__succeeds. Safer approach:pop()optional fields and recreate the Rx pipeline based on_write_optionswithout re-calling__init__.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
tests/test_write_api.py:252
WriteApi.write()expects positional arguments as(bucket, org, record, ...), but this test passes them as(org, bucket, record). That swaps query params and makes the test exercise the wrong behavior. Use keyword args (or correct positional order) to match thewrite()signature.
tests/test_write_api.py:294- This
write()call also swaps(org, bucket)positional order. Since the assertion is about timeout propagation rather than parameter order, using keyword args avoids accidentally testing the wrong thing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:252
- In this test,
WriteApi.write()expects positional args in the order(bucket, org, record). Passing org first swaps the routing parameters and can hide bugs in request construction/error handling.
tests/test_write_api.py:293 WriteApi.write()positional parameters are(bucket, org, record). This call passes org first, which makes the test exercise the wrong request routing.
87ccdef to
273b4ce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
influxdb_client_3/write_client/client/write_api.py:1179
__getstate__()deletes_write_service, butWriteApino longer sets this attribute, so pickling will raiseKeyError. Additionally,__setstate__()calls__init__()with the wrong signature (it now requirestoken/bucket/org/...).
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
tests/test_write_api.py:252
- Argument order is swapped in this
write()call:WriteApi.write(bucket, org, record, ...)expects bucket first, org second, but this passes org then bucket. This makes the test less meaningful and can hide real regressions.
tests/test_write_api.py:293 - Argument order is swapped in this
write()call:WriteApi.write(bucket, org, record, ...)expects bucket first, org second. As written, the test exercises the wrong parameter mapping (and still "works" only because values are strings).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 78.90% 83.96% +5.05%
==========================================
Files 36 28 -8
Lines 2607 2014 -593
==========================================
- Hits 2057 1691 -366
+ Misses 550 323 -227 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d4ed5f3 to
6e100f8
Compare
166cec0 to
1313ede
Compare
719f50e to
14e1a52
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:252
WriteApi.write()expects(bucket, org, record)(see examples inwrite_api.py). This test passes org/bucket in the opposite order, which makes the call misleading and can hide parameter-order bugs. Prefer keyword arguments here to make the intent explicit.
tests/test_write_api.py:294WriteApi.write()expects(bucket, org, record). This call passes org/bucket in the opposite order; using keyword args avoids accidentally swapping them when adding_request_timeout.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
influxdb_client_3/write_client/client/write_api.py:362
WriteApi.__init__initializesself._write_optionswith a default whenwrite_optionsisNone, but then immediately overwrites it with the rawwrite_optionsargument. Whenwrite_optionsis omitted, this setsself._write_optionsback toNone, and later attribute access (for exampleself._write_options.write_type) will fail.
self._point_settings = point_settings if point_settings is not None else PointSettings()
self._write_options = write_options if write_options is not None else WriteOptions()
self._write_options = write_options
# TODO - callbacks seem to be used with batching type only - could they be used with sync or async?
self._success_callback = kwargs.get('success_callback', None)
self._error_callback = kwargs.get('error_callback', None)
self._retry_callback = kwargs.get('retry_callback', None)
if self._write_options.write_type is WriteType.batching:
self._subject, self._disposable = self._create_batching_pipeline()
influxdb_client_3/write_client/client/write_api.py:1155
WriteApi.__getstate__deletes_write_service, but this attribute is no longer defined onWriteApi. Pickling an instance will raiseKeyErrorduring__getstate__.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
tests/test_write_api.py:253
- This call passes
organdbucketpositionally in the wrong order forWriteApi.write(bucket, org, record, ...), which can make the test exercise the wrong query parameters.
tests/test_write_api.py:295 - This call also swaps the positional
bucket/orgarguments forWriteApi.write(bucket, org, record, ...), which makes the test send org=bucket and bucket=org.
| try: | ||
| return await self.call_api( | ||
| resource_path=path, | ||
| method='POST', | ||
| query_params=query_params, | ||
| header_params=header_params, | ||
| body=body, | ||
| async_req=local_var_params.get('async_req'), | ||
| _request_timeout=local_var_params.get('_request_timeout'), | ||
| urlopen_kw=kwargs.get('urlopen_kw', None)) | ||
| except ApiException as e: | ||
| raise self._translate_write_exception(e, use_v2_api) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:253
WriteApi.write()signature is(bucket, org, record, ...), but this test passes("TEST_ORG", "TEST_BUCKET", ...), swapping bucket and org. This makes the test validate the wrong request parameters and may mask real issues.
tests/test_write_api.py:294- This timeout test also swaps positional
bucket/orgarguments (write("TEST_ORG", "TEST_BUCKET", ...)). With the currentWriteApi.write(bucket, org, record, ...)API, the correct order is bucket first, then org.
| try: | ||
| _HAS_DATACLASS = True | ||
| except ModuleNotFoundError: | ||
| _HAS_DATACLASS = False |
| self._point_settings = point_settings if point_settings is not None else PointSettings() | ||
| self._write_options = write_options if write_options is not None else WriteOptions() | ||
|
|
||
| self._write_options = write_options | ||
| # TODO - callbacks seem to be used with batching type only - could they be used with sync or async? |
| """Set your object with the provided dict.""" | ||
| self.__dict__.update(state) | ||
| # Init Rx | ||
| self.__init__(self._influxdb_client, | ||
| self._write_options, | ||
| self._point_settings, | ||
| self.__init__(token=self.token, | ||
| bucket=self.bucket, | ||
| org=self.org, |
| # self.host = 'http://localhost:9000' | ||
| # self.token = 'apiv3_YFw5WFYo7m0m2A1dMV8J9SIViuPzTlvrGlF3kaQARM8efwjjdX7mrUJB6sEBC-ZIhIUr_VgeOGB96we0VtkYkw' | ||
| # self.database = 'bucket0' |
| def close(self): | ||
| """Close the client and clean up resources.""" | ||
| self._write_api.close() | ||
| self._query_api.close() | ||
| self._client.close() | ||
|
|
Closes #216
Proposed Changes
Summarize
Removed features:
Checklist