[feat] Add updateRemoteServer endpoint #5148
Conversation
| userMappingOptions: UserMappingOptions, | ||
| ) { | ||
| // CURRENT_USER works for now since we are using only one user. But if we switch to a user per workspace, we need to change this. | ||
| return `ALTER USER MAPPING FOR CURRENT_USER SERVER "${foreignDataWrapperId}" OPTIONS (SET user '${userMappingOptions.username}', SET password '${userMappingOptions.password}')`; |
There was a problem hiding this comment.
Here you assumes that all fields will always be present. But we should only update the fields that are defined
| remoteServerId: string; | ||
| workspaceId: string; | ||
| }) { | ||
| const tables = await this.remoteTableRepository.find({ |
There was a problem hiding this comment.
return this.remoteTableRepository.find({...
Then you do await findCurrentRemoteTablesByServerId. This way you let the caller decide if he wants to use the function asynchronously or synchronously
packages/twenty-server/src/engine/metadata-modules/remote-server/remote-server.service.ts
Show resolved
Hide resolved
|
|
||
| @IsOptional() | ||
| @Field(() => GraphQLJSON, { nullable: true }) | ||
| userMappingOptions?: UserMappingOptions; |
There was a problem hiding this comment.
Let's add some form of validation on this json type directly from graphql by creating an input type
@InputType()
class UserMappingOptionsInput {
@Field(() => String)
@IsString()
username: string;
@Field(() => String)
@IsString()
password: string;
}
or something like that, then
@IsOptional()
@Field(() => UserMappingOptionsInput, { nullable: true })
userMappingOptions?: UserMappingOptionsInput;
If you don't want a new input type, you can also use class-validator and validate your json in your resolver like
class UserMappingOptionsValidator {
@IsDefined()
@IsString()
username: string;
@IsDefined()
@IsString()
password: string;
}
and then use validateOrReject from 'class-validator' package.
What do you think?
| remoteServerInput: UpdateRemoteServerInput<T>, | ||
| workspaceId: string, | ||
| ): Promise<RemoteServerEntity<RemoteServerType>> { | ||
| validateObject(remoteServerInput.foreignDataWrapperOptions); |
There was a problem hiding this comment.
I struggled to understand what validateObject does as it's very broad, I think we should fine a more explicit name @thomtrp
| @@ -13,7 +13,7 @@ export const validateObject = (input: object) => { | |||
| } | |||
| }; | |||
|
|
|||
| export const validateString = (input: string) => { | |||
| export const validateStringRemoteServerInput = (input: string) => { | |||
There was a problem hiding this comment.
I renamed this but I am not sure what this validation is for on the id? @thomtrp
| userMappingOptions?: Partial<UserMappingOptions>; | ||
| } | ||
|
|
||
| @InputType() |
There was a problem hiding this comment.
can you put it in a separated file and use it in the create input too please?
packages/twenty-server/src/engine/metadata-modules/remote-server/remote-server.service.ts
Outdated
Show resolved
Hide resolved
|
|
||
| if (remoteServerInput.userMappingOptions) { | ||
| validateObjectRemoteServerInput(remoteServerInput.userMappingOptions); | ||
| } |
There was a problem hiding this comment.
let's have a global validation for create and update
| [P in keyof T]?: DeepPartial<T[P]>; | ||
| }; | ||
|
|
||
| export const updateRemoteServerRawQuery = ( |
There was a problem hiding this comment.
suffix file name with .utils
There was a problem hiding this comment.
and please separate the functions that create the userMapping and the other options
packages/twenty-server/src/engine/metadata-modules/utils/validate-string-input.utils.ts
Outdated
Show resolved
Hide resolved
| @InputType() | ||
| export class UpdateRemoteServerInput<T extends RemoteServerType> { | ||
| @Field(() => String) | ||
| id: T; |
There was a problem hiding this comment.
its a mistake thanks for spotting this !
| continue; | ||
| } | ||
|
|
||
| if (!value || !INPUT_REGEX.test(value.toString())) { |
There was a problem hiding this comment.
sorry I think there is a misunderstood there. I don't think this is the role of this function to check if the value is not empty. If no value, just continue. If value, call validateStringAgainstInjections(value.toString())
## Context twentyhq#4765 Following investigations ([twentyhq#5083](twentyhq#5083)) we decided to restrict updates of server from which zero tables have been synchronized only ## How was it tested Locally with /metadata 1. Updating a database that already has synchronized tables <img width="1072" alt="Capture d’écran 2024-04-24 à 16 16 05" src="https://github.com/twentyhq/twenty/assets/51697796/f9a84c34-2dcd-4f3c-b0bc-b710abae5021"> 2. Updating a database that has no synchronized tables <img width="843" alt="Capture d’écran 2024-04-24 à 16 17 28" src="https://github.com/twentyhq/twenty/assets/51697796/f320fe03-a6bc-4724-bcd0-4e89d3ac31f5"> + tested that the connection works well
Context
#4765
Following investigations (#5083) we decided to restrict updates of server from which zero tables have been synchronized only
How was it tested
Locally with /metadata