feat(ontologies): add Ontologies service for UiPath knowledge graph - #622
feat(ontologies): add Ontologies service for UiPath knowledge graph #622diwakar-jha3110 wants to merge 2 commits into
Conversation
|
…anagement Implements the full OntologyService covering CRUD on ontologies and component file operations (upsert, bulk-upsert, get, list, delete, validate). Wires up the `@uipath/uipath-typescript/ontologies` subpath export with ESM/CJS/UMD dist output. Integration tests are skipped pending API deployment (target mid-July 2026). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| export const ONTOLOGY_ENDPOINTS = { | ||
| GET_ALL: `${ONTOLOGY_BASE}/api/ontology`, | ||
| CREATE: `${ONTOLOGY_BASE}/api/ontology`, |
There was a problem hiding this comment.
Convention violation: duplicate endpoint constants without justifying comments.
CREATE (line 9), UPDATE (line 11), and DELETE (line 12) share identical URL patterns with GET_ALL and GET_BY_ID respectively. Likewise inside ARTIFACT, UPLOAD_BULK duplicates GET_ALL and UPSERT/DELETE duplicate GET.
Per conventions: "Avoid creating a duplicate endpoint constant whose URL pattern is identical to an existing one. HTTP method differences (GET vs PUT) are resolved at the call site (this.get() vs this.put()), not in the constant. Either reuse the existing constant directly, or — if a distinct name genuinely aids readability — add an explicit comment explaining the intentional duplication."
Options:
- Collapse the duplicates and reuse the constants at the call site (
this.post(ONTOLOGY_ENDPOINTS.GET_ALL, ...),this.patch(ONTOLOGY_ENDPOINTS.GET_BY_ID(id), ...)), or - Keep the named aliases but add a comment on each duplicate explaining the intentional duplication (e.g.,
// Same URL as GET_ALL; distinct name aids readability for write operations).
All four sets of duplicates need to be addressed:
GET_ALL/CREATEGET_BY_ID/UPDATE/DELETEARTIFACT.GET_ALL/ARTIFACT.UPLOAD_BULKARTIFACT.GET/ARTIFACT.UPSERT/ARTIFACT.DELETE
be399d3 to
73ca849
Compare
| createTime?: string; | ||
| updatedBy?: string; | ||
| updateTime?: string; | ||
| } |
There was a problem hiding this comment.
Timestamp field naming doesn't match SDK convention.
createTime and updateTime (line 90) will surface as-is in the public OntologySummary type because there is no transform pipeline applied in this service. The SDK convention requires past-tense *Time names throughout: createdTime, updatedTime (or lastModifiedTime).
Fix: add an OntologyMap field-rename table and apply transformData(data, OntologyMap) in the service's response pipeline:
// src/models/ontology/ontology.constants.ts
export const OntologyMap: Record<string, string> = {
createTime: 'createdTime',
updateTime: 'updatedTime',
};Then in the service:
const transformed = transformData(response.data, OntologyMap);
return createOntologyWithMethods(transformed as RawOntologySummary, this);And update RawOntologySummary accordingly:
createdTime?: string;
updatedTime?: string;Same fix needed for ArtifactMetadata below.
| createTime?: string; | ||
| updatedBy?: string; | ||
| updateTime?: string; | ||
| } |
There was a problem hiding this comment.
Same timestamp naming issue as in RawOntologySummary above. ArtifactMetadata.createTime should be createdTime and updateTime (line 105) should be updatedTime. Apply the same OntologyMap transformation (or a separate ArtifactMap) to the listArtifacts, upsertArtifact, and uploadArtifacts response paths.
| | `getGovernanceDecisions()` | `Traces.Api Insights.RealTimeData Insights OR.Folders.Read` | | ||
| | `getGovernanceSummary()` | `Traces.Api Insights.RealTimeData Insights OR.Folders.Read` | | ||
|
|
||
| ## Ontologies |
There was a problem hiding this comment.
OntologyServiceModel is tagged @internal — this docs section should be omitted.
Per conventions: "methods tagged @internal in their JSDoc do not get an OAuth scope entry — they are not part of the public API surface and do not appear in the OAuth integration guide. Similarly, mkdocs.yml nav entries and docs site pages are not needed for services where every public-facing method is tagged @internal."
Since OntologyServiceModel carries @internal, this entire section should be removed. The same applies to the mkdocs.yml nav entry (- Ontologies: api/interfaces/OntologyServiceModel.md) and the docs/pagination.md row — both should also be reverted. Re-add them all once the service graduates to public.
| async upsertArtifact(idOrName: string, fileName: string, request: ArtifactUpsertRequest): Promise<ArtifactMetadata> { | ||
| if (!request.mediaType) { | ||
| throw new Error(`upsertArtifact: --media-type is required (e.g. 'text/owl-functional', 'text/turtle')`); | ||
| } |
There was a problem hiding this comment.
Two issues here (and at the parallel check in validateArtifact line ~152):
-
Wrong error type. Convention says "
ValidationError— for user input validation only: missing required params, invalid option values, malformed user-provided data." UseValidationError, not rawError:throw new ValidationError({ message: 'upsertArtifact: mediaType is required' });
-
Avoid SDK-side duplication of backend validation. Convention: "Do not duplicate backend validation SDK-side — when an API parameter has backend-enforced constraints, document the constraint in JSDoc rather than adding matching SDK-side validation." The backend will already return a 400 for a missing or empty
Content-Type. This guard is redundant. Remove bothif (!request.mediaType)blocks and instead document the constraint in the JSDoc@paramforrequest. -
CLI-style flag syntax in an SDK error message.
--media-typeis a CLI flag; in a TypeScript SDK the error should reference the parameter name as it appears in code:mediaType, not--media-type.
|
|
||
| // skip: Ontology service is not yet provisioned on the alpha tenant. | ||
| // Tests are wired and ready — remove describe.skip once the service is enabled on appsdev/appsdevDefault. | ||
| describe.skip('Ontology Service — Integration Tests', () => { |
There was a problem hiding this comment.
describe.skip is not permitted here per conventions.
Convention: "describe.skip is permitted only when the service does not support PAT auth. In this case, write the full test body as if it will eventually run, add a comment explaining the limitation, and use describe.skip rather than omitting the test entirely."
"Not yet provisioned on the alpha tenant" is an infrastructure gap, not a PAT auth restriction. For infrastructure gaps, the convention says to use beforeAll + throw so the test suite fails visibly in CI (rather than silently being skipped):
describe('Ontology Service — Integration Tests', () => {
let service: OntologyService;
beforeAll(() => {
const svc = getServices().ontologies;
if (!svc) throw new Error('Ontology service not provisioned on this tenant — enable it before running these tests');
service = svc;
});
// ...This way the test is tracked as a real failure when the tenant lacks the service, rather than silently passing.
| export * from './orchestrator'; | ||
| export * from './action-center'; | ||
| export * from './ontology'; | ||
|
|
There was a problem hiding this comment.
New services must not be added to the top-level services barrel.
Convention: "New services must be modularized only — export only via subpath (@uipath/uipath-typescript/ontologies), no top-level barrel export needed."
If src/index.ts re-exports src/services/index.ts (which is the typical pattern for the existing services listed here), adding export * from './ontology' here would expose OntologyService and all its types in the top-level @uipath/uipath-typescript bundle — contrary to the modular-only requirement for new services. Remove this line; the subpath export in package.json and rollup.config.js is sufficient.
Review findings — 7 issuesBlocking
Should fix
|
| ARTIFACT_TYPE_SCHEMA: 'schema', | ||
| ARTIFACT_TYPE_CONSTRAINTS: 'constraints', |
There was a problem hiding this comment.
Unused dead constants — remove per code hygiene convention.
ARTIFACT_TYPE_SCHEMA: 'schema' and ARTIFACT_TYPE_CONSTRAINTS: 'constraints' are never referenced in any test file. Every test that checks artifact types uses the enum directly (ArtifactType.Schema, ArtifactType.Constraints). These raw-string constants also violate the convention "Use enums for fixed value sets — NEVER leave raw strings/numbers".
Per conventions: "NEVER leave unused code — unused imports, variables, redundant constructors."
| ARTIFACT_TYPE_SCHEMA: 'schema', | |
| ARTIFACT_TYPE_CONSTRAINTS: 'constraints', |
Simply delete both lines. Tests already use the ArtifactType enum values directly.
| import { PaginationType } from '../../utils/pagination/internal-types'; | ||
| import { BaseService } from '../base'; | ||
|
|
||
| export class OntologyService extends BaseService implements OntologyServiceModel { |
There was a problem hiding this comment.
Missing @internal propagation on the service class.
OntologyServiceModel carries /** @internal */, but the OntologyService class itself has no JSDoc at all. Per conventions:
"Propagate
@internaland@experimentalto every public layer that exposes the same API — if an underlying service method is tagged@internal, the corresponding wrapper on theUiPathclass (or any other public-facing class) must also carry the same tag. TypeDoc runs directly on public class methods; a missing tag on one layer surfaces the method in generated docs even if the service layer correctly marks it."
Add a JSDoc block to the class:
| export class OntologyService extends BaseService implements OntologyServiceModel { | |
| /** @internal */ | |
| export class OntologyService extends BaseService implements OntologyServiceModel { |
The same tag should be propagated to the Ontologies export alias in src/services/ontology/index.ts once all three fixes (this, the docs cleanup in thread on docs/oauth-scopes.md, and the barrel removal in the thread on src/services/index.ts) are applied together.
|
Two new findings this run (in addition to the 7 open threads already on this PR):
|
|
@diwakar-jha3110 My understanding is Ontology is far from preview, why do we want to add it to SDK now itself? If this is meant to just be for testing, lets just for the have a branch and push a dev package into GitHub feed. |
| await expect(svc.getById(ontology.id)).rejects.toThrow(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
exportOntology has no integration test.
Convention: "Every new method must also have an integration test in tests/integration/shared/{domain}/."
All 11 other service methods have an integration test block, but exportOntology is missing entirely. Add a describe('exportOntology', ...) block alongside the others — even while the suite is skipped, it should be written as if it will run:
describe('exportOntology', () => {
it('should export the ontology as a zip archive', async () => {
const svc = getService();
const ontology = await svc.create('export-test', { displayName: 'Export Test' });
createdIds.push(ontology.id);
const zip = await svc.exportOntology(ontology.id);
expect(zip).toBeInstanceOf(Uint8Array);
expect(zip.length).toBeGreaterThan(0);
// ZIP local file header magic: PK\x03\x04
expect(zip[0]).toBe(0x50);
expect(zip[1]).toBe(0x4b);
});
});
|
One new finding this run (in addition to the 9 open threads already on this PR): Missing |
|
2 participants
Adds OntologyService to the SDK — CRUD for ontologies and full artifact management.
What changed
OntologyServicewith 11 methods: create, getAll, getById, update, deleteById, exportOntology, upsertArtifact, uploadArtifacts, getArtifact, listArtifacts, deleteArtifact, validateArtifactArtifactTypeenum: schema, constraints, mapping, business-rules, summary, context, functions, actionsgetAllvia existingPaginationHelpers@uipath/uipath-typescript/ontologiesapi-client.ts: fixed empty-body guard ordering — TEXT responseType was returning""instead ofundefinedfor zero-byte responses