Adding bulk actions to 1.1 #53

ssanjay1 · 2024-02-12T14:57:10Z

We added bulk actions to the top level client ontology due to difficulties wrangling with the types that would conditionally enforce options and return types based on what parameters were passed in (array vs single input)

changelog-app · 2024-02-12T14:57:16Z

Generate changelog in `packages/generator/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Adding bulk actions to 1.1

Generate changelog in `packages/legacy-client/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Adding bulk actions to 1.1

Generate changelog in `packages/shared.test/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Adding bulk actions to 1.1

Check the box to generate changelog(s)

Generate changelog entry

ssanjay1

Left some comments as well, also general question, where is the best place to expose some sort of docs to indicate that the bulk actions endpoint only supports up to 20 requests at a time?

ssanjay1 · 2024-02-12T17:09:09Z

packages/legacy-client/src/client/baseTypes/ActionType.ts

@@ -141,8 +163,8 @@ export const ActionResponse = {
      ],
    };
    if (response.edits?.type === "edits") {
-      const added = [];
-      const modified = [];
+      const added: any[] = [];


Didn't touch this, but for some reason had to add this type, or else checks error out with something along the lines of :
Argument of type '{ apiName: string; primaryKey: any; get: () => Promise<Result<OntologyObject<string, NonNullable<ParameterValue>>, GetObjectError>>; }' is not assignable to parameter of type 'never'.ts(2345)

Hmm. I am a little worried this means we broke something somewhere. If we need to keep a change like this we could at least type it to the real shape we are pushing to prevent future errors.

ssanjay1 · 2024-02-12T17:20:29Z

packages/legacy-client/src/client/ontology.ts

@@ -38,6 +41,10 @@ export class Ontology<O extends OntologyDefinition<any>> {
    return createActionProxy(this.#client);
  }

+  get bulkActions(): BulkActions<O> {


I only added this here, and not in the actual OntologyDefinition since this seems like we're only adding this for 1.1

That would be correct, since there is no custom definition for actions vs bulk actions. Only the runtime client.ontology. should be showing bulkActions.

ssanjay1 · 2024-02-12T17:39:26Z

packages/legacy-client/src/client/actions/actions.ts

+    IsEmptyRecord<O["actions"][A]["parameters"]> extends true
+      ? <Op extends BulkActionExecutionOptions>(
+        options?: Op,
+      ) => WrappedBulkActionReturnType<O, A, Op>


Right now I'm just following pattern of how we've handled other actions with no parameters, but the way its setup bulk actions don't really work for actions without parameters. This could be modified so it accepts an array of {} potentially? And we just use those to tell us how many empty requests we need for the batch endpoint?

Yeah, I think {}[] or undefined[] is better than nothing, otherwise you arent really calling bulk.

Would undefined[] be preferable here? Because with {}[]won't that just let you pass in any sort of object https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBDAnmYcBCBXANgawIIDGMAlhAHYCiIwBGJ5A8mPWQM5wC8cA3gFBxwAthAAmwAPwAuOGQyCARsCgBufnCjAYGKJRHEYrKTLmKocXgF9eYglgCGGuADMMZIqTLOyACghhpmLiELFQ0dB5MLKwAlNIAbhDEIqq8Tj7c5gICAPTZcMRkMMBYWMSswGyorAAWEADu7AAGwmKNltG8vKCQsAjIqIH47uShtCyRHqwATJw8ahpaOhR6BkayCkrmVjb2ji5uLF5TADwACnCgRWQi7IPBHqPhjMyTUwB8vv5wp7FwCUmqNJTbx8LJCUTAaQAVlyMgg8CwwAA5nYsHAwA47IJ8k44IgIBg4AArDCseAQQT6dqdIHHO7DSjUMYRF7kVgfUFZFqQuAwvJkeFwREotEYqBYnF4gnE0nkykwaldcDQeBIFBweTYHDAQTMRCzTkFapKSHcCwAbQAuqorLwCGyEZoyVBEJJNbgdXqDUaTZJzdwAJIEOzXerVOxFOJKSQARgsABoeBZLRYgA

But also feel like it could be weird syntax to do something like client.ontology.bulkActions.noParamaction([undefined, undefined,...], {options})?

Oh wait could we use Record<string,never>[]?

ssanjay1 · 2024-02-12T17:40:57Z

packages/legacy-client/src/client/baseTypes/ActionType.ts

@@ -30,6 +33,11 @@ export type ActionExecutionOptions = {
  returnEdits?: ReturnEditsMode;
 };

+export type BulkActionExecutionOptions = {
+  mode?: never;


Don't want users trying to pass in any sort of execution mode as that isn't supported by the endpoint

The problem with this though is it is suggested in intellisense even though it can only be undefined.

It can just be omitted right? https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBDAnmYcBCBXANgawIIDGMAlhAHYCiIwBGJ5A8mPWQM5wC8cA3gFBxwAthAAmwAPwAuOGWAA3YFADc-OFGAwMUSiOIxWUmRkEAjRXF4BfXmIJYAhurgAzDGSKkyLsgAoIYaUxcQhYqGjpPJhZWAEppOQhiERVeZ18+AQEAeiy4YjIYYCwsYlZgNlRWAAsIAHd2AANhMQarGN5eUEhYBGRUIPwPcjDaFijPVgAmTh5VdU1tCl19QzJjMygLa1sHJ1d3Fm9JvwD0bEHQ6lHI5gnJuLgEpJU044zM5uBpAFYlOByZBB4FhgABzexYOBgRz2QR5ZxwRAQDBwABWGFY8Aggj0bSAA

Ah yea you're right, I think I needed this before when we tried the original syntax and I had some conditional types for the options, but yea I think I can just omit now, on it

styu

defer to @ericanderson on the meat of the changes

styu · 2024-02-12T18:11:50Z

examples/one_dot_one/src/generatedNoCheck/Ontology.ts

@@ -58,5 +59,6 @@ export const Ontology: {
 export interface Ontology extends ClientOntology<typeof Ontology> {
  objects: Objects;
  actions: Actions;
+  bulkActions: BulkActions;


can you mention rationale in the PR description as to why we add it at the top level vs within actions, for posterity?

ericanderson

Some notes to fix up before merging.

ericanderson · 2024-02-12T20:14:41Z

packages/generator/src/v1.1/generateBulkActions.ts

+    const entries = Object.entries(action.parameters);
+
+    const modifiedEntityTypes = getModifiedEntityTypes(action);
+    const addedObjects = Array.from(modifiedEntityTypes.addedObjects);


I think .addedObjects/.modifiedObjects is a Set<string>. So we are converting it to an array so we can forEach add it to a different set. At the very least you could just forEach on the returned sets instead of the memory copy.

Ah yea I just pulled this from the generateActions, it looks like we converted to Array so we could just do a simple join on line 74 when generating the edits return type

ericanderson · 2024-02-12T20:18:29Z

packages/legacy-client/src/client/actions/actions.ts

+    IsEmptyRecord<O["actions"][A]["parameters"]> extends true
+      ? <Op extends BulkActionExecutionOptions>(
+        options?: Op,
+      ) => WrappedBulkActionReturnType<O, A, Op>


Yeah, I think {}[] or undefined[] is better than nothing, otherwise you arent really calling bulk.

ericanderson · 2024-02-12T20:19:23Z

packages/legacy-client/src/client/actions/createActionProxy.ts

-          return async function<Op extends ActionExecutionOptions>(
+          return async function<
+            Op extends ActionExecutionOptions,
+          >(


nit: unnecessary change

ericanderson · 2024-02-12T20:19:38Z

packages/legacy-client/src/client/actions/createActionProxy.ts

            params: ActionArgs<O, typeof p>,
            options?: Op,
          ): Promise<WrappedActionReturnType<O, typeof p, Op>> {
-            return executeAction<O, typeof p, Op>(client, p, params, options);
+            return executeAction<O, typeof p, Op>(


nit: unnecessary change

ericanderson · 2024-02-12T20:22:53Z

packages/legacy-client/src/client/baseTypes/ActionType.ts

@@ -30,6 +33,11 @@ export type ActionExecutionOptions = {
  returnEdits?: ReturnEditsMode;
 };

+export type BulkActionExecutionOptions = {
+  mode?: never;


The problem with this though is it is suggested in intellisense even though it can only be undefined.

It can just be omitted right? https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBDAnmYcBCBXANgawIIDGMAlhAHYCiIwBGJ5A8mPWQM5wC8cA3gFBxwAthAAmwAPwAuOGWAA3YFADc-OFGAwMUSiOIxWUmRkEAjRXF4BfXmIJYAhurgAzDGSKkyLsgAoIYaUxcQhYqGjpPJhZWAEppOQhiERVeZ18+AQEAeiy4YjIYYCwsYlZgNlRWAAsIAHd2AANhMQarGN5eUEhYBGRUIPwPcjDaFijPVgAmTh5VdU1tCl19QzJjMygLa1sHJ1d3Fm9JvwD0bEHQ6lHI5gnJuLgEpJU044zM5uBpAFYlOByZBB4FhgABzexYOBgRz2QR5ZxwRAQDBwABWGFY8Aggj0bSAA

ericanderson · 2024-02-12T20:25:41Z

packages/legacy-client/src/client/baseTypes/ActionType.ts

@@ -141,8 +163,8 @@ export const ActionResponse = {
      ],
    };
    if (response.edits?.type === "edits") {
-      const added = [];
-      const modified = [];
+      const added: any[] = [];


Hmm. I am a little worried this means we broke something somewhere. If we need to keep a change like this we could at least type it to the real shape we are pushing to prevent future errors.

ericanderson · 2024-02-12T20:25:51Z

packages/legacy-client/src/client/baseTypes/ActionType.ts

+  ): BulkActionResponse<Edits<TAddedObjects, TModifiedObjects> | undefined> => {
+    if (response.edits?.type === "edits") {
+      const added: any[] = [];
+      const modified: any[] = [];


ericanderson · 2024-02-12T20:27:34Z

packages/legacy-client/src/client/baseTypes/ActionType.ts

+    if (response.edits?.type === "edits") {
+      const added: any[] = [];
+      const modified: any[] = [];
+      for (const edit of response.edits.edits) {


This seems like a copy/paste of the above. Maybe extract function for getting the edits shape ({type:"edits", added,modified})?

ericanderson · 2024-02-12T20:28:40Z

packages/legacy-client/src/client/net/executeAction.ts

+        {
+          requests: params
+            ? remapBulkActionParams<O, A>(params)
+            : [],


Given empty array is used here, does this mean that any actions that take zero arguments can never be called in bulk? If thats the intention, should we just exclude them in the type mapping for creating bulk actions?

ericanderson · 2024-02-12T20:30:40Z

packages/legacy-client/src/client/ontology.ts

@@ -38,6 +41,10 @@ export class Ontology<O extends OntologyDefinition<any>> {
    return createActionProxy(this.#client);
  }

+  get bulkActions(): BulkActions<O> {


That would be correct, since there is no custom definition for actions vs bulk actions. Only the runtime client.ontology. should be showing bulkActions.

:

second attempt bulk actions

93b6a91

ssanjay1 added 3 commits February 12, 2024 11:17

bulk actions fix

5fa8a4f

refactor

45d9d57

one more refactor

6e866dc

ssanjay1 commented Feb 12, 2024

View reviewed changes

Add generated changelog entries

9e07b3a

styu reviewed Feb 12, 2024

View reviewed changes

ericanderson reviewed Feb 12, 2024

View reviewed changes

ssanjay1 added 3 commits February 12, 2024 16:08

Fix bulk action type

db62515

remove anys

3357ae8

use helper function

722ecd8

ericanderson approved these changes Feb 13, 2024

View reviewed changes

ssanjay1 added 5 commits February 14, 2024 13:40

Merge branch 'main' into ssanjay/bulkActions2

fb10026

add bulk no params options

78f048c

restrict object

0fa395a

:

fix generate type

493acd0

Merge branch 'main' into ssanjay/bulkActions2

30fce6a

ssanjay1 merged commit e2964be into main Feb 14, 2024
6 checks passed

ssanjay1 deleted the ssanjay/bulkActions2 branch February 14, 2024 22:05

ssanjay1 mentioned this pull request Feb 15, 2024

Add bulk actions #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding bulk actions to 1.1 #53

Adding bulk actions to 1.1 #53

ssanjay1 commented Feb 12, 2024 •

edited

Loading

changelog-app bot commented Feb 12, 2024 •

edited by ssanjay1

Loading

ssanjay1 left a comment

ssanjay1 Feb 12, 2024

ericanderson Feb 12, 2024

ssanjay1 Feb 12, 2024

ericanderson Feb 12, 2024

ssanjay1 Feb 12, 2024

ericanderson Feb 12, 2024

ssanjay1 Feb 12, 2024

ssanjay1 Feb 12, 2024 •

edited

Loading

ssanjay1 Feb 12, 2024

ssanjay1 Feb 12, 2024

ericanderson Feb 12, 2024

ssanjay1 Feb 12, 2024

styu left a comment

styu Feb 12, 2024

ericanderson left a comment

ericanderson Feb 12, 2024

ssanjay1 Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

ericanderson Feb 12, 2024

Adding bulk actions to 1.1 #53

Adding bulk actions to 1.1 #53

Conversation

ssanjay1 commented Feb 12, 2024 • edited Loading

changelog-app bot commented Feb 12, 2024 • edited by ssanjay1 Loading

Generate changelog in packages/generator/changelog/@unreleased

Generate changelog in packages/legacy-client/changelog/@unreleased

Generate changelog in packages/shared.test/changelog/@unreleased

ssanjay1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssanjay1 Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssanjay1 commented Feb 12, 2024 •

edited

Loading

changelog-app bot commented Feb 12, 2024 •

edited by ssanjay1

Loading

Generate changelog in `packages/generator/changelog/@unreleased`

Generate changelog in `packages/legacy-client/changelog/@unreleased`

Generate changelog in `packages/shared.test/changelog/@unreleased`

ssanjay1 Feb 12, 2024 •

edited

Loading