Support passing name filter to ListStores#586
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change adds support for filtering stores by name in the ListStores API across Go, Java, and JavaScript client SDKs. It introduces a new name filter option, updates method signatures, modifies client and test code, and updates the OpenAPI reference to ensure compatibility with the new filter parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientSDK (Go/Java/JS)
participant API
User->>ClientSDK (Go/Java/JS): ListStores({ name: "My Store" })
ClientSDK (Go/Java/JS)->>API: GET /stores?name=My%20Store
API-->>ClientSDK (Go/Java/JS): ListStoresResponse (filtered)
ClientSDK (Go/Java/JS)-->>User: Filtered list of stores
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (9)
config/clients/java/template/src/main/api/configuration/ClientListStoresOptions.java.mustache (1)
40-47: Optional: add brief Javadoc for name filter semanticsA one-liner noting whether matching is exact/prefix/substring (per API behavior) will help users.
config/clients/go/template/example/example1/example1.go (1)
71-80: LGTM; minor consistency nit on field accessTo match earlier usage (Line 50) and avoid mixing styles, prefer GetStores() when printing the count.
- fmt.Printf("Stores with name filter Count: %d\n", len(storesWithFilter.Stores)) + fmt.Printf("Stores with name filter Count: %d\n", len(storesWithFilter.GetStores()))config/clients/java/template/src/test/api/OpenFgaApiTest.java.mustache (1)
101-123: Good addition; please also cover URL-encoding and mixed query paramsThis verifies name=... works. Consider:
- Add a case where name contains spaces/special chars to ensure proper encoding.
- Add a case combining pageSize, continuationToken, and name to verify query construction (and a stable param order if the mock requires exact string matching).
Example (encoding-focused) snippet you could add:
@Test public void listStoresTest_withNameFilter_specialChars() throws Exception { String storeName = "team work+demo"; String encoded = java.net.URLEncoder.encode(storeName, java.nio.charset.StandardCharsets.UTF_8); String responseBody = String.format("{\"stores\":[{\"id\":\"%s\",\"name\":\"%s\"}]}", DEFAULT_STORE_ID, storeName); mockHttpClient.onGet("https://api.fga.example/stores?name=" + encoded).doReturn(200, responseBody); var response = fga.listStores(null, null, storeName).get(); mockHttpClient.verify().get("https://api.fga.example/stores?name=" + encoded).called(1); assertEquals(storeName, response.getData().getStores().get(0).getName()); }config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache (1)
288-308: Solid coverage for name filter; add mixed-params and encoding cases
- Add a test combining name with page_size and continuation_token (ensures the client merges all query params correctly).
- Add an encoding test (e.g., spaces, plus, slash) to ensure URL encoding matches expectations.
Note: if the mock requires exact query order, ensure the client uses a deterministic order or adjust assertions accordingly.
Makefile (1)
247-250: CI: shellcheck target failing (acknowledged as unrelated in PR)Non-blocking here, but if you want, I can help by running shellcheck locally and proposing fixes or suppressions in a follow-up PR.
config/clients/go/template/client/client_test.mustache (1)
210-256: Good test for name filter; consider combo and encoding edge cases
- Add a combined options test to ensure name + page_size + continuation_token are all sent.
- Add an encoding test (e.g., “Team A+B/Trial”) and assert req.URL.Query().Get("name") matches the expected literal decoded value (httpmock compares decoded query params) and that page_size/continuation_token are present.
Example outline:
t.Run("ListStoresWithNameAndOptions", func(t *testing.T) { filtered := "Team A+B/Trial" test := TestDefinition{ /* ... */ } var expectedResponse openfga.ListStoresResponse _ = json.Unmarshal([]byte(test.JsonResponse), &expectedResponse) httpmock.Activate() defer httpmock.DeactivateAndReset() httpmock.RegisterResponder(test.Method, fmt.Sprintf("%s/stores", fgaClient.GetConfig().ApiUrl), func(req *http.Request) (*http.Response, error) { q := req.URL.Query() if q.Get("name") != filtered { t.Fatalf("expected name=%q, got %q", filtered, q.Get("name")) } if q.Get("page_size") != "5" || q.Get("continuation_token") != "ctok" { t.Fatalf("missing/invalid page_size or continuation_token") } return httpmock.NewJsonResponse(test.ResponseStatus, expectedResponse) }) opts := ClientListStoresOptions{ Name: openfga.PtrString(filtered), PageSize: openfga.PtrInt32(5), ContinuationToken: openfga.PtrString("ctok"), } _, err := fgaClient.ListStores(context.Background()).Options(opts).Execute() if err != nil { t.Fatalf("%v", err) } })config/clients/js/template/tests/helpers/nocks.ts.mustache (1)
59-66: Support query params robustly by stripping undefined keys before matchingIf any callers pass { name: undefined } (or similar), nock’s strict .query(obj) will fail. Filter keys first to avoid brittle tests.
Apply within this hunk:
- ) => { - const mock = nock(basePath).get("/stores"); - if (queryParams) { - mock.query(queryParams); - } - return mock.reply(responseCode, response); - }, + ) => { + const mock = nock(basePath).get("/stores"); + if (queryParams) { + const qp = Object.fromEntries( + Object.entries(queryParams).filter(([, v]) => v !== undefined) + ); + if (Object.keys(qp).length) { + mock.query(qp); + } + } + return mock.reply(responseCode, response); + },config/clients/js/template/client.mustache (1)
119-119: Avoid leaking endpoint-specific fields into a shared PaginationOptionsAdding
nameto the globally reusedPaginationOptionscouples unrelated endpoints to a ListStores-specific filter. Prefer a dedicated type to keep semantics clean.Example:
-export type PaginationOptions = { pageSize?: number, continuationToken?: string, name?: string; }; +export type PaginationOptions = { pageSize?: number, continuationToken?: string }; +export type ListStoresOptions = ClientRequestOpts & PaginationOptions & { name?: string };And update the signature accordingly:
-async listStores(options: ClientRequestOptsWithAuthZModelId & PaginationOptions = {}): PromiseResult<ListStoresResponse> { - return this.api.listStores(options.pageSize, options.continuationToken, options.name, options); +async listStores(options: ListStoresOptions = {}): PromiseResult<ListStoresResponse> { + return this.api.listStores(options.pageSize, options.continuationToken, options.name, options); }config/clients/go/template/client/client.mustache (1)
607-618: Wires pagination and name filters correctly into the request builderConditional application of
PageSize,ContinuationToken, andNameis correct and idiomatic for the generated request pattern.Optional: for stylistic consistency with other methods using pagination helpers, you could reuse
ClientPaginationOptionsand existing getters to reduce duplicate patterns, but not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Makefile(1 hunks)config/clients/go/template/README_calling_api.mustache(1 hunks)config/clients/go/template/client/client.mustache(3 hunks)config/clients/go/template/client/client_test.mustache(2 hunks)config/clients/go/template/example/example1/example1.go(1 hunks)config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache(2 hunks)config/clients/java/template/src/main/api/configuration/ClientListStoresOptions.java.mustache(2 hunks)config/clients/java/template/src/test-integration/api/OpenFgaApiIntegrationTest.java.mustache(2 hunks)config/clients/java/template/src/test/api/OpenFgaApiTest.java.mustache(5 hunks)config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache(1 hunks)config/clients/js/template/client.mustache(2 hunks)config/clients/js/template/tests/client.test.ts.mustache(1 hunks)config/clients/js/template/tests/helpers/nocks.ts.mustache(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Tests
Makefile
[error] Makefile target 'shellcheck' failed with exit code 1.
🔇 Additional comments (13)
config/clients/java/template/src/main/api/configuration/ClientListStoresOptions.java.mustache (1)
10-10: Add name filter option: LGTMField and fluent setter/getter align with the new API filter and existing style in this class. No concerns.
Also applies to: 40-47
config/clients/java/template/src/test-integration/api/OpenFgaApiIntegrationTest.java.mustache (1)
78-78: Test signature update for listStores: LGTMPassing null for the new optional name param maintains previous behavior and keeps tests backward compatible.
Also applies to: 109-109
config/clients/js/template/tests/client.test.ts.mustache (1)
103-121: Add test for name filter on ListStores: LGTMCovers query param propagation and response handling. Scope assertions ensure the filter was applied.
config/clients/java/template/src/test/api/OpenFgaApiTest.java.mustache (2)
89-89: Signature update LGTMPassing null as the third arg (name) aligns with the updated method signature and preserves the no-filter path.
135-136: Error-path tests updated for the new signatureUsing the third null parameter keeps these cases consistent with the new API.
Also applies to: 158-159, 180-181
Makefile (1)
3-3: OpenAPI ref bumped; ‘name’ query parameter for GET /stores is confirmed presentVerified that the OpenAPI spec at commit
e53c69cc55317404d02a6d8e418d626268f28a59includes an optionalnamequery parameter for the GET /stores endpoint:
- Check returned
true.- Parameter schema:
{ "name": "name", "description": "The name parameter instructs the API to only include results that match that name. Multiple results may be returned. Only exact matches will be returned; substring matches and regexes will not be evaluated", "in": "query", "required": false, "type": "string" }No further changes required.
config/clients/go/template/client/client_test.mustache (1)
15-15: Import alias for root package is clearUsing the openfga alias improves readability and avoids symbol conflicts with the client package.
config/clients/js/template/client.mustache (2)
303-303: JSDoc addition for name filter looks goodDocstring clearly describes the new
namefilter.
311-311: Parameter order confirmed in client.mustacheThe call
return this.api.listStores( options.pageSize, options.continuationToken, options.name, options );matches the generated OpenAPI signature
(pageSize, continuationToken, name, options). No inconsistencies were found in the template or accompanying examples/tests.config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache (2)
86-89: Correctly threads name filter through to the API
options.getName()is inserted beforeoverrides, matching the new signature and preserving headers handling.
75-78: Ignore the override‐parameter suggestion for the no‐arg listStoresThe no-arg
listStores()method intentionally invokes the three-argument overload
api.listStores(null, null, null), which matches the generatedOpenFgaApi#listStores(pageSize, continuationToken, name)signature (as confirmed by the existing raw‐API tests). Configuration overrides are only applied when options are provided. No changes are required.Likely an incorrect or invalid review comment.
config/clients/go/template/client/client.mustache (2)
169-174: Continuation token helper is fineNo behavior change; helper remains consistent with the page size accessor.
580-584: Add Name to ClientListStoresOptions: LGTMOptional
Namewithjson:"name,omitempty"is correct and matches query param usage.
2 participants
My original idea was to address this only for the Go SDK to support passing the name filter but it had cascading effects due to having to update the OpenAPI schema which is used by all clients (ofc).
Hence, this PR is updating the schema and making sure that all clients are compatible.
See related issue here: openfga/go-sdk#186
The failure for shellcheck seems unrelated to this PR and is also happening on main. To not make the scope of this PR even larger I've not addressed that here.
Here is the follow up PR for the Go SDK which is what I originally fixed: openfga/go-sdk#213
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores