-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add structs read support #1047
Add structs read support #1047
Changes from all commits
8951fb6
7d5605c
71dcfb2
a8be10e
b6ae16d
5628328
1de9dcd
d7b6ec9
20adee1
fa6013e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
--- | ||
"@osdk/foundry-sdk-generator": patch | ||
"@osdk/generator-converters": patch | ||
"@osdk/cli.cmd.typescript": patch | ||
"@osdk/shared.test": patch | ||
"@osdk/generator": patch | ||
"@osdk/client": patch | ||
"@osdk/maker": patch | ||
"@osdk/api": patch | ||
--- | ||
|
||
Add support for reading struct properties. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,10 @@ | |
import type { | ||
AggregateOpts, | ||
AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy, | ||
CompileTimeMetadata, | ||
GroupByClause, | ||
ObjectOrInterfaceDefinition, | ||
PropertyKeys, | ||
ValidAggregationKeys, | ||
} from "@osdk/api"; | ||
import type { Employee } from "@osdk/client.test.ontology"; | ||
|
@@ -37,6 +40,8 @@ import { | |
type Mock, | ||
vi, | ||
} from "vitest"; | ||
import type { Client } from "../Client.js"; | ||
import { createClient } from "../createClient.js"; | ||
import { createMinimalClient } from "../createMinimalClient.js"; | ||
import type { MinimalClient } from "../MinimalClientContext.js"; | ||
import { aggregate } from "./aggregate.js"; | ||
|
@@ -50,6 +55,7 @@ const metadata = { | |
|
||
let mockFetch: Mock; | ||
let clientCtx: MinimalClient; | ||
let client: Client; | ||
|
||
beforeAll(() => { | ||
mockFetch = vi.fn(); | ||
|
@@ -63,10 +69,18 @@ beforeAll(() => { | |
clientCtx = createMinimalClient( | ||
metadata, | ||
"https://host.com", | ||
async () => "", | ||
async () => "myAccessToken", | ||
{}, | ||
mockFetch, | ||
); | ||
|
||
client = createClient( | ||
"https://host.com", | ||
metadata.ontologyRid, | ||
async () => "", | ||
undefined, | ||
mockFetch, | ||
); | ||
}); | ||
|
||
const aggregationResponse: AggregateObjectsResponseV2 = { | ||
|
@@ -171,13 +185,7 @@ describe("aggregate", () => { | |
> | ||
>(false); // subSelect should hide unused keys | ||
|
||
const grouped = await aggregate( | ||
clientCtx, | ||
objectTypeWithAllPropertyTypes, | ||
{ | ||
type: "base", | ||
objectType: "ToDo", | ||
}, | ||
const grouped = await client(objectTypeWithAllPropertyTypes).aggregate( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason type tests were having issues when using the aggregate function directly, but were all good when using the user exposed aggregate function. This should be fine since the user facing stuff works, but just flagging There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, that seems off. what kind of issues? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason, the aggregate function was not capturing clause types correctly and everything was still in generic form (e.g. like it wasn't getting the concrete information passed in |
||
{ | ||
$select: { | ||
"id:approximateDistinct": "unordered", | ||
|
@@ -509,13 +517,7 @@ describe("aggregate", () => { | |
}); | ||
|
||
it("prohibits ordered select with multiple groupBy", async () => { | ||
aggregate( | ||
clientCtx, | ||
Todo, | ||
{ | ||
type: "base", | ||
objectType: "ToDo", | ||
}, | ||
client(Todo).aggregate( | ||
{ | ||
$select: { | ||
// @ts-expect-error | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
WireAndStructPropertyTypes
? That way we don't breakWirePropertyTypes
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked a bit on slack, just to clarify here as well, the reason you think this is a break is because now instead of just a list of strings, its
SimpleWirePropertyTypes
and the record type? Because technically this boils down to the same thing and is just an additive change, but if someone is using this type, could be confusing since now it referencs something calledSimpleWirePropertyTypes
?