feat: extend twenty orm

[magrinj]

This PR is a follow up of PR #5153.
This one introduce some changes on how we're querying composite fields.
We can do:

export class CompanyService {
  constructor(
    @InjectWorkspaceRepository(CompanyObjectMetadata)
    private readonly companyObjectMetadataRepository: WorkspaceRepository<CompanyObjectMetadata>,
  ) {}

  async companies(): Promise<CompanyObjectMetadata[]> {
    // Old way
    // const companiesFilteredByLinkLabel = await this.companyObjectMetadataRepository.find({
    //   where: { xLinkLabel: 'MyLabel' },
    // });
    // Result will return xLinkLabel property

    // New way
    const companiesFilteredByLinkLabel = await this.companyObjectMetadataRepository.find({
      where: { xLink: { label:  'MyLabel' } },
    });
    // Result will return { xLink: { label: 'MyLabel' } } property instead of  { xLinkLabel: 'MyLabel' }

    return companiesFilteredByLinkLabel;
  }
}

Also we can now inject TwentyORMManage class to manually create a repository based on a given workspaceId using getRepositoryForWorkspace function that way:

export class CompanyService {
  constructor(
    // TwentyORMModule should be initialized
    private readonly twentyORMManager,
  ) {}

  async companies(): Promise<CompanyObjectMetadata[]> {
    const repository = await this.twentyORMManager.getRepositoryForWorkspace(
      '8bb6e872-a71f-4341-82b5-6b56fa81cd77',
      CompanyObjectMetadata,
    );

    const companies = await repository.find();

    return companies;
  }
}

[charlesBochet]

Before looking at the code feedback:
I'm a big fan the second way twentyORMManager (I'm not a big fan of injecting repository by repository, it's too heavy). Is there any drawback to only use the second way?

[charlesBochet]

let's avoid using abbreviations (repo) in the code base. Also let's keep the { } around one liner if content instructions per project convention

[charlesBochet]

what is object literal storage?

[Weiko]

Shouldn't we name it EntitySchemaStorage? It seems you started to call your file like that already

[magrinj]

I'm actually storing ObjectLiteral as value and EntitySchema as key, os it's better to call it ObjectLiteralStorage.
This storage is actually used inside the WorkspaceRepository, it allow to get the corresponding ObjectLiteral (our metadata entity), to retrieve the composite fields.

[charlesBochet]

Really like the approach.
I'm still really favor of keeping objectMetadata, fieldMetadata, relationMetadata naming to distinguish from objectRecord, objectField, objectRelation (which is the data). Not specific to this PR but general direction

[Weiko]

I think this one is a bit dangerous since we don't provide the schema name here if I understood correctly. I guess ScopedWorkspaceDatasourceFactory should throw if the request object is not properly set and the token is not valid.
For other cases where we can't use the datasource without requests, the exception should tell us that we are probably not using the right class (for example if I'm using this in a command) as I don't think we want to use ScopedWorkspaceDatasourceFactory but explicitly the one that takes workspaceId as parameter in those cases (workspaceDataSourceFactory/TwentyORMManager). Wdyt?

[Weiko]

@magrinj I'd rather not return anything if possible? this datasource won't be usable without a proper schema defined anyway and you would have to use TwentyORMManager.getRepositoryForWorkspace right?

[magrinj]

It's a bit complicated on my opinion to return something null or undefined or throw.
As the datasource is created when we inject it inside the class and we can have authenticated or unauthenticated resolver inside this class.

[magrinj]

I've tested multiple things, and the "best" things I can do is returning null when workspace is not defined, so @InjectWorkspaceDataSource or @InjectWorkspaceReposiroty can now return null value.
We need to be careful with that as decorators doesn't enforce the type of the value under them.

[Weiko]

Nice API! It's a shame we need to override all those things from the repository to handle composite fields but I guess we don't have much choice. LGTM overall 👏

[Weiko]

Before looking at the code feedback: I'm a big fan the second way twentyORMManager (I'm not a big fan of injecting repository by repository, it's too heavy). Is there any drawback to only use the second way?

It means you will have to call await this.twentyORMManager.getRepositoryForWorkspace in each function of your service to get an instance of your repository instead of sharing one within your class, which you often do. Also it might be a bit harder to test. I like both ways I think it depends on your use case. Usually it shouldn't be too heavy, I don't expect us to inject 10+ repositories within the same service or maybe that means we need to separate our code better. Wdyt?

[magrinj]

Before looking at the code feedback: I'm a big fan the second way twentyORMManager (I'm not a big fan of injecting repository by repository, it's too heavy). Is there any drawback to only use the second way?

It means you will have to call await this.twentyORMManager.getRepositoryForWorkspace in each function of your service to get an instance of your repository instead of sharing one within your class, which you often do. Also it might be a bit harder to test. I like both ways I think it depends on your use case. Usually it shouldn't be too heavy, I don't expect us to inject 10+ repositories within the same service or maybe that means we need to separate our code better. Wdyt?

I agree with that, I think it's better to keep both API, as the injection one is really easier to use, and will be the most used in my opinion

[Weiko]

LGTM!