feat: add streamedListObjects for unlimited object retrieval #654
Conversation
WalkthroughThis PR enables streaming support for the StreamedListObjects endpoint in the JavaScript SDK generator by adding a configuration flag, implementing an NDJSON parser, conditionally updating API method signatures to support streaming return types, and documenting the new streaming API functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OpenFgaClient
participant Factory as {{classname}}Factory
participant ParamCreator as {{classname}}AxiosParamCreator
participant RequestFn as Request Function
participant Stream as Node.js Stream
Client->>Factory: Call streamedListObjects(params)
alt x-fga-streaming flag enabled
Factory->>ParamCreator: Call (async)
ParamCreator->>RequestFn: Return (axios) => Promise<any>
ParamCreator-->>Factory: via createStreamingRequestFunction
Factory->>RequestFn: Invoke with axios instance
RequestFn->>Stream: Return streaming response
Stream->>Client: AsyncIterable<parsed JSON objects>
else x-fga-streaming flag disabled
Factory->>ParamCreator: Call (async)
ParamCreator->>RequestFn: Return (axios) => Promise<CallResult<ReturnType>>
ParamCreator-->>Factory: via createRequestFunction
Factory->>RequestFn: Invoke with axios instance
RequestFn->>Client: Promise<CallResult<ReturnType>>
end
sequenceDiagram
participant App as Application
participant Parser as parseNDJSONStream
participant Decoder as UTF-8 Decoder
participant Buffer as Internal Buffer
participant JSON as JSON Parser
App->>Parser: Pass Readable stream / AsyncIterable / string
alt Input is string or Buffer
Parser->>JSON: Split by newline and parse each line
JSON-->>App: Yield parsed JSON objects
else Input is stream-like
Parser->>Decoder: Iterate chunks, decode UTF-8
Decoder->>Buffer: Accumulate into buffer
Buffer->>JSON: Split buffer by newline, parse complete lines
JSON->>App: Yield parsed JSON objects
Parser->>Buffer: On stream end, flush and parse remaining
Buffer->>JSON: Parse final buffer content
JSON->>App: Yield final JSON objects
end
alt Error during iteration
Parser-->>App: Reject with error, cleanup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/clients/js/template/apiInner.mustache (1)
285-285: Critical: Interface and implementation return type mismatch for streaming operations.The interface method signature always declares a return type of
PromiseResult<ReturnType>, but the class implementation (lines 351, 356) returnsPromise<any>whenx-fga-streamingis enabled. This creates a type mismatch that will cause issues when streaming operations are used through the interface.Apply this diff to add conditional return types to the interface:
- {{nickname}}({{#allParams}}{{paramName}}{{^required}}?{{/required}}: {{{dataType}}}, {{/allParams}}options?: any): PromiseResult<{{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}void{{/returnType}}>; + {{nickname}}({{#allParams}}{{paramName}}{{^required}}?{{/required}}: {{{dataType}}}, {{/allParams}}options?: any): {{#vendorExtensions}}{{#x-fga-streaming}}Promise<any>{{/x-fga-streaming}}{{^x-fga-streaming}}PromiseResult<{{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}void{{/returnType}}>{{/x-fga-streaming}}{{/vendorExtensions}}{{^vendorExtensions}}PromiseResult<{{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}void{{/returnType}}>{{/vendorExtensions}};
🧹 Nitpick comments (5)
config/clients/js/template/api.mustache (1)
13-14: GatecreateStreamingRequestFunctionimport to avoid unused-imports when no streamed endpoints are present.If a spec has no
x-fga-streamingpaths, this import may be unused and trigger TS/Lint noise. Gate it with the same flag you use for docs/templates.Apply:
import { DUMMY_BASE_URL, setSearchParams, serializeDataIfNeeded, toPathString, createRequestFunction, - createStreamingRequestFunction, +{{#supportsStreamedListObjects}} + createStreamingRequestFunction, +{{/supportsStreamedListObjects}} RequestArgs, CallResult, PromiseResult } from './common';Please generate a JS client from a spec without
streamed-list-objectsand confirm there are no unused-import warnings in the generatedapi.ts.config/clients/js/config.overrides.json (1)
12-12: Confirm versioning policy; consider a minor bump.New public surface (streaming) warrants a semver minor bump. Ensure
packageVersionhere and the base config remain consistent.Please confirm/update
packageVersionand cross-checkconfig/common/config.base.jsonfor consistency. Based on coding guidelines.config/clients/js/template/README_api_endpoints.mustache (1)
17-18: Consider consistent labelling withListObjects.
ListObjectsis tagged “[EXPERIMENTAL]”; consider mirroring that tag forStreamedListObjects, or remove fromListObjectsif the status changed.config/clients/js/template/streaming.mustache (1)
118-129: Remove unnecessary encode/decode for string chunks; append directly.Avoid
TextEncoderand double-decoding for string chunks.Apply:
- for await (const chunk of source) { - // Node.js streams can return Buffer or string chunks - // Convert to Uint8Array if needed for TextDecoder - const uint8Chunk = typeof chunk === "string" - ? new TextEncoder().encode(chunk) - : chunk instanceof Buffer - ? new Uint8Array(chunk) - : chunk; - - // Append decoded chunk to buffer - buffer += decoder.decode(uint8Chunk, { stream: true }); + for await (const chunk of source) { + if (typeof chunk === "string") { + buffer += chunk; + } else { + const uint8Chunk = chunk instanceof Buffer ? new Uint8Array(chunk) : (chunk as Uint8Array); + buffer += decoder.decode(uint8Chunk, { stream: true }); + }config/clients/js/template/apiInner.mustache (1)
213-213: Consider improving type safety for streaming return types.Streaming operations return
Promise<any>, which loses type safety compared to the typedPromiseResultorPromise<CallResult<ReturnType>>used for non-streaming operations. While streaming return types can be complex, consider defining a more specific type such asPromise<ReadableStream>orPromise<AsyncIterable<T>>to provide better type information and improve developer experience.Also applies to: 256-256, 351-351, 356-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/clients/js/config.overrides.json(2 hunks)config/clients/js/template/README_api_endpoints.mustache(1 hunks)config/clients/js/template/README_calling_api.mustache(1 hunks)config/clients/js/template/api.mustache(1 hunks)config/clients/js/template/apiInner.mustache(4 hunks)config/clients/js/template/index.mustache(1 hunks)config/clients/js/template/streaming.mustache(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
config/**/*.mustache
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Validate mustache syntax and variable references across all template files, including CHANGELOG.md.mustache
Files:
config/clients/js/template/api.mustacheconfig/clients/js/template/streaming.mustacheconfig/clients/js/template/index.mustacheconfig/clients/js/template/README_calling_api.mustacheconfig/clients/js/template/apiInner.mustacheconfig/clients/js/template/README_api_endpoints.mustache
config/**/*.{json,mustache}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never hardcode API keys or credentials in configuration or template files
Files:
config/clients/js/template/api.mustacheconfig/clients/js/template/streaming.mustacheconfig/clients/js/template/index.mustacheconfig/clients/js/template/README_calling_api.mustacheconfig/clients/js/config.overrides.jsonconfig/clients/js/template/apiInner.mustacheconfig/clients/js/template/README_api_endpoints.mustache
config/clients/*/config.overrides.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
config/clients/*/config.overrides.json: Always update the packageVersion in each language-specific config.overrides.json when making version changes
Maintain FOSSA compliance notice IDs in each language’s config.overrides.json
Files:
config/clients/js/config.overrides.json
config/{common/config.base.json,clients/*/config.overrides.json}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure consistent versioning across base and language override configuration files to avoid version conflicts
Files:
config/clients/js/config.overrides.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-test-java-sdk
- GitHub Check: build-and-test-go-sdk
- GitHub Check: build-and-test-dotnet-sdk
🔇 Additional comments (5)
config/clients/js/template/index.mustache (1)
16-16: Export looks good; verify browser builds still type-check.
streaming.tsimports Node types (type-only). Ensure consumers without@types/node(browser-only) don’t fail type-checks.Run a browser-target TS build (no
@types/node) against the generated SDK and confirm type-check passes when not importingparseNDJSONStream.config/clients/js/config.overrides.json (1)
28-31: Supporting file mapping forstreaming.mustachelooks correct.Generates
streaming.tsvia SupportingFiles — aligned with the new export inindex.mustache.config/clients/js/template/README_calling_api.mustache (1)
469-490: Current code is correct; review comment is based on incorrect assumptions about the SDK.The JavaScript template correctly uses the async iterable API. Both the Python example and the web-verified JavaScript SDK v1 behavior confirm that
streamedListObjectsreturns an async iterable—not a raw stream—where each response contains anobjectfield. TheparseNDJSONStreamutility does not exist in this codebase, and the current pattern (directfor awaititeration) matches the actual SDK design across all languages. No changes needed.Likely an incorrect or invalid review comment.
config/clients/js/template/apiInner.mustache (2)
11-11: LGTM: Import addition for streaming support.The
createStreamingRequestFunctionimport is correctly added and used conditionally throughout the template when thex-fga-streamingvendor extension is present.
213-230: Code duplication and telemetry gap for streaming confirmed—verify intentionality.The observations are valid:
Code duplication confirmed (lines 217-221 and 223-227): Identical storeId and request body telemetry attributes appear in two separate conditional blocks—one for
{{#vendorExtensions}}{{^x-fga-streaming}}and another for{{^vendorExtensions}}.Telemetry gap for streaming confirmed: When
x-fga-streamingis true, the{{^x-fga-streaming}}condition is false, so storeId and body telemetry are omitted. Streaming operations only include the request method in telemetry.Before refactoring, verify:
- Is the omission of storeId and body telemetry for streaming operations intentional or an oversight?
- Are there operational or performance reasons streaming should exclude this telemetry?
- Is this template manually maintained or auto-generated?
Once intentionality is confirmed, the duplication can be refactored to improve maintainability.
Adds NDJSON streaming parser template to support streamedListObjects in the JavaScript SDK. Templates provide parsing utilities; actual streaming logic remains in custom code (client.ts, common.ts) maintained in js-sdk repository. Templates Added/Modified (5 files): - streaming.mustache (NEW): NDJSON parser for Node.js streams - Proper error propagation (reject pending promises on error) - onEnd guard prevents processing after error - Uint8Array handling alongside string/Buffer - Stream destruction in return()/throw() methods - Widened type signature for better DX - index.mustache: Export parseNDJSONStream utility - config.overrides.json: Register streaming + supportsStreamedListObjects flag - README_calling_api.mustache: Usage documentation - README_api_endpoints.mustache: API endpoint table entry Architecture: - Templates generate utilities (streaming.ts, exports) - Custom js-sdk code implements streaming (common.ts, client.ts) - No OpenAPI spec changes required Generated & Verified: - streaming.ts includes all error handling fixes - parseNDJSONStream exported correctly - Works with custom js-sdk streaming implementation Related: - Fixes #76 (JavaScript SDK) - Implements openfga/js-sdk#236 - Related PR: openfga/js-sdk#280
daniel-jonathan
force-pushed
the
feat/streaming-only
branch
from
fc55cb3 to
0053fe8
Compare
Adds streamedListObjects method for retrieving unlimited objects via the streaming API endpoint. Node.js-only implementation with resilient NDJSON parsing, proper error handling, and automatic resource cleanup. Requires OpenFGA server v1.2.0+ Features: - Streams beyond 1000-object limit (tested with 2000 objects) - Memory-efficient incremental results via async generators - Automatic stream cleanup prevents connection leaks - Proper error propagation through async iterators - Flexible input types (Readable|AsyncIterable|string|Buffer|Uint8Array) - Telemetry maintained through streaming request helper Implementation: - streaming.ts: NDJSON parser with robust error handling - common.ts: createStreamingRequestFunction for axios streaming - client.ts: streamedListObjects() async generator wrapper - Error handling: Pending promises reject on error, onEnd guarded - Resource management: Stream destruction in return()/throw()/finally - Type safety: Wide signature eliminates unnecessary casts Testing (153/153 tests passing): - 17 streaming tests (parsing, errors, cleanup, edge cases) - 95% coverage on streaming.ts - Live tested: 3-object and 2000-object streaming verified Examples: - example/streamed-list-objects: Full model with 2000 tuples - example/streamed-list-objects-local: Minimal local setup Related: - Fixes #236 - Parent issue: openfga/sdk-generator#76 - Related PR: openfga/sdk-generator#654 (templates)
|
|
||
| import type { Readable } from "node:stream"; | ||
|
|
||
| // Helper: create async iterable from classic EventEmitter-style Readable streams |
There was a problem hiding this comment.
I think we don't need this file anymore in the generator, since this would require reverse synching it back from the SDK, we can just do the changes in the SDK directly
Unless you're using something from the generator here
There was a problem hiding this comment.
https://github.com/openfga/js-sdk/blob/0c2a0ab16995585d98582f32efec8b82090a1249/streaming.ts
If this is using some config/constants that should come from the generator, maybe then we can continue having it here
There was a problem hiding this comment.
Good catch. I included it, so that all files would come from the generator. That said, looks like the need for the generator has been simplified and this can be removed.
Add documentation for the StreamedListObjects API to the List Objects guide. Changes: - Add Streamed List Objects section explaining streaming differences - Include Node.js and Python SDK examples - Add note about SDK availability - Add related link to StreamedListObjects API reference Related: - openfga/js-sdk#280 - openfga/sdk-generator#654 - openfga/sdk-generator#76
Summary
Adds NDJSON streaming parser template to support
streamedListObjectsin the JavaScript SDK. Templates provide parsing utilities; streaming implementation remains in custom code maintained in js-sdk repository.Fixes #76 (JavaScript SDK)
Fixes openfga/js-sdk#236
Changes
Templates Modified (5 files):
streaming.mustache(NEW): NDJSON parser for Node.js streams with error handlingindex.mustache: Export parseNDJSONStream utilityconfig.overrides.json: Register streaming template and feature flagREADME_calling_api.mustache: Usage documentationREADME_api_endpoints.mustache: API endpoint table entryArchitecture:
Testing
Generated SDK verified:
Related