Import full distant schema and store in remote server #5211
Conversation
TODOs/FIXMEs:
|
| remoteServer: RemoteServerEntity<RemoteServerType>, | ||
| workspaceId: string, | ||
| ): Promise<string[]> { | ||
| if (!remoteServer.schema) { |
There was a problem hiding this comment.
I don't see where we ask the schema name from the UI. Is it planned for an upcoming UI? Currently it's failing with this check
There was a problem hiding this comment.
Also, I wouldn't throw an "Error" here, this does not seem to be internal issue but more tied to the user input and we don't want to capture such exceptions to Sentry for example.
Can you throw a BadRequestException? Ideally in the future we will have specific exceptions internal to twenty and not tied to REST/Graphql implementations but in the meantime let's use that!
There was a problem hiding this comment.
Ideally in the future we will have specific exceptions internal to twenty and not tied to REST/Graphql implementations but in the meantime let's use that!
cc @charlesBochet 😄
There was a problem hiding this comment.
Waiting on @ijreilly's work to add the schema as input!
There was a problem hiding this comment.
Finally added it in this PR
| @@ -181,14 +175,14 @@ export class RemoteTableService { | |||
| localTableName, | |||
| input, | |||
| remoteServer, | |||
| remoteTableColumns, | |||
| distantTableColumns, | |||
| ); | |||
|
|
|||
| await this.createRemoteTableMetadata( | |||
There was a problem hiding this comment.
This should be in a transaction to avoid having a foreign table created without metadata
There was a problem hiding this comment.
this requires a refacto that should not be done in this PR IMO. The service fieldMetadataService is doing a transaction for each creation. We would need to send a manager as input so it uses it when defined
| case RemoteServerType.POSTGRES_FDW: | ||
| return FeatureFlagKeys.IsPostgreSQLIntegrationEnabled; | ||
| default: | ||
| throw new BadRequestException('PostgreSQL integration is not enabled'); |
There was a problem hiding this comment.
I don't think this is the right error message here. Should probably be ${remoteServerType} integration is not supported or something, you check postgresql integration above already
| case 'int4': | ||
| return FieldMetadataType.NUMBER; | ||
| default: | ||
| return FieldMetadataType.TEXT; |
There was a problem hiding this comment.
I'm wondering if we should throw here instead to avoid side effects with unhandled types
There was a problem hiding this comment.
Good point. We would throw often, but at least we will know when a type is not supported. Will do in another PR, I should look for all types available
| // TODO: return error to the user when a column cannot be managed | ||
| try { | ||
| const field = await this.fieldMetadataService.createOne({ | ||
| name: column.columnName, |
There was a problem hiding this comment.
To check if this works with column names with snake_case, I believe this is rejected as of today (you can try by calling a column "created_at" in your distant DB) to be compliant with graphql type names
There was a problem hiding this comment.
I updated the logic to use camelCase !
We should not depend on the foreign data wrapper type to manage distant table. The remote server should be enough to handle the table creation. Here is the new flow to fetch available tables: - check if the remote server have available tables already stored - if not, import full schema in a temporary schema - copy the tables into the available tables field - delete the schema Left todo: - update remote server input for postgres so we receive the schema --------- Co-authored-by: Thomas Trompette <[email protected]>
We should not depend on the foreign data wrapper type to manage distant table. The remote server should be enough to handle the table creation.
Here is the new flow to fetch available tables:
Left todo: