Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RDP's in TS OSDK #1035

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

RDP's in TS OSDK #1035

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
246f893
Runtime Derived Property Syntax
nihalbhatnagar Dec 4, 2024
50ca838
Working derive clause
nihalbhatnagar Dec 4, 2024
4e17a4e
Update to use map
nihalbhatnagar Dec 4, 2024
01f5935
Update aggregation keys
nihalbhatnagar Dec 5, 2024
ebe19e4
Update
nihalbhatnagar Dec 5, 2024
4ea27dc
Update PR
nihalbhatnagar Dec 5, 2024
d2d0a0d
Initial test suite
nihalbhatnagar Dec 6, 2024
efbed3d
Working version
nihalbhatnagar Dec 9, 2024
40ee625
Cleanup
nihalbhatnagar Dec 9, 2024
3e535da
Add test case
nihalbhatnagar Dec 9, 2024
20835ea
remove use of crypto
nihalbhatnagar Dec 9, 2024
76a3ee1
Changeset
nihalbhatnagar Dec 9, 2024
73c23fe
E2E testing
nihalbhatnagar Dec 10, 2024
d605787
Osdk.Instance typing update
nihalbhatnagar Dec 10, 2024
214dad7
Rename and cleanup
nihalbhatnagar Dec 10, 2024
73fbffa
Fix UUID
nihalbhatnagar Dec 10, 2024
9070c29
Quick fixes
nihalbhatnagar Dec 13, 2024
ac7f03f
Update aggregations syntax
nihalbhatnagar Dec 16, 2024
2777334
Merge branch 'main' into nihalb/rdp-v1
nihalbhatnagar Dec 16, 2024
a8546ca
Update
nihalbhatnagar Dec 18, 2024
0cd49f5
Merge branch 'main' into nihalb/rdp-v1
nihalbhatnagar Jan 8, 2025
0a89f9c
Update names
nihalbhatnagar Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions etc/api.report.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,13 +948,12 @@ export type TimeSeriesQuery = {
// @public (undocumented)
export type TwoDimensionalQueryAggregationDefinition = AggregationKeyDataType<"date" | "double" | "timestamp">;

// Warning: (ae-forgotten-export) The symbol "StringAggregateOption" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "NumericAggregateOption" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "AGG_FOR_TYPE" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "PropertyValueClientToWire" needs to be exported by the entry point index.d.ts
//
// @public (undocumented)
export type ValidAggregationKeys<Q extends ObjectOrInterfaceDefinition, StringOptions extends string = StringAggregateOption, NumericOptions extends string = NumericAggregateOption> = keyof ({
[KK in AggregatableKeys<Q> as `${KK & string}:${number extends PropertyValueClientToWire[CompileTimeMetadata<Q>["properties"][KK]["type"]] ? NumericOptions : string extends PropertyValueClientToWire[CompileTimeMetadata<Q>["properties"][KK]["type"]] ? StringOptions : never}`]?: any;
export type ValidAggregationKeys<Q extends ObjectOrInterfaceDefinition, R extends "aggregate" | "withPropertiesAggregate" = "aggregate"> = keyof ({
[KK in AggregatableKeys<Q> as `${KK & string}:${AGG_FOR_TYPE<PropertyValueClientToWire[CompileTimeMetadata<Q>["properties"][KK]["type"]], R extends "aggregate" ? true : false>}`]?: any;
} & {
$count?: any;
});
Expand Down Expand Up @@ -987,7 +986,7 @@ export type WithPropertiesClause<Q extends ObjectOrInterfaceDefinition> = {

// @public (undocumented)
export type WithPropertyDefinition<T extends ObjectMetadata.Property> = {
definitionId: unknown;
definitionId: string;
type: T;
};

Expand Down
23 changes: 13 additions & 10 deletions packages/api/src/aggregate/AggregatableKeys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
* limitations under the License.
*/

import type {
NumericDeriveAggregateOption,
StringDeriveAggregateOption,
} from "../derivedProperties/WithPropertyObjectSet.js";
import type { PropertyValueClientToWire } from "../mapping/PropertyValueMapping.js";
import type {
ObjectOrInterfaceDefinition,
Expand All @@ -30,25 +34,24 @@ export type NumericAggregateOption =
| "approximateDistinct"
| "exactDistinct";

type AGG_FOR_TYPE<T> = number extends T ? NumericAggregateOption
: string extends T ? StringAggregateOption
type AGG_FOR_TYPE<T, U extends boolean> = number extends T
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename U so its clear what that boolean generic is for

? U extends true ? NumericAggregateOption : NumericDeriveAggregateOption
: string extends T
? U extends true ? StringAggregateOption : StringDeriveAggregateOption
: never;

export type ValidAggregationKeys<
Q extends ObjectOrInterfaceDefinition,
StringOptions extends string = StringAggregateOption,
NumericOptions extends string = NumericAggregateOption,
R extends "aggregate" | "withPropertiesAggregate" = "aggregate",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ssanjay1 Is this what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that works

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though maybe keeping parity with the other names it should be aggregate | derivedPropertiesAggregate or something along those lines, basically including the word derive

> = keyof (
& {
[
KK in AggregatableKeys<Q> as `${KK & string}:${number extends
KK in AggregatableKeys<Q> as `${KK & string}:${AGG_FOR_TYPE<
PropertyValueClientToWire[
CompileTimeMetadata<Q>["properties"][KK]["type"]
] ? NumericOptions
: string extends PropertyValueClientToWire[
CompileTimeMetadata<Q>["properties"][KK]["type"]
] ? StringOptions
: never}`
],
R extends "aggregate" ? true : false
>}`
]?: any;
}
& { $count?: any }
Expand Down
2 changes: 1 addition & 1 deletion packages/api/src/derivedProperties/WithPropertiesClause.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
} from "../ontology/ObjectOrInterface.js";
import type {
ObjectMetadata,
PropertyDef,

Check failure on line 22 in packages/api/src/derivedProperties/WithPropertiesClause.ts

View workflow job for this annotation

GitHub Actions / Build and Test (20) 

'PropertyDef' is defined but never used

Check failure on line 22 in packages/api/src/derivedProperties/WithPropertiesClause.ts

View workflow job for this annotation

GitHub Actions / Build and Test (22) 

'PropertyDef' is defined but never used
} from "../ontology/ObjectTypeDefinition.js";
import type { BaseWithPropertyObjectSet } from "./WithPropertyObjectSet.js";

export type WithPropertyDefinition<
T extends ObjectMetadata.Property,
> = {
definitionId: unknown;
definitionId: string;
type: T;
};

Expand Down
34 changes: 18 additions & 16 deletions packages/api/src/derivedProperties/WithPropertyObjectSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,45 +51,43 @@ interface FilterableDeriveObjectSet<
clause: WhereClause<Q>,
) => this;
}
export type CollectDeriveAggregations = "collectSet" | "collectList";

type CollectAggregations = "collectSet" | "collectList";

type BaseAggregations =
export type BaseDeriveAggregations =
| "approximateDistinct"
| "exactDistinct"
| "approximatePercentile";

type StringDeriveAggregateOption =
| BaseAggregations
| CollectAggregations;
export type StringDeriveAggregateOption =
| BaseDeriveAggregations
| CollectDeriveAggregations;

type NumericDeriveAggregateOption =
export type NumericDeriveAggregateOption =
| "min"
| "max"
| "sum"
| "avg"
| BaseAggregations
| CollectAggregations;
| BaseDeriveAggregations
| CollectDeriveAggregations;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am thinking about whats ideal we need to add some type tests. I'm okay if its in a follow up

interface AggregatableWithPropertyObjectSet<
Q extends ObjectOrInterfaceDefinition,
> extends FilterableDeriveObjectSet<Q> {
> {
readonly aggregate: <
V extends ValidAggregationKeys<
Q,
StringDeriveAggregateOption,
NumericDeriveAggregateOption
"withPropertiesAggregate"
>,
>(
aggregationSpecifier: V,
opts?: V extends `${any}:${infer P}`
? P extends CollectAggregations ? { limit: number }
? P extends CollectDeriveAggregations ? { limit: number }
: P extends "approximatePercentile" ? { percentile: number }
: never
: never,
) => WithPropertyDefinition<
V extends `${infer N}:${infer P}`
? P extends CollectAggregations ? PropertyDef<
? P extends CollectDeriveAggregations ? PropertyDef<
CompileTimeMetadata<Q>["properties"][N]["type"],
"nullable",
"array"
Expand All @@ -104,15 +102,19 @@ interface AggregatableWithPropertyObjectSet<

interface SingleLinkWithPropertyObjectSet<
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseObjectSet extends SingleLinkWithPropertyObjectSet. This makes the selectProperty method available only for chains of single links, which correlates with the backend. The pivotTo operation in BaseObjectSet enforces that if we pivot to a many link, we from then on use ManyLinkWithPropertyObjectSet which does not have selectProperty available and only returns further ManyLink object sets.

Q extends ObjectOrInterfaceDefinition,
> extends AggregatableWithPropertyObjectSet<Q>, BaseWithPropertyObjectSet<Q> {
> extends
AggregatableWithPropertyObjectSet<Q>,
FilterableDeriveObjectSet<Q>,
BaseWithPropertyObjectSet<Q>
{
readonly selectProperty: <R extends PropertyKeys<Q>>(
propertyName: R,
) => WithPropertyDefinition<CompileTimeMetadata<Q>["properties"][R]>;
}

interface ManyLinkWithPropertyObjectSet<
Q extends ObjectOrInterfaceDefinition,
> extends AggregatableWithPropertyObjectSet<Q> {
> extends AggregatableWithPropertyObjectSet<Q>, FilterableDeriveObjectSet<Q> {
readonly pivotTo: <L extends LinkNames<Q>>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just clarifying, the reason we need to duplicate pivotTo here is because if there's a many link anywhere in the clause, you can't select anymore? Even if you go from a many to a single link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's correct, which makes sense because for this to work you have to basically be navigating to exactly one object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably document that here too.

type: L,
) => ManyLinkWithPropertyObjectSet<LinkedType<Q, L>>;
Expand Down
2 changes: 1 addition & 1 deletion packages/api/src/objectSet/ObjectSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
} from "../ontology/ObjectOrInterface.js";
import type {
CompileTimeMetadata,
ObjectMetadata,

Check failure on line 42 in packages/api/src/objectSet/ObjectSet.ts

View workflow job for this annotation

GitHub Actions / Build and Test (20) 

'ObjectMetadata' is defined but never used

Check failure on line 42 in packages/api/src/objectSet/ObjectSet.ts

View workflow job for this annotation

GitHub Actions / Build and Test (22) 

'ObjectMetadata' is defined but never used
ObjectTypeDefinition,
PropertyDef,
} from "../ontology/ObjectTypeDefinition.js";
Expand Down Expand Up @@ -288,7 +288,7 @@
[T in Extract<keyof D, string>]: D[T] extends
(baseObjectSet: any) => WithPropertyDefinition<infer P>
? P extends PropertyDef<infer A, any, any>
? PropertyValueWireToClient[A]
? PropertyValueWireToClient[A] | undefined
: never
: never;
};
Expand Down
16 changes: 11 additions & 5 deletions packages/client/src/derivedProperties/createWithPropertiesObjectSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export function createWithPropertiesObjectSet<
type: "searchAround",
objectSet,
link,
}, definitionMap) as any;
}, definitionMap);
},
where: (clause) => {
return createWithPropertiesObjectSet(objectType, {
Expand Down Expand Up @@ -100,24 +100,30 @@ export function createWithPropertiesObjectSet<
"Invalid aggregation operation " + aggregationOperation,
);
}
definitionMap.set(definitionId, {
definitionMap.set(definitionId.toString(), {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose for a moment definitionMap is actually Map<object, DerivedPropertyDefinition>.

instead of definitionId we could do this:

const ret = {} as WithPropertyDefinition<any>;
definitionMap.set(ret, {... });
return ret;

No need to do accounting of ids and we don't have to leak them/make them part of our public API.

Its still not perfect because people might return their own objects thinking thats okay when its not and they still might do something weird like reuse an object from before (which won't work because its a different map) but its closer.

Ultimately I think we are probably better just designing an IR that is based on raw objects.

type: "selection",
objectSet: objectSet,
operation: aggregationOperationDefinition,
});
return { definitionId: definitionId, type: { type: "integer" } as any };
return {
definitionId: definitionId.toString(),
type: {} as any,
};
},
selectProperty: (name) => {
const definitionId = idCounter++;
definitionMap.set(definitionId, {
definitionMap.set(definitionId.toString(), {
type: "selection",
objectSet: objectSet,
operation: {
type: "get",
selectedPropertyApiName: name,
},
});
return { definitionId: definitionId, type: { type: "integer" } as any };
return {
definitionId: definitionId.toString(),
type: {} as any,
};
},
};

Expand Down
33 changes: 29 additions & 4 deletions packages/client/src/objectSet/ObjectSet.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
CompileTimeMetadata,
ConvertProps,
InterfaceDefinition,
ObjectMetadata,

Check failure on line 21 in packages/client/src/objectSet/ObjectSet.test.ts

View workflow job for this annotation

GitHub Actions / Build and Test (18) 

'ObjectMetadata' is defined but never used
ObjectSet,
Osdk,
PropertyKeys,
Expand Down Expand Up @@ -466,6 +466,19 @@
});
});

it("enforces a return only of correct type", () => {
client(Employee).withProperties({
// @ts-expect-error
"derivedPropertyName": (base) => {
return base.pivotTo("peeps");
},
// @ts-expect-error
"derivedPropertyName2": (base) => {
return { incorrect: "type" };
},
});
});

// Executed code fails since we're providing bad strings to the function
it.fails("correctly narrows types of aggregate function", () => {
client(Employee).withProperties({
Expand All @@ -479,6 +492,9 @@
// @ts-expect-error
base.pivotTo("lead").aggregate("employeeId:notAnOp");

// @ts-expect-error
base.pivotTo("lead").aggregate("");

base.pivotTo("lead").aggregate("employeeId:collectList");

return base.pivotTo("lead").aggregate("employeeId:sum");
Expand Down Expand Up @@ -590,8 +606,17 @@
base.pivotTo("lead").selectProperty("employeeId"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test to verify things don't break if we select a property that has all undefined values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically if a prop returns an undefined value

}).fetchOne(50035);

expectTypeOf(objectWithRdp.derivedPropertyName).toEqualTypeOf<number>();
expectTypeOf(objectWithRdp.derivedPropertyName).toEqualTypeOf<
number | undefined
>();
expect(objectWithRdp.derivedPropertyName).toBe(1);

const objectWithUndefinedRdp = await client(Employee).withProperties({
"derivedPropertyName": (base) =>
base.pivotTo("lead").selectProperty("employeeId"),
}).fetchOne(50036);

expect(objectWithUndefinedRdp.derivedPropertyName).toBeUndefined();
});

describe("withPropertiesObjectSet", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: don't pass this string. This test seems to be directly testing createWithPropertiesObjectSet and so you can literally pass a reference to said function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also means that this entire describe should really be in createWithPropertiesObjectSet.test.ts so its clearer whats under test.

Expand All @@ -607,7 +632,7 @@
};

const result = clause["derivedPropertyName"](deriveObjectSet);
const definition = map.get(result.definitionId as string);
const definition = map.get(result.definitionId);
expect(definition).toMatchInlineSnapshot(`
{
"objectSet": {
Expand Down Expand Up @@ -645,12 +670,12 @@
};

const result = clause["derivedPropertyName"](deriveObjectSet);
const definition = map.get(result.definitionId as string);
const definition = map.get(result.definitionId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some mixed feelings about the definitionId still, especially considering it ends up on the public api.


const secondResult = clause["secondaryDerivedPropertyName"](
deriveObjectSet,
);
const secondDefinition = map.get(secondResult.definitionId as string);
const secondDefinition = map.get(secondResult.definitionId);

expect(definition).toMatchInlineSnapshot(`
{
Expand Down
30 changes: 30 additions & 0 deletions packages/shared.test/src/stubs/objectSetRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
employee2,
employee3,
employee4withDerived,
employee5withUndefinedDerived,
employeeFailsStrict,
nycOffice,
objectWithAllPropertyTypes1,
Expand Down Expand Up @@ -108,6 +109,32 @@ const derivedPropertyBody: LoadObjectSetRequestV2 = {
select: [],
};

const derivedPropertyBodyUndefinedValue: LoadObjectSetRequestV2 = {
objectSet: {
objectSet: {
derivedProperties: {
derivedPropertyName: {
objectSet: {
link: "lead",
objectSet: { type: "methodInput" },
type: "searchAround",
},
operation: {
selectedPropertyApiName: "employeeId",
type: "get",
},
type: "selection",
},
},
objectSet: { "objectType": "Employee", "type": "base" },
type: "withProperties",
},
type: "filter",
where: { "field": "employeeId", "type": "eq", "value": 50036 },
},
select: [],
};

const eqSearchBody: LoadObjectSetRequestV2 = {
objectSet: {
type: "filter",
Expand Down Expand Up @@ -473,6 +500,9 @@ export const loadObjectSetRequestHandlers: {
[stableStringify(intersectedObjectSet)]: [employee3],
[stableStringify(subtractedObjectSet)]: [employee2, employee3],
[stableStringify(derivedPropertyBody)]: [employee4withDerived],
[stableStringify(derivedPropertyBodyUndefinedValue)]: [
employee5withUndefinedDerived,
],
[stableStringify(eqSearchBody)]: [employee1],
[stableStringify(eqSearchBody2)]: [employee2],
[stableStringify(eqSearchBodyBadObject)]: [employeeFailsStrict],
Expand Down
17 changes: 17 additions & 0 deletions packages/shared.test/src/stubs/objects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,23 @@ export const employee4withDerived = {
employeeLocation: "GeotimeSeriesReferencePlaceholder",
};

export const employee5withUndefinedDerived = {
__rid:
"ri.phonograph2-objects.main.object.b9a0b2b0-0a2b-0b8b-9e4b-a9a9b9a0b9a0",
__primaryKey: 50036,
__apiName: "Employee",
__title: "Jack Smith",
employeeId: 50036,
fullName: "Jack Smith",
office: "LON",
class: "Red",
startDate: "2015-05-15",
derivedPropertyName: undefined,
employeeStatus: "TimeSeries<String>",
employeeSensor: "TimeSeries<>",
employeeLocation: "GeotimeSeriesReferencePlaceholder",
};

export const employeeFailsStrict = {
__rid:
"ri.phonograph2-objects.main.object.b9a0b2b0-0a2b-0b8b-9e4b-a9a9b9a0b9a0",
Expand Down
Loading