refactor(platform)!: source the organization from the SDK instance - #691
refactor(platform)!: source the organization from the SDK instance#691vnaren23 wants to merge 4 commits into
Conversation
| expect(spec.params.userId).toBe(PLATFORM_TEST_CONSTANTS.USER_ID); | ||
| expect(spec.params).not.toHaveProperty('partitionGlobalId'); | ||
| // The SDK name never reaches the wire — only the API's own `partitionGlobalId` | ||
| expect(spec.params).not.toHaveProperty('organizationId'); |
There was a problem hiding this comment.
The test name says "alongside the organization" but the body only asserts userId is present and that the SDK alias organizationId doesn't leak to the wire. It never checks that partitionGlobalId itself IS included, so removing the partitionGlobalId: this.config.orgName line from getUserSettings would leave this test green.
The sibling test above ("should scope the read to the SDK's organization…") does verify the full param object, but the intent of this test — an "always" invariant covering both user and org — is incomplete without the positive assertion.
| expect(spec.params.userId).toBe(PLATFORM_TEST_CONSTANTS.USER_ID); | |
| expect(spec.params).not.toHaveProperty('partitionGlobalId'); | |
| // The SDK name never reaches the wire — only the API's own `partitionGlobalId` | |
| expect(spec.params).not.toHaveProperty('organizationId'); | |
| expect(spec.params.userId).toBe(PLATFORM_TEST_CONSTANTS.USER_ID); | |
| expect(spec.params.partitionGlobalId).toBe(PLATFORM_TEST_CONSTANTS.ORGANIZATION_ID); | |
| // The SDK name never reaches the wire — only the API's own `partitionGlobalId` | |
| expect(spec.params).not.toHaveProperty('organizationId'); |
|
Review summary: one finding posted this run — the test 'should always send userId alongside the organization' (platform.test.ts L141-143) doesn't assert that partitionGlobalId IS included, only that userId is present and the SDK alias is absent. Suggestion posted inline. Everything else looks good: BaseService.config typing is correct (Zod enforces orgName non-empty at SDK construction), the write-path test at L262-270 implicitly covers partitionGlobalId via Object.keys(), and the overall refactor is clean. |
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
kittyyueli
left a comment
There was a problem hiding this comment.
Is it possible to get the userId from the SDK instance as well? Or because it can be an external app, that's not possible?
Right, external app, backend usage and cli usage means we might not have user id at all times. |
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
Platform settings are scoped to an (organization, user) pair, but the organization was passed in per call — as an option on getUserSettings and a required positional argument on updateUserSettings. The SDK already knows which organization it was initialized against, so both methods now read it from the instance and callers only supply the user. BREAKING CHANGE: updateUserSettings() no longer takes an organizationId argument and getUserSettings() no longer takes an options argument. PlatformSettingGetOptions is removed. Both shipped in 1.6.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remove the integration-test organizationId config, orphaned when the Platform suite stopped needing it; drop the stale README row; stop the module @example deriving userId from a read result that may omit the key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restore the service class comment and drop the per-method scoping notes, so the module, service class, and ServiceModel comments all read as they did before. The signature change alone drives the remaining JSDoc edits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5d76286 to
be8d5c8
Compare
The organization is already on the request path — ApiClient builds
{baseUrl}/{orgName}/identity_/api/Setting — so sending it again as
partitionGlobalId on the read and in the body on the write is redundant.
Reverts the BaseService.config.orgName accessor added earlier on this
branch: Platform was its only consumer, and nothing reads it now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
Methods Changed
The organization is no longer a call-site argument on either method — both read it from the SDK instance they were constructed with.
platform.getUserSettings()getUserSettings(keys: PlatformSettingKey[], userId: string, options?: PlatformSettingGetOptions): Promise<PlatformSetting[]>getUserSettings(keys: PlatformSettingKey[], userId: string): Promise<PlatformSetting[]>platform.updateUserSettings()updateUserSettings(settings: PlatformSettingUpsert[], userId: string, organizationId: string): Promise<PlatformSetting[]>updateUserSettings(settings: PlatformSettingUpsert[], userId: string): Promise<PlatformSetting[]>Warning
Breaking. Both methods shipped in 1.6.1.
updateUserSettings()loses a required positional argument andgetUserSettings()loses its options argument; thePlatformSettingGetOptionstype is removed from the public API.Endpoint Called
Unchanged — no new endpoints, so no Cloudflare whitelist update was needed.
getUserSettings()../identity_/api/SettingPM.Setting/PM.Setting.ReadupdateUserSettings()../identity_/api/SettingPM.Setting/PM.Setting.WriteBaseService, unchanged. The organization reaches the API through the request path alone —ApiClientbuilds{baseUrl}/{orgName}/identity_/api/Settingfrom the SDK's own config — so the methods no longer send an organization of their own.userIdis always sent, so a call never reads or writes across an organization.UIPATH_ORGANIZATION_IDtest secret, so its config field and README row are removed with it.Note
One follow-up left:
.github/workflows/coverage.ymlstill plumbs the now-unusedUIPATH_ORGANIZATION_IDsecret through (lines 87 and 124). Nothing reads it any more, so it is inert, but it should be dropped — it could not be pushed from here without theworkflowOAuth scope.Example Usage
API Response vs SDK Response
No response change. The transform pipeline and every field mapping are untouched:
Transform pipeline
transformData(data, PlatformSettingMap)Field mapping
partitionGlobalIdorganizationIdid,key,value,userIdWhat changed is the request, not the response:
partitionGlobalId(GET query)options.organizationId{orgName}segment of the request pathpartitionGlobalId(PUT body)organizationIdargumentEvery
PlatformSettingstill carriesorganizationId, so callers who need the organization back can still read it off any row.Important
Needs a live check before merge. When this endpoint was onboarded (#629), omitting the scope was observed to fall back to the host partition and return
403— and{orgName}was already on the path then, sinceApiClientalways inserts it. If that still holds, droppingpartitionGlobalIdwill 403 and the last commit should be reverted. The integration suite intests/integration/shared/platform/covers exactly this; it has not been run here (no live credentials in the authoring environment).Sample SDK Response
No sample captured for this PR — the response shape is unchanged from #629, and there are no live credentials in this environment to re-run the E2E app against. Unit tests assert the wire params and the response transform on both methods; the integration suite exercises the live round-trip.
Files
src/services/platform/platform.tssrc/models/platform/platform.types.ts(removedPlatformSettingGetOptions)src/models/platform/platform.models.ts(JSDoc + signatures)src/services/platform/index.ts(module@example)tests/unit/services/platform/platform.test.ts(30 tests)tests/integration/shared/platform/platform.integration.test.ts(8 tests)tests/integration/config/test-config.ts,tests/integration/README.md(removed the now-unusedUIPATH_ORGANIZATION_ID)Verification
npm run typecheckclean ·npm run lint0 errors ·npm run test:unit2467 passed ·npm run buildOK ·npm run docs:validate0 errors (2 pre-existing warnings inconversational-agent/protocol.types.ts)Integration tests were not run — no live credentials in this environment.
🤖 Auto-generated using onboarding skills