Repository navigation
feat(clients): add Binary Store alpha APIs - #1329
AdepuSriCharan wants to merge 4 commits into
Conversation
Generate protobuf bindings from Dapr runtime v1.19.0-rc.4 (commit 1cdc46c820eede065522f80652c40304d668ea66) using grpcio-tools 1.81.1 and mypy-protobuf 5.0.0. Support immutable runtime refs and reliable generation failures without changing dependency floors. Signed-off-by: Adepu Sri Charan <sricharan.adepu36@gmail.com>
Add synchronous and asynchronous streaming file operations with overwrite, deadlines, authentication, examples and regression coverage. Signed-off-by: Adepu Sri Charan <sricharan.adepu36@gmail.com>
Signed-off-by: Adepu Sri Charan <sricharan.adepu36@gmail.com>
sicoyle
left a comment
There was a problem hiding this comment.
Thanks so much @AdepuSriCharan for your interest in the Dapr open source project and for putting this together! 🙌 The streaming logic is really careful: sequence validation, bounded reads, preserving local reader errors and never replaying are exactly what this API needs, and the test coverage is great to see 🧪. I left a few comments inline, please take a look. One more thing: the generated proto files aren't in this diff, so could you confirm they match the runtime commit you mention in the description?
Drafted with AI assistance and reviewed by me.
|
|
||
|
|
||
| class DaprClientInterceptor(UnaryUnaryClientInterceptor, StreamStreamClientInterceptor): | ||
| class DaprClientInterceptor( |
There was a problem hiding this comment.
DaprClientInterceptor is exported, and with this change it also implements StreamUnaryClientInterceptor and UnaryStreamClientInterceptor. That means existing unary-stream and stream-unary calls (configuration subscriptions, for example) will now get dapr-api-token and any other interceptor metadata as well.
I think that's probably the right behaviour, but it's an auth change for RPCs that already exist, so I'd like to be explicit about it. Did those calls go out without the token before this PR? And does any caller already attach dapr-api-token by hand on those paths? If so, the header would now be sent twice. Right now the single-token assertions only cover the binary-store and encrypt RPCs.
What do you think about adding a single-token assertion for at least one pre-existing unary-stream RPC, and calling out the behaviour change in the PR description so it ends up in the release notes?
| if settings.DAPR_API_TOKEN: | ||
| channel_interceptors.append( | ||
| DaprClientInterceptorAsync([('dapr-api-token', settings.DAPR_API_TOKEN)]) | ||
| metadata = [('dapr-api-token', settings.DAPR_API_TOKEN)] |
There was a problem hiding this comment.
Same idea on the async side: registering _DaprUnaryStreamInterceptorAsync and _DaprStreamUnaryInterceptorAsync whenever DAPR_API_TOKEN is set changes token handling for every async unary-stream and stream-unary RPC, not just binary store.
Which existing async RPCs previously sent no token? Could you list them in the description and add one regression test that checks the token is present exactly once on a pre-existing async server-streaming call? That way we lock in the new behaviour on purpose.
| except AioRpcError as error: | ||
| await self.aclose() | ||
| raise DaprGrpcError(error) from error | ||
| except BaseException: |
There was a problem hiding this comment.
When the underlying call is cancelled by something other than the caller's task (client.close(), another aclose(), and so on), grpc.aio raises asyncio.CancelledError from __anext__, and this branch re-raises it unchanged. The problem is that a CancelledError coming out of a task that wasn't itself cancelled gets treated by asyncio and TaskGroup as task cancellation, so it can quietly tear down unrelated work. The sync client raises DaprGrpcError(CANCELLED) in the same situation.
I think we could check whether the caller's own task is being cancelled, for example with asyncio.current_task().cancelling() or call.cancelled(). If it isn't, translate the error into a DaprGrpcError with status CANCELLED to match the sync client. If it is, re-raise CancelledError as you do now. test_client_shutdown_cancels_download would need updating to expect the new error.
| uv run pytest tests/examples/ | ||
|
|
||
| validate-binary-store: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
validate-complete only needs: [validate, validate-dapr-head], so a failing validate-binary-store job won't block merges through the required check. Could you add it to validate-complete.needs, along with a matching result check?
Also, the other jobs pass commit: to setup-dapr-runtime, but this one passes version: 1.19.0-rc.4. Does that action accept a version input? If it doesn't, the job may quietly run against the wrong runtime.
| async def test_streaming_round_trip(binary_store_client, size): | ||
| name = f'async/round-trip-{size}' | ||
| data = (bytes(range(256)) * (size // 256 + 1))[:size] | ||
| async with DaprClient(address='127.0.0.1:13501') as client: |
There was a problem hiding this comment.
These async tests connect to a hard-coded 127.0.0.1:13501 instead of the sidecar the binary_store_client fixture actually started. If the integration port allocation changes, or another module's sidecar holds that port, the tests will break or talk to the wrong sidecar.
What do you think about having the fixture expose the gRPC endpoint it started (or adding a small companion async fixture) and using that here?
| self._closed = True | ||
| self._buffer = b'' | ||
| if not self._eof: | ||
| self._stream.cancel() |
There was a problem hiding this comment.
nit: the sync BinaryFileResponse.close handles interceptors that return a plain generator without cancel(), but aclose here calls self._stream.cancel() unconditionally and would raise AttributeError in the same case. Could we mirror the sync fallback here? Use getattr(self._stream, 'cancel', None), and call aclose() on the async iterator when cancel isn't there.
| @@ -0,0 +1 @@ | |||
| dapr | |||
There was a problem hiding this comment.
nit: no published dapr release has set_binary_file_alpha1 yet, so anyone who runs pip install -r requirements.txt will hit an AttributeError when running the example. Could we pin to the first release that includes the API once we know it? Until then, a comment pointing to the in-repo install steps in the README would help readers 📝.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1329 +/- ##
==========================================
+ Coverage 87.27% 87.58% +0.30%
==========================================
Files 163 165 +2
Lines 13069 13406 +337
==========================================
+ Hits 11406 11741 +335
- Misses 1663 1665 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Adepu Sri Charan <sricharan.adepu36@gmail.com>
|
Thanks for the detailed review, @sicoyle! I appreciate the feedback. As per the feedback I have added regression tests for async cancellation, generator cleanup, and API-token handling on existing configuration streams. I also updated the CI validation gate, removed the hard-coded port from the integration tests, and clarified the example installation instructions. Regarding the generated protobuf files, #1327 already brought in the newer rc.5 bindings. I regenerated all 57 files and confirmed they match upstream exactly. I've updated the PR description to clarify this as well. All 2,148 non-e2e tests and 77 feature/integration/example tests passed locally, along with the Python 3.10 compatibility checks. |
Description
This PR adds support for the Binary Store alpha API in the Python SDK, allowing developers to upload, download, and delete files through Dapr.
It introduces
set_binary_file_alpha1,get_binary_file_alpha1, anddelete_binary_file_alpha1for both synchronous and asynchronous clients.Uploads support bytes and binary file-like objects, while downloads can be streamed in chunks without loading the entire file into memory. The implementation also handles overwrites, timeouts, authentication, cancellation, and resource cleanup.
I've added unit and integration tests, sync/async examples, documentation, and dedicated CI validation for Dapr 1.19.
Updates after review:
SubscribeConfigurationandSubscribeConfigurationAlpha1in both sync and async clients, without duplicating an identical caller-provided token.Generated protobuf files:
PR #1327 has already merged the Dapr v1.19.0-rc.5 generated bindings upstream, so they no longer appear as changes in this PR. I verified that regenerating all 57 files produces identical output. Integration CI uses the Dapr 1.19.0-rc.4 runtime.
Validation:
A corresponding documentation update is prepared for the Dapr 1.19 documentation branch.
Known limitation: An upstream gRPC issue ([grpc/grpc#36066](grpc/grpc#36066)) can occasionally return
INTERNALinstead of the original server error during early async upload failures. This behavior remains documented and covered by regression tests.Issue reference
Closes #1296
Checklist