Skip to content

feat: add Data Fabric roles and directory services#473

Open
dsuresh-ap wants to merge 16 commits into
mainfrom
feature/data-fabric-roles-sdk
Open

feat: add Data Fabric roles and directory services#473
dsuresh-ap wants to merge 16 commits into
mainfrom
feature/data-fabric-roles-sdk

Conversation

@dsuresh-ap

@dsuresh-ap dsuresh-ap commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

  • Add DataFabricRoles and DataFabricDirectory services to the SDK surface.
  • Introduce typed models for role listing, directory listing, and role assignment payloads/results.
  • Wire the new services into UiPath legacy accessors and export them from the Data Fabric module.
  • Add unit coverage for role listing, directory paging, and role assignment behavior.

Testing

  • Unit tests added for Data Fabric roles and directory services.
  • Legacy UiPath service wiring test updated to verify the new accessors are exposed.
  • Tested against alp

@dsuresh-ap dsuresh-ap requested a review from a team May 28, 2026 16:33
Comment thread src/models/data-fabric/roles.types.ts Outdated
Comment thread src/models/data-fabric/directory.types.ts Outdated
Comment thread src/services/data-fabric/directory.ts
Comment thread src/services/data-fabric/directory.ts Outdated
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review summary

Four inline comments posted. Two additional checklist items from the post-implementation verification checklist in agent_docs/rules.md:

Missing: integration tests
Every new method requires an integration test in tests/integration/shared/data-fabric/. The PR description notes these were not run/written. Per conventions:

Every new method must also have an integration test in tests/integration/shared/{domain}/. These run against a live API and catch issues unit tests miss — wrong endpoints, broken transforms, auth/header problems.

Both DataFabricRoleService.getAll and all three DataFabricDirectoryService methods need integration test coverage.

Missing: docs/oauth-scopes.md update
Per conventions:

When adding methods, update docs/oauth-scopes.md with required OAuth scopes. NEVER skip this — missing scopes break the OAuth integration guide.

The new /api/v2/Role and /api/Directory endpoints will require specific OAuth scopes — these need to be documented.

@dsuresh-ap dsuresh-ap force-pushed the feature/data-fabric-roles-sdk branch 2 times, most recently from 256693b to 49ed7eb Compare May 28, 2026 19:33
Comment thread src/services/data-fabric/directory.ts Outdated
Comment thread src/services/data-fabric/directory.ts
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review summary

Two new findings this run:

1. DirectoryPaginationOptions belongs in directory.internal-types.ts (line 31)
Service-internal operation type defined at module scope in the service file - conventions require it to be in *.internal-types.ts.

2. assignRoles calling this.getAll() fires spurious telemetry (line 255)
Every assignRoles call with preserveExisting: true fires both DataFabricDirectory.AssignRoles and DataFabricDirectory.GetAll, inflating GetAll metrics. Fix: extract a private un-tracked fetchAllEntries() helper and have both getAll() and assignRoles delegate to it.

Comment thread src/utils/pagination/helpers.ts Outdated
Comment thread src/services/data-fabric/directory.ts Outdated
Comment thread tests/integration/shared/data-fabric/access.integration.test.ts Outdated
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review summary

Three new findings this pass (prior four threads are open and unchanged):

New inline comments posted:

  1. src/utils/pagination/helpers.ts line 368getAllPages options parameter typed as Record<string, any>. Violates the no-any rule; should use a constrained generic like PaginationHelpers.getAll does.

  2. src/services/data-fabric/directory.ts line 240assignRoles calls this.getAll(), which is decorated with @track('DataFabricDirectory.GetAll'). Every assignRoles(preserveExisting: true) call fires a spurious DataFabricDirectory.GetAll telemetry event. Fix: extract the internal fetch into a private untracked fetchAllEntries() helper.

  3. tests/integration/shared/data-fabric/access.integration.test.ts line 91describe.skip used to skip a write test for mutation-safety reasons. Convention only permits describe.skip for PAT-auth failures; mutation risk should be handled with a beforeAll guard that throws unless a dedicated env var is set.

@dsuresh-ap dsuresh-ap force-pushed the feature/data-fabric-roles-sdk branch 2 times, most recently from dab1d8b to d0e4c3e Compare May 28, 2026 19:47
Comment thread tests/unit/utils/pagination/helpers.test.ts Outdated
Comment thread tests/unit/services/data-fabric/directory.test.ts
Comment thread tests/unit/services/data-fabric/roles.test.ts
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review summary

Three new findings this pass:

1. tests/unit/utils/pagination/helpers.test.ts line 862let mockConfig: any violates the no-any rule. Should use GetAllConfig<RawType, TransformedType> from the helpers module.

2. tests/unit/services/data-fabric/directory.test.ts line 29list and getAll describe blocks have no error scenario tests. Convention requires testing both success and error paths for every public method.

3. tests/unit/services/data-fabric/roles.test.ts line 27getAll describe block has no error scenario test. Same convention gap.

@dsuresh-ap dsuresh-ap force-pushed the feature/data-fabric-roles-sdk branch 2 times, most recently from f4223ca to c80d2a9 Compare May 28, 2026 19:54
Comment thread src/utils/constants/common.ts Outdated
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding this run:

src/utils/constants/common.ts line 64 — DIRECTORY_PAGINATION.ITEMS_FIELD and TOTAL_COUNT_FIELD only handle lowercase results/totalCount, while extractDirectoryItems() and extractTotalCount() (used by list()) also handle Results/TotalCount. If the live API sends PascalCase fields, getAll() silently returns [] while list() returns the correct data. The presence of both casings in RawDataFabricDirectoryListResponse suggests the actual API response format was not verified before the PR was written.

Comment thread src/models/data-fabric/directory.types.ts Outdated
@claude

claude Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding this pass:

src/models/data-fabric/directory.types.ts line 41type?: string | number uses raw primitives when DataFabricDirectoryEntityType already exists for this fixed value set. Suggested DataFabricDirectoryEntityType | string to preserve enum semantics while staying defensive against string API responses.

@claude

claude Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

Comment thread README.md Outdated
Comment thread src/services/data-fabric/index.ts Outdated
Comment thread src/models/data-fabric/index.ts Outdated
Comment thread src/services/data-fabric/index.ts Outdated
Comment thread src/services/base.ts Outdated
Comment thread src/services/base.ts Outdated
Comment thread src/services/data-fabric/directory.ts Outdated
@dsuresh-ap dsuresh-ap force-pushed the feature/data-fabric-roles-sdk branch from 8fde337 to fed2aa0 Compare June 23, 2026 20:02
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

@dsuresh-ap dsuresh-ap force-pushed the feature/data-fabric-roles-sdk branch from fed2aa0 to 91a3837 Compare June 23, 2026 20:21
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

@sonarqubecloud

Copy link
Copy Markdown

@dsuresh-ap dsuresh-ap requested a review from Sarath1018 June 23, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants