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

feat: add missing updateMany and deleteMany resolvers on flexible backend #2758

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

gitstart-twenty
Copy link
Contributor

Fixes #2721

…kend

Co-authored-by: v1b3m <vibenjamin6@gmail.com>
@@ -21,6 +21,10 @@ export const getResolverName = (
return `update${pascalCase(objectMetadata.nameSingular)}`;
case 'deleteOne':
return `delete${pascalCase(objectMetadata.nameSingular)}`;
case 'updateMany':
return `updateMany${pascalCase(objectMetadata.namePlural)}`;
Copy link
Member

Choose a reason for hiding this comment

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

@gitstart-twenty Juste use the name plural without the Many

Suggested change
return `updateMany${pascalCase(objectMetadata.namePlural)}`;
return `update${pascalCase(objectMetadata.namePlural)}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

case 'updateMany':
return `updateMany${pascalCase(objectMetadata.namePlural)}`;
case 'deleteMany':
return `deleteMany${pascalCase(objectMetadata.namePlural)}`;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return `deleteMany${pascalCase(objectMetadata.namePlural)}`;
return `delete${pascalCase(objectMetadata.namePlural)}`;

result,
targetTableName,
'update',
)?.records[0];
Copy link
Member

Choose a reason for hiding this comment

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

@gitstart-twenty It's a many query, so we should directly return records and not the first element of the array.

Suggested change
)?.records[0];
)?.records;

result,
targetTableName,
'deleteFrom',
)?.records[0];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
)?.records[0];
)?.records;

>(
args: DeleteManyResolverArgs<Filter>,
options: WorkspaceQueryRunnerOptions,
): Promise<Record | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
): Promise<Record | undefined> {
): Promise<Record[] | undefined> {

async updateMany<Record extends IRecord = IRecord>(
args: UpdateManyResolverArgs<Record>,
options: WorkspaceQueryRunnerOptions,
): Promise<Record | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
): Promise<Record | undefined> {
): Promise<Record[] | undefined> {

@gitstart-twenty
Copy link
Contributor Author

@magrinj Just one question, where are the return types for these auto-generated resolvers defined? Where do I specify that update[targetTableNamePlural] should not return Record but rather [Record!]
Screenshot 2023-11-29 at 15 19 40

@gitstart-twenty
Copy link
Contributor Author

When you check the return types of the mutations, you notice that not a single one returns an array(list) of items, all just return one item. For a quick example, even the create[collectionNamePlural] which one would expect to return the many created items, returns just one. Looks like this is the default behavior for these mutations(or at least it's what was implemented)

Screenshot 2023-11-30 at 08 10 25

@gitstart-twenty
Copy link
Contributor Author

@magrinj I've noticed the output types are defined in server > src > workspace > workspace-schema-builder > factories > root-type.factory.ts > RootTypeFactory > generateFields > outputType. In order for the type definitions not to default to a plain object, we need to make changes here and also make related modifications elsewhere. Should we return a connection for updateMany, deleteMany e.t.c

Screenshot 2023-11-30 at 11 16 45

@magrinj
Copy link
Member

magrinj commented Nov 30, 2023

@magrinj Just one question, where are the return types for these auto-generated resolvers defined? Where do I specify that update[targetTableNamePlural] should not return Record but rather [Record!] Screenshot 2023-11-29 at 15 19 40

In RootTypeFactory you should add the following changes:

--- root-type.factory.ts	2023-11-30 09:54:52
+++ root-type.factory.new.ts	2023-11-30 09:54:37
@@ -9,6 +9,7 @@
 import { TypeDefinitionsStorage } from 'src/workspace/workspace-schema-builder/storages/type-definitions.storage';
 import { getResolverName } from 'src/workspace/utils/get-resolver-name.util';
 import { getResolverArgs } from 'src/workspace/workspace-schema-builder/utils/get-resolver-args.util';
+import { TypeMapperService } from 'src/workspace/workspace-schema-builder/services/type-mapper.service';
 
 import { ArgsFactory } from './args.factory';
 import { ObjectTypeDefinitionKind } from './object-type-definition.factory';
@@ -25,6 +26,7 @@
 
   constructor(
     private readonly typeDefinitionsStorage: TypeDefinitionsStorage,
+    private readonly typeMapperService: TypeMapperService,
     private readonly argsFactory: ArgsFactory,
   ) {}
 
@@ -70,7 +72,7 @@
       for (const methodName of workspaceResolverMethodNames) {
         const name = getResolverName(objectMetadata, methodName);
         const args = getResolverArgs(methodName);
-        const outputType = this.typeDefinitionsStorage.getObjectTypeByKey(
+        const objectType = this.typeDefinitionsStorage.getObjectTypeByKey(
           objectMetadata.id,
           methodName === 'findMany'
             ? ObjectTypeDefinitionKind.Connection
@@ -84,7 +86,7 @@
           options,
         );
 
-        if (!outputType) {
+        if (!objectType) {
           this.logger.error(
             `Could not find a GraphQL type for ${objectMetadata.id} for method ${methodName}`,
             {
@@ -99,6 +101,10 @@
           );
         }
 
+        const outputType = this.typeMapperService.mapToGqlType(objectType, {
+          isArray: ['updateMany', 'deleteMany'].includes(methodName),
+        });
+
         fieldConfigMap[name] = {
           type: outputType,
           args: argsType,

@magrinj
Copy link
Member

magrinj commented Nov 30, 2023

When you check the return types of the mutations, you notice that not a single one returns an array(list) of items, all just return one item. For a quick example, even the create[collectionNamePlural] which one would expect to return the many created items, returns just one. Looks like this is the default behavior for these mutations(or at least it's what was implemented)

Good catch on that, I guess it's an issue, if you can fix it and update the above patch to include createMany :)

@gitstart-twenty
Copy link
Contributor Author

gitstart-twenty commented Nov 30, 2023

Hey @magrinj,
Thanks for the hints and the patch. I believe we're now good.

Screenshot 2023-11-30 at 13 30 02

Copy link
Member

@magrinj magrinj left a comment

Choose a reason for hiding this comment

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

Great job, thanks for your work !

@magrinj magrinj merged commit 1822370 into main Nov 30, 2023
9 of 14 checks passed
@magrinj magrinj deleted the TWNTY-2721 branch November 30, 2023 12:13
@charlesBochet charlesBochet mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add missing updateMany and deleteMany resolvers on flexible backend
3 participants