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.

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
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
7 changes: 7 additions & 0 deletions .changeset/forty-eggs-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@osdk/shared.test": minor
"@osdk/client": minor
"@osdk/api": minor
---

Adds support for runtime derived properties
30 changes: 27 additions & 3 deletions etc/api.report.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,10 @@ export interface ObjectSet<Q extends ObjectOrInterfaceDefinition = any, _UNUSED
};
readonly subtract: (...objectSets: ReadonlyArray<CompileTimeMetadata<Q>["objectSet"]>) => this;
readonly union: (...objectSets: ReadonlyArray<CompileTimeMetadata<Q>["objectSet"]>) => this;
// Warning: (ae-forgotten-export) The symbol "WithPropertiesObjectDefinition" needs to be exported by the entry point index.d.ts
//
// (undocumented)
readonly withProperties: <D extends WithPropertiesClause<Q>>(clause: D) => ObjectSet<WithPropertiesObjectDefinition<Q, D>>;
}

// @public (undocumented)
Expand Down Expand Up @@ -941,12 +945,13 @@ export type TimeSeriesQuery = {
// @public (undocumented)
export type TwoDimensionalQueryAggregationDefinition = AggregationKeyDataType<"date" | "double" | "timestamp">;

// 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 "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 "PropertyValueClientToWire" needs to be exported by the entry point index.d.ts
//
// @public (undocumented)
export type ValidAggregationKeys<Q extends ObjectOrInterfaceDefinition> = keyof ({
[KK in AggregatableKeys<Q> as `${KK & string}:${AGG_FOR_TYPE<PropertyValueClientToWire[CompileTimeMetadata<Q>["properties"][KK]["type"]]>}`]?: any;
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;
} & {
$count?: any;
});
Expand All @@ -972,10 +977,29 @@ export type WhereClause<T extends ObjectOrInterfaceDefinition> = OrWhereClause<T
// @public (undocumented)
export type WirePropertyTypes = "string" | "datetime" | "double" | "boolean" | "integer" | "timestamp" | "short" | "long" | "float" | "decimal" | "byte" | "marking" | "numericTimeseries" | "stringTimeseries" | "sensorTimeseries" | "attachment" | "geopoint" | "geoshape" | "geotimeSeriesReference";

// @public (undocumented)
export type WithPropertiesClause<Q extends ObjectOrInterfaceDefinition> = {
[key: string]: (baseObjectSet: BaseWithPropertyObjectSet<Q>) => WithPropertyDefinition<ObjectMetadata.Property>;
};

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

// Warning: (ae-forgotten-export) The symbol "AggregatableWithPropertyObjectSet" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "SingleLinkWithPropertyObjectSet" needs to be exported by the entry point index.d.ts
//
// @public (undocumented)
export interface WithPropertyObjectSet<Q extends ObjectOrInterfaceDefinition> extends BaseWithPropertyObjectSet<Q>, AggregatableWithPropertyObjectSet<Q>, SingleLinkWithPropertyObjectSet<Q> {
}

// Warnings were encountered during analysis:
//
// src/aggregate/AggregateOpts.ts:26:3 - (ae-forgotten-export) The symbol "UnorderedAggregationClause" needs to be exported by the entry point index.d.ts
// src/aggregate/AggregateOpts.ts:26:3 - (ae-forgotten-export) The symbol "OrderedAggregationClause" needs to be exported by the entry point index.d.ts
// src/derivedProperties/WithPropertiesClause.ts:36:3 - (ae-forgotten-export) The symbol "BaseWithPropertyObjectSet" needs to be exported by the entry point index.d.ts

// (No @packageDocumentation comment for this package)

Expand Down
11 changes: 8 additions & 3 deletions packages/api/src/aggregate/AggregatableKeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,19 @@ type AGG_FOR_TYPE<T> = number extends T ? NumericAggregateOption

export type ValidAggregationKeys<
Q extends ObjectOrInterfaceDefinition,
StringOptions extends string = StringAggregateOption,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to keep all the agg options a bit simpler to parse though, another way to format this type could be keeping the Agg_For_Type and modifying it to take an extra boolean (that controls whether this is for derive or not, default to false) and have it branch to the right agg options that way, rather than having the responsibility on the user to pass in custom options if they want. That being said, if you feel like in the future we'll need to customize agg options even more, this does make it more flexible so up to you

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do keep it like this, lets remove AGG_FOR_TYPE if its not being used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah this does make sense to me, will refactor

NumericOptions extends string = NumericAggregateOption,
> = keyof (
& {
[
KK in AggregatableKeys<Q> as `${KK & string}:${AGG_FOR_TYPE<
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;
}
& { $count?: any }
Expand Down
39 changes: 39 additions & 0 deletions packages/api/src/derivedProperties/WithPropertiesClause.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2024 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import type {
ObjectOrInterfaceDefinition,
} from "../ontology/ObjectOrInterface.js";
import type {
ObjectMetadata,
PropertyDef,
} from "../ontology/ObjectTypeDefinition.js";
import type { BaseWithPropertyObjectSet } from "./WithPropertyObjectSet.js";

export type WithPropertyDefinition<
T extends ObjectMetadata.Property,
> = {
definitionId: unknown;
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 not narrow this type down at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can type this down to a string, I was wondering if we should type this as unknown to tell the user -> dont touch this

Copy link
Contributor Author

@nihalbhatnagar nihalbhatnagar Dec 13, 2024

Choose a reason for hiding this comment

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

Typed it as a string even though we could use a number in case we decide we want to use a UUD or something else in the future

type: T;
};

export type WithPropertiesClause<
Q extends ObjectOrInterfaceDefinition,
> = {
[key: string]: (
baseObjectSet: BaseWithPropertyObjectSet<Q>,
) => WithPropertyDefinition<ObjectMetadata.Property>;
};
119 changes: 119 additions & 0 deletions packages/api/src/derivedProperties/WithPropertyObjectSet.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* Copyright 2024 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import type { ValidAggregationKeys } from "../aggregate/AggregatableKeys.js";
import type { WhereClause } from "../aggregate/WhereClause.js";
import type {
ObjectOrInterfaceDefinition,
PropertyKeys,
} from "../ontology/ObjectOrInterface.js";
import type {
CompileTimeMetadata,
PropertyDef,
} from "../ontology/ObjectTypeDefinition.js";
import type { LinkedType, LinkNames } from "../util/LinkUtils.js";
import type { WithPropertyDefinition } from "./WithPropertiesClause.js";

Copy link
Contributor Author

@nihalbhatnagar nihalbhatnagar Dec 10, 2024

Choose a reason for hiding this comment

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

I use inheritance here to make sure we enforce what methods are available and to reduce duplication

export interface WithPropertyObjectSet<Q extends ObjectOrInterfaceDefinition>
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets be consistent with naming, see some withProperty and derive here.

extends
BaseWithPropertyObjectSet<Q>,
AggregatableWithPropertyObjectSet<Q>,
SingleLinkWithPropertyObjectSet<Q>
{}

export interface BaseWithPropertyObjectSet<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This object set only has filter and pivot as aggregations and selectProperty is not available on the base object set.

Q extends ObjectOrInterfaceDefinition,
> extends FilterableDeriveObjectSet<Q> {
readonly pivotTo: <L extends LinkNames<Q>>(
type: L,
) => NonNullable<CompileTimeMetadata<Q>["links"][L]["multiplicity"]> extends
false ? SingleLinkWithPropertyObjectSet<LinkedType<Q, L>>
: ManyLinkWithPropertyObjectSet<LinkedType<Q, L>>;
}

interface FilterableDeriveObjectSet<
Q extends ObjectOrInterfaceDefinition,
> {
readonly where: (
clause: WhereClause<Q>,
) => this;
}

type CollectAggregations = "collectSet" | "collectList";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for this and the type below can we call them something like CollectDeriveAggregations, really want to make sure we don't confuse these with existing normal agg options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debating between the above Derive, ow WithPropertyBaseAggregations, or BaseWithPropertyAggregations. We aren't using "derive" as a term, but it is much cleaner than the other 2. Number 3 is kind of confusing, but number 2 would mean they al start the same which I don't want. Stuck with number 1 for conciseness


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

type StringDeriveAggregateOption =
| BaseAggregations
| CollectAggregations;

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

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> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to extend FilterableDeriveObjectSet<Q> purely to make ManyLinkWithPropertyObjectSet work? If so, could we just have that extend FilterableDeriveObjectSet<Q> as well? Feels a bit confusing the way its setup now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Agreed this makes it more readable

readonly aggregate: <
V extends ValidAggregationKeys<
Q,
StringDeriveAggregateOption,
NumericDeriveAggregateOption
>,
>(
aggregationSpecifier: V,
opts?: V extends `${any}:${infer P}`
? P extends CollectAggregations ? { limit: number }
: P extends "approximatePercentile" ? { percentile: number }
: never
: never,
) => WithPropertyDefinition<
V extends `${infer N}:${infer P}`
? P extends CollectAggregations ? PropertyDef<
CompileTimeMetadata<Q>["properties"][N]["type"],
"nullable",
"array"
>
: P extends "approximateDistinct" | "exactDistinct" | "$count"
? PropertyDef<"integer">
: PropertyDef<"double">
: V extends "$count" ? PropertyDef<"integer">
: never
>;
}

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> {
readonly selectProperty: <R extends PropertyKeys<Q>>(
propertyName: R,
) => WithPropertyDefinition<CompileTimeMetadata<Q>["properties"][R]>;
}

interface ManyLinkWithPropertyObjectSet<
Q extends ObjectOrInterfaceDefinition,
> extends AggregatableWithPropertyObjectSet<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>>;
}
8 changes: 8 additions & 0 deletions packages/api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ export type {
OsdkObjectLinksObject,
SingleLinkAccessor,
} from "./definitions/LinkDefinitions.js";
export type {
WithPropertiesClause,
WithPropertyDefinition,
} from "./derivedProperties/WithPropertiesClause.js";
export type {
WithPropertyObjectSet,
} from "./derivedProperties/WithPropertyObjectSet.js";

Copy link
Member

Choose a reason for hiding this comment

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

Extra new line causes grouping when organizing imports instead of perfect sorting.

export { DurationMapping } from "./groupby/GroupByClause.js";
export type {
AllGroupByValues,
Expand Down
39 changes: 39 additions & 0 deletions packages/api/src/objectSet/ObjectSet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ import type { AggregateOpts } from "../aggregate/AggregateOpts.js";
import type { AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy } from "../aggregate/AggregateOptsThatErrors.js";
import type { AggregationsResults } from "../aggregate/AggregationsResults.js";
import type { WhereClause } from "../aggregate/WhereClause.js";
import type {
WithPropertiesClause,
WithPropertyDefinition,
} from "../derivedProperties/WithPropertiesClause.js";
import type { PropertyValueWireToClient } from "../mapping/PropertyValueMapping.js";
import type {
AsyncIterArgs,
Augments,
Expand All @@ -34,7 +39,9 @@ import type {
} from "../ontology/ObjectOrInterface.js";
import type {
CompileTimeMetadata,
ObjectMetadata,
ObjectTypeDefinition,
PropertyDef,
} from "../ontology/ObjectTypeDefinition.js";
import type { PrimaryKeyType } from "../OsdkBase.js";
import type { ExtractOptions, Osdk } from "../OsdkObjectFrom.js";
Expand Down Expand Up @@ -254,4 +261,36 @@ export interface ObjectSet<
listener: ObjectSetListener<Q, P>,
opts?: ObjectSetListenerOptions<Q, P>,
) => { unsubscribe: () => void };

readonly withProperties: <
D extends WithPropertiesClause<Q>,
>(
clause: D,
) => ObjectSet<
WithPropertiesObjectDefinition<
Q,
D
Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with it for now but this feels like a place where having the simpler types that are not based on functions produces more readable types? We have to do the extraction work anyway to create things like properties and props below but we are required to keep the function hierarchy in the type generics as this object is passed around instead of resolving them at the time withProperties is called and thus carrying a lot less. Like we could just be carrying { foo: number, bar: string } lets say. Or even { foo: PropertyDefinition, bar: PropertyDefinition }.

>
>;
}

type WithPropertiesObjectDefinition<
K extends ObjectOrInterfaceDefinition,
D extends WithPropertiesClause<K>,
> = {
__DefinitionMetadata: {
properties: {
Copy link
Contributor Author

@nihalbhatnagar nihalbhatnagar Dec 10, 2024

Choose a reason for hiding this comment

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

This is all we need to propagate the types through all further object set operations. The FLUP here is to make the type of the whole object set extractable and definable so that a user can pass around object sets that are correctly typed with the additional properties and do not have to be inferred.

[T in Extract<keyof D, string>]: D[T] extends
Copy link
Member

Choose a reason for hiding this comment

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

I'd have to check but my gut is keyof D & string is better for the compiler than the conditional type that Extract causes.

(baseObjectSet: any) => WithPropertyDefinition<infer P> ? P
: never;
};
props: {
[T in Extract<keyof D, string>]: D[T] extends
(baseObjectSet: any) => WithPropertyDefinition<infer P>
? P extends PropertyDef<infer A, any, any>
? PropertyValueWireToClient[A]
: never
: never;
};
};
} & K;
Loading
Loading