[Typescript] DGS-23476: Allow id-pool header to be optional for union-of-pools#452
[Typescript] DGS-23476: Allow id-pool header to be optional for union-of-pools#452Tobi Ogunbi (tobiogunbi) merged 1 commit intomasterfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the bearer authentication functionality for Schema Registry by making the identityPoolId parameter optional and allowing it to accept an array of pool IDs for union-of-pools scenarios. Previously, identityPoolId was required and only accepted a single string value.
Changes:
- Modified
identityPoolIdtype fromstringtostring | string[]inBearerAuthCredentialsinterface - Made
identityPoolIdparameter optional (no longer required for bearer auth) - Added logic to join array of pool IDs into comma-separated header value
- Added comprehensive test coverage for single pool, multiple pools (array), and omitted pool scenarios
- Updated example documentation to clarify comma-separated format
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| schemaregistry/rest-service.ts | Modified type signature and handling logic for identityPoolId to support optional string or array of strings |
| schemaregistry/test/rest-service.spec.ts | Added three new test cases covering single pool, array of pools, and omitted pool scenarios |
| schemaregistry-examples/src/constants.ts | Updated comment to clarify comma-separated format for union-of-pools |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const poolId = Array.isArray(bearerAuthCredentials.identityPoolId) | ||
| ? bearerAuthCredentials.identityPoolId.join(',') | ||
| : bearerAuthCredentials.identityPoolId; | ||
|
|
||
| if (poolId) { | ||
| headers['Confluent-Identity-Pool-Id'] = poolId; | ||
| } |
There was a problem hiding this comment.
Empty array case is not handled properly. When identityPoolId is an empty array, the join operation will produce an empty string, which is a falsy value and will prevent the header from being set. However, this behavior may be misleading since an empty array is semantically different from an undefined/omitted value. Consider adding explicit validation to reject empty arrays or handle them distinctly.
| describe('RestService Bearer Auth', () => { | ||
| let mock: InstanceType<typeof MockAdapter>; | ||
|
|
||
| beforeEach(() => { | ||
| mock = new MockAdapter(axios); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| mock.reset(); | ||
| }); | ||
|
|
||
| it('should set Confluent-Identity-Pool-Id header when identityPoolId is provided', async () => { | ||
| const bearerAuth: BearerAuthCredentials = { | ||
| credentialsSource: 'STATIC_TOKEN', | ||
| token: 'my-token', | ||
| logicalCluster: 'lsrc-abc123', | ||
| identityPoolId: 'pool-Gx30', | ||
| }; | ||
|
|
||
| const restService = new RestService( | ||
| ['http://localhost'], false, {}, undefined, bearerAuth, | ||
| maxRetries, retriesWaitMs, retriesMaxWaitMs | ||
| ); | ||
|
|
||
| mock.onGet('/subjects').reply(200, ['subject1']); | ||
| await restService.handleRequest('/subjects', 'GET'); | ||
|
|
||
| const requestHeaders = mock.history.get[0].headers; | ||
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBe('pool-Gx30'); | ||
| expect(requestHeaders?.['target-sr-cluster']).toBe('lsrc-abc123'); | ||
| expect(requestHeaders?.['Authorization']).toBe('Bearer my-token'); | ||
| }); | ||
|
|
||
| it('should join array of identityPoolIds into comma-separated header', async () => { | ||
| const bearerAuth: BearerAuthCredentials = { | ||
| credentialsSource: 'STATIC_TOKEN', | ||
| token: 'my-token', | ||
| logicalCluster: 'lsrc-abc123', | ||
| identityPoolId: ['pool-1', 'pool-2', 'pool-3'], | ||
| }; | ||
|
|
||
| const restService = new RestService( | ||
| ['http://localhost'], false, {}, undefined, bearerAuth, | ||
| maxRetries, retriesWaitMs, retriesMaxWaitMs | ||
| ); | ||
|
|
||
| mock.onGet('/subjects').reply(200, ['subject1']); | ||
| await restService.handleRequest('/subjects', 'GET'); | ||
|
|
||
| const requestHeaders = mock.history.get[0].headers; | ||
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBe('pool-1,pool-2,pool-3'); | ||
| }); | ||
|
|
||
| it('should not set Confluent-Identity-Pool-Id header when identityPoolId is omitted', async () => { | ||
| const bearerAuth: BearerAuthCredentials = { | ||
| credentialsSource: 'STATIC_TOKEN', | ||
| token: 'my-token', | ||
| logicalCluster: 'lsrc-abc123', | ||
| }; | ||
|
|
||
| const restService = new RestService( | ||
| ['http://localhost'], false, {}, undefined, bearerAuth, | ||
| maxRetries, retriesWaitMs, retriesMaxWaitMs | ||
| ); | ||
|
|
||
| mock.onGet('/subjects').reply(200, ['subject1']); | ||
| await restService.handleRequest('/subjects', 'GET'); | ||
|
|
||
| const requestHeaders = mock.history.get[0].headers; | ||
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBeUndefined(); | ||
| expect(requestHeaders?.['target-sr-cluster']).toBe('lsrc-abc123'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for edge case where identityPoolId is an empty array. This should either be tested to ensure it behaves correctly (header not set) or validation should be added to reject empty arrays with a clear error message.
| const oauthClientSecret = '<your-client-secret>'; | ||
| const scope = '<your-scope>'; // e.g. 'schemaregistry'; | ||
| const identityPoolId = '<your-pool>'; // e.g. pool-Gx30 | ||
| const identityPoolId = '<your-pool>'; // single pool e.g. 'pool-Gx30', or comma-separated for union-of-pools e.g. 'pool-Gx30,pool-Ab12' |
There was a problem hiding this comment.
The comment suggests using comma-separated string for union-of-pools (e.g., 'pool-Gx30,pool-Ab12'), but the code now accepts an array of strings (e.g., ['pool-Gx30', 'pool-Ab12']). The comment should mention both options: passing a single string, an array of strings for union-of-pools, or a comma-separated string. Update the comment to clarify that arrays are the preferred approach.
| const identityPoolId = '<your-pool>'; // single pool e.g. 'pool-Gx30', or comma-separated for union-of-pools e.g. 'pool-Gx30,pool-Ab12' | |
| const identityPoolId = '<your-pool>'; // single pool e.g. 'pool-Gx30'; for union-of-pools prefer an array of strings e.g. ['pool-Gx30', 'pool-Ab12'], or use a comma-separated string e.g. 'pool-Gx30,pool-Ab12' |
| retriesMaxWaitMs?: number, | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank line added. This appears to be accidental whitespace that should be removed for consistency with the rest of the file.
| ? bearerAuthCredentials.identityPoolId.join(',') | ||
| : bearerAuthCredentials.identityPoolId; |
There was a problem hiding this comment.
Missing validation for array elements. If the identityPoolId array contains empty strings, null, or undefined values, these will be joined into the header value (e.g., ['pool-1', '', 'pool-3'] becomes 'pool-1,,pool-3'). Consider filtering out falsy values or validating that all array elements are non-empty strings before joining.
| ? bearerAuthCredentials.identityPoolId.join(',') | |
| : bearerAuthCredentials.identityPoolId; | |
| ? bearerAuthCredentials.identityPoolId | |
| .filter((id): id is string => typeof id === 'string' && id.trim().length > 0) | |
| .join(',') | |
| : (typeof bearerAuthCredentials.identityPoolId === 'string' | |
| && bearerAuthCredentials.identityPoolId.trim().length > 0 | |
| ? bearerAuthCredentials.identityPoolId | |
| : undefined); |
| const requestHeaders = mock.history.get[0].headers; | ||
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBeUndefined(); | ||
| expect(requestHeaders?.['target-sr-cluster']).toBe('lsrc-abc123'); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for edge cases with invalid array elements such as empty strings, whitespace-only strings, or arrays containing null/undefined values. Tests should verify that the implementation either handles these gracefully or rejects them with clear error messages.
| }); | |
| }); | |
| it('should handle identityPoolId array containing empty strings', async () => { | |
| const bearerAuth: BearerAuthCredentials = { | |
| credentialsSource: 'STATIC_TOKEN', | |
| token: 'my-token', | |
| logicalCluster: 'lsrc-abc123', | |
| identityPoolId: ['pool-1', '', 'pool-3'], | |
| }; | |
| const restService = new RestService( | |
| ['http://localhost'], false, {}, undefined, bearerAuth, | |
| maxRetries, retriesWaitMs, retriesMaxWaitMs | |
| ); | |
| mock.onGet('/subjects').reply(200, ['subject1']); | |
| await restService.handleRequest('/subjects', 'GET'); | |
| const requestHeaders = mock.history.get[0].headers; | |
| // Document current behavior: empty string elements result in consecutive commas. | |
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBe('pool-1,,pool-3'); | |
| }); | |
| it('should handle identityPoolId array containing whitespace-only strings', async () => { | |
| const bearerAuth: BearerAuthCredentials = { | |
| credentialsSource: 'STATIC_TOKEN', | |
| token: 'my-token', | |
| logicalCluster: 'lsrc-abc123', | |
| identityPoolId: ['pool-1', ' ', 'pool-3'], | |
| }; | |
| const restService = new RestService( | |
| ['http://localhost'], false, {}, undefined, bearerAuth, | |
| maxRetries, retriesWaitMs, retriesMaxWaitMs | |
| ); | |
| mock.onGet('/subjects').reply(200, ['subject1']); | |
| await restService.handleRequest('/subjects', 'GET'); | |
| const requestHeaders = mock.history.get[0].headers; | |
| // Document current behavior: whitespace-only elements are preserved as-is in the joined string. | |
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBe('pool-1, ,pool-3'); | |
| }); | |
| it('should handle identityPoolId array containing null values', async () => { | |
| const bearerAuth: BearerAuthCredentials = { | |
| credentialsSource: 'STATIC_TOKEN', | |
| token: 'my-token', | |
| logicalCluster: 'lsrc-abc123', | |
| identityPoolId: ['pool-1', null as unknown as string, 'pool-3'], | |
| }; | |
| const restService = new RestService( | |
| ['http://localhost'], false, {}, undefined, bearerAuth, | |
| maxRetries, retriesWaitMs, retriesMaxWaitMs | |
| ); | |
| mock.onGet('/subjects').reply(200, ['subject1']); | |
| await restService.handleRequest('/subjects', 'GET'); | |
| const requestHeaders = mock.history.get[0].headers; | |
| // Document current behavior: null elements are treated as empty strings when joined. | |
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBe('pool-1,,pool-3'); | |
| }); | |
| it('should handle identityPoolId array containing undefined values', async () => { | |
| const bearerAuth: BearerAuthCredentials = { | |
| credentialsSource: 'STATIC_TOKEN', | |
| token: 'my-token', | |
| logicalCluster: 'lsrc-abc123', | |
| identityPoolId: ['pool-1', undefined as unknown as string, 'pool-3'], | |
| }; | |
| const restService = new RestService( | |
| ['http://localhost'], false, {}, undefined, bearerAuth, | |
| maxRetries, retriesWaitMs, retriesMaxWaitMs | |
| ); | |
| mock.onGet('/subjects').reply(200, ['subject1']); | |
| await restService.handleRequest('/subjects', 'GET'); | |
| const requestHeaders = mock.history.get[0].headers; | |
| // Document current behavior: undefined elements are treated as empty strings when joined. | |
| expect(requestHeaders?.['Confluent-Identity-Pool-Id']).toBe('pool-1,,pool-3'); | |
| }); |
Please prefix all TypeScript pull-requests with
[Typescript]What
Checklist
References
JIRA:
Test & Review
Open questions / Follow-ups