## Context
Following https://github.com/twentyhq/twenty/pull/16399
Now using the new global orm manager everywhere and returning a
GlobalDatasource/WorkspaceDatasource based on a feature flag.
This means we now need to wrap all our ORM calls within
executeInWorkspaceContext callback (at least for now) so the global
datasource can dynamically hydrate its context via the new store (the
global datasource does not store anything related to workspaces as it is
now a unique singleton). If feature flag is off it still uses local data
stored in the workspace datasource.
Should be merged once https://github.com/twentyhq/twenty/pull/16206 has
been released + command run to prod
In this PR
- Remove usage of viewGroup.fieldMetadataId, both in BE and FE states.
- But we still need to properly populate it until we fully remove
viewGroup.fieldMetadataId from db and ORM entity (upcoming 3rd PR out of
3). fieldMetadataId was removed from CoreViewGroup type and
CreateViewGroupInput and is determined BE-side based on the associated
view's mainGroupByFieldMetadataId. **I expect this means a downtime on
viewGroup creation, until both FE and BE are deployed and cache is
flushed.** This seems acceptable to me as it only regards viewGroup
creation.
- this information is replaced by view.mainGroupByFieldMetadataID
- Handle view group creation, update and deletion in the BE as a
side-effect of a view creation, update or deletion. Optimistic effects
are still used
- Add validation at view creation or update regarding
mainGroupByFieldMetadata
Left to do in 3rd PR
- Remove viewGroup.fieldMetadataId from db and ORM entity
- Restore feature allowing to update an existing grouped view's group by
field (already OK on BE side but need to rebuild FE optimistic)
In this PR (1/3)
- introduce view.mainGroupByFieldMetadataId as the new reference
determining which fieldMetadataId is used in a grouped view, in order to
deprecate viewGroup.fieldMetadataId which creates inconsistencies.
view.mainGroupByFieldMetadataId is now filled at every view creation,
though not in use yet.
- Introduce a command to backfill view.mainGroupByFieldMetadataId for
existing views + delete all viewGroup.fieldMetadataId with a
fieldMetadataId that is not view.mainGroupByFieldMetadataId. (It should
concern 37 active workspaces)
- Temporarily disable the option to change a grouped view's
fieldMetadataId as for now it creates inconsistencies. This feature can
be reintroduced when we have done the full migration.
In a next PR
- (2/3) use view.mainGroupByFieldMetadataId instead of
viewGroup.fieldMetadataId. In FE we may keep viewGroup.fieldMetadataId
as a state (TBD). View groups will now be created / deleted as a side
effect of view's mainGroupByFieldMetadataId update.
- (3/3) remove viewGroup.fieldMetadataId
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
## Context
Deprecating the old objectMetadataMap type in favour of split flat
entities to match with our new caching.
In the long run, trying to achieve:
- Better performance through caching
- Consistent data access patterns across the codebase
- Reduced database queries
Now that everything is based on flat entities, which are cached, we can
finish the refactoring of workspace context cache which should already
improve performances.
Then the last step will be to consume that new cache in the new global
datasource to get rid of the many workspace datasources stored in the
server
While investigating the issue a customer was facing, I discovered that
before records and after records could be in different order, making the
orm event and timeline activity engine mix records
- on draft creation, do not fetch the full version. Avoid the fetch of
steps and trigger
- on activation, we were performing 8 queries/mutations synchronously +
2 additional for automated triggers. I refacto the call it it gets
reduced to 4 queries/mutations + 2 additional for automated triggers
Soft deletion events use before in event properties. But we set the
after instead, that only contains a few data.
Fixes https://github.com/twentyhq/twenty/issues/15120
Debugging raised an additional issue. UpdateMany, RestoreMany and
SoftDeleteMany were broken for custom objects. The `where` clause was
using "_objectName.id" instead of "objectName.id" during select. That's
why we need to use different where clause between the select of the
existing value and the actual mutation.
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
Implements permission intersection (AND logic) to prevent permission
escalation when agents act on behalf of users.
### Changes:
- **Permission Intersection**: Operations requiring both user AND agent
permissions
- **RoleContext Type**: Unified type supporting single `roleId` or
multiple `roleIds` for intersection
- **CRUD Services**: Updated to accept `roleContext` for granular
permission control
- **Agent Integration**: Chat agents now use user + agent role
intersection for all operations
- **ORM Layer**: Enhanced `getRepository` to support multi-role
permission checks
### Related:
- Part 2 of ["Acting on behalf of user" concept
PR](https://github.com/twentyhq/twenty/pull/15103)
[Closes#1661](https://github.com/twentyhq/core-team-issues/issues/1661)
---------
Co-authored-by: Félix Malfait <felix.malfait@gmail.com>
In this PR
- Introduction of readableFields and updatableFields in
objectMetadataItem selector to ease filtering from a developer
experience perspective ( + to help developers think to do it). In
discussion @lucasbordeau @charlesBochet
- Remove non-updatable field from CSV import process (@etiennejouan)
- QA fix / Non-readable fields should not show on show page
- QA fix / It should not be offered to create a kanban view on a
non-readable field
- QA fix / It should not be offered to create view groups on a
non-readable field
- QA fix / Rating field should have a readonly mode
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
In this PR
1. Fix delete and soft-delete repository methods for repositories where
permission checks are NOT bypassed: they need to have a selection of
columns to return by default. To match what we did for insert I set it
to `'*'` by default. But I feel this may be bug prone as developers will
not necessarily think to fill the right values in. Maybe we should
change the api to use selectable fields by default. @charlesBochet
2. Add field permission seeds and enable field permission feature flag
in dev
We had two mistakes in updateMany orm behavior:
- format of manyInput should be done earlier
- relation connect/disconnect does not support batching and should be
computed for each input at last moment
In this PR:
- refactor timelineActivity computation to batch it
- make sure to pass the authContext to insert-query-builder in order to
pass user information to timelineActivity
- refactor message-participant /calendar-participant creation to batch
more
- fix favorite on view race condition (FE)
- deprecate PARTIAL_CALENDAR_EVENT_FETCH_LIST syncStage as we will
deprecate partial vs full notion (we will just leverage cursor emptyness
or not)
- introduce calendar / messging SCHEDULED syncStage that will allow
better performance granularity later
- activate quick message import after message list fetch to speed
performance on small message lists
In this PR we adapt field permission to two things
1. Connect: the recent insertion of the connect feature introduces the
possibility to have "connect" objects in typeORM's expressionMap's
valueSet, where we previously only had column names (if i followed
correctly). For instance for a person object that a N - 1 relationship
to company, person will have both companyId and company as possible
valueSet keys for the upsert. We need to reflect that in our
`getColumnNameToFieldMetadataIdMap` util that returns a map containing
every possible value we could encounter in valueSet. In an attempt to
tie this to the schema generation where this is introduced, I created
the shallow util extractGraphQLRelationFieldNames (probably ill-named -
im willing to update the name if @etiennejouan has a better idea?) to
remind us that these two are linked.
2. CreateMany: When calling query builders methods directly on custom
object (like we do in graphql-create-many-resolver), we need to be
careful to call them with a selection of readable fields. We had a
debate on whether we should compute this selection containing all
readable fields by default under the hood when no selected fields are
indicated. I am still not 100% convinced as I think it should remain the
caller's responsibility, but this case reminds us that it could easily
be forgotten by developers - although it is all the more the case as we
don't have seeds yet that help us realize that ([PR on the
way](https://github.com/twentyhq/twenty/pull/13646)).
- Removed caching of IMAP client as it was problematic and didn't really
matter because of our pipeline, also added retry logic to client.
- Some code refactoring and cleanup
- Breaking changes:
- Removed messageChannel.syncCursor
- Uses messageFolder.syncCursor instead
- Removed Date based sync cursor in favor of message UID which is much
more reliable and precise
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
In this PR:
- adding a try / catch around all ORM internal methods save, insert,
upsert, findOne, ...
- leveraging this error to prevent messageChannels to get FAILED
- optimizing messaging BATCH_SIZE and THROTTLE threshold according to
local tests
-
<img width="1510" height="851" alt="image"
src="https://github.com/user-attachments/assets/802fd933-caac-4291-9cde-34a1ddf59c06"
/>
This PR aims to improve performances as we are making A LOT of queries
after having added events emission to the ORM layer. While investigating
issues, I've come across multiple problems I will describe below
## Add index on workspace.activationStatus as we are querying it a lot
As per title
## Add logs on core datasource destroy
It seems that we have postgres connection pool destruction in
production. I cannot reproduce locally but I suspect the Query Timeouts
to be the root cause. I'm fixing most of the Query Timeouts cause in
this PR but I'm adding the logs so we have more information in case in
keeps happening in production.
## GraphQL query runner createMany
It was using a for loop on each record. This is as issue has we will
emit an event separately for each record => we should always try to
batch events.
Replacing by a save on all records. Note that this is not perfect as we
should avoid using save (bad performances), and use insert + updateMany
instead. As I have follow up discussions regarding permissions, I
haven't replaced it by insert and updateMany yet. Using save instead of
a for loop makes the problem less worrying.
## Introduce updateMany in ORM @Weiko @ijreilly FYI
.save(manyRecords) is bad as it's querying the data for no good reason
(and doing a select for each record...).
We already have .insert(), i'm introducing .updateMany()
I think our ORM layer should be simplified a lot but I'm not starting
the refacto yet
## Fixing ORM @Weiko @ijreilly FYI
- Fixing events emission in delete function
- Fixing events emission in insert function
- make sure everything is batched
## Events performance @Weiko @ijreilly FYI
Do not emit timelineActivity db events as this does not seem useful and
is quite heavy
## Messaging and Calendar performance @bosiraphael FYI
Rework many functions to make sure they are batched. This does not touch
the driver layer and I have heavily tested it. (multiple account, with
multiple channels, common thread, common messages, etc...)
## Workflow Trigger relation Fetch performance @martmull FYI
Improved performance by batching
Fixes https://github.com/twentyhq/core-team-issues/issues/1262
In this PR we add the update permission check layer by
- for the graphql api: extracting columns to update from the
expressionMap
- for rest api: .save() is used so we need to add the permission layer
to .save directly. We also take advantage of this PR to filter out
non-readable fields from save response (other save returns the whole
entity) - this was planned in
https://github.com/twentyhq/core-team-issues/issues/1216
The current solution does not work with rest api depth 2 queries, but
this seem to already not work on main (for timeout reasons though, so
different). I offer to create a ticket to fix it altogether later.
Context :
Large PR with 600+ test files. Enable connect and disconnect logic in
createMany (upsert true) / updateOne / updateMany resolvers
- Add disconnect logic
- Gather disconnect and connect logic -> called relation nested queries
- Move logic to query builder (insert and update one) with a preparation
step in .set/.values and an execution step in .execute
- Add integration tests
Test :
- Test API call on updateMany, updateOne, createMany (upsert:true) with
connect/disconnect
In this PR, behind a feature flag, we add a permission layer check based
on the read permission.
It is done by computing a map of an object's fields, where keys are the
column names and values the fieldMetadata id, making them comparable to
the restricted fields ids list stored in the permission cache.
For mutations (create, update, delete, destroy), we need to check the
read permission on the returned field, as they may differ from the
updated field. The write field permission will be tackled in a different
PR.
# Introduction
From the moment replaced the FieldMetadataInterface definition to:
```ts
import { FieldMetadataType } from 'twenty-shared/types';
import { FieldMetadataEntity } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity';
export type FieldMetadataInterface<
T extends FieldMetadataType = FieldMetadataType,
> = FieldMetadataEntity<T>;
```
After this PR merge will create a new one removing the type and
replacing it to `FieldMetadataEntity`.
Did not renamed it here to avoid conflicts on naming + type issues fixs
within the same PR
## Field metadata entity RELATION or MORPH
Relations fields cannot be null for those field metadata entity instance
anymore, but are never for the others see
`packages/twenty-server/src/engine/metadata-modules/field-metadata/types/field-metadata-entity-test.type.ts`
( introduced TypeScript tests )
## Concerns
- TS_VECTOR is the most at risk with the `generatedType` and
`asExpression` removal from interface
## What's next
- `FielMetadataInterface` removal and rename ( see introduction )
- Depcrecating `ObjectMetadataInterface`
- Refactor `FieldMetadataEntity` optional fiels to be nullable only
- TO DIG `never` occurences on settings, defaultValue etc
- Some interfaces will be replaced by the `FlatFieldMetadata` when
deprecating the current sync and comparators tools
## Context
Add an eventEmitter instance to twenty datasources so we can emit DB
events.
Add input and output formatting to twenty orm (formatData, formatResult)
Those 2 elements simplified existing logic when we interact with the
ORM, input will be formatted by the ORM so we can directly use
field-like structure instead of column-like. The output will be
formatted, for builder queries it will be in `result.generatedMaps`
where `result.raw` preserves the previous column-like structure.
Important change: We now have an authContext that we can pass when we
get a repository, this will be used for the different events emitted in
the ORM. We also removed the caching for repositories as it was not
scaling well and not necessary imho
Note: An upcoming PR should handle the onDelete: cascade behavior where
we send DESTROY events in cascade when there is an onDelete: CASCADE on
the FK.
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
Fixes
https://github.com/twentyhq/core-team-issues/issues/255?issue=twentyhq%7Ccore-team-issues%7C1214.
Until then, in the endpoints of our dynamic schema, we were querying all
columns and then formatting the result by removing the non-requested
fields (fields not mentioned in the graphql Query) from the result.
This is not compatible with field-level permissions that we are about to
introduce because users would see their request denied if they have
restricted rights on any of the fields of the objects they are querying,
even if they did not query it in the first place.
To prepare for this change, we are restricting the list of queried
columns to those made necessary by the graphql query.
I only made the changes in the dynamic schema for now. We will
potentially need to do updates to other part of the app that use
createQueryBuilder directly or not (for instance, when calling
repository methods such as .findOne()), but they mostly regard system
objects that are not subject to permissions or are executed by entities
that bypass permission such as jobs creating People and Companies from
their email sync.
No changes have been brought to existingRecords related logic in the
dynamic schema because @Weiko is currently working on it, so I may need
to adapt the new logic after he is done.
No feature flag have been added so far as this should not change
anything at the moment.
Fixes https://github.com/twentyhq/twenty/issues/12867
Issue:
when you have a variable `toto` which is: `Record<string, MyType>` and
you do toto['xxx'], this will be typed as `MyType` instead of `MyType |
undefined`
Solutions:
- activate `noUncheckedIndexedAccess` check in tsconfig, this is the
preferred solution but will take time to get there (this raises 600+
errors)
- use a Map: cf https://github.com/twentyhq/twenty/pull/13125/files
- set the type to Partial<Record<string, MyType>>. Drawback is that when
you do Object.values(toto), you'll get `Array<MyType | undefined>`.
Hence why we have to filter these behind
<img width="1512" alt="image"
src="https://github.com/user-attachments/assets/d0a0bfed-c441-4e53-84c2-2da98ccbcf50"
/>
Large PR, sorry for that. Don't hesitate to reach me to have full
context (env. 500lines for integration and unit tests)
- Add connect logic in Workspace Entity Manager
- Update QueryDeepPartialEntity type to enable dev to use connect
- Add integration test on createOne / createMany
- Add unit test to cover main utils
- Remove feature flag on connect
closes https://github.com/twentyhq/core-team-issues/issues/1148
closes https://github.com/twentyhq/core-team-issues/issues/1147
In this PR
- introduction of fieldPermission entity
- addition of upsertFieldPermission in role resolver
- computing of permissions taking fieldPermission into account. In order
to limit what is stored in Redis we only store fields restrictions. For
instance for objectMetadata with id XXX with a restriction on field with
id YYY we store:
`"XXX":{"canRead":true,"canUpdate":false,"canSoftDelete":false,"canDestroy":false,"restrictedFields":{"YYY":{"canRead":false,"canUpdate":null}}}`
---------
Co-authored-by: Charles Bochet <charlesBochet@users.noreply.github.com>
In this PR
1. Add missing override of insert() method on
WorkspaceSelectQueryBuilder to return our custom
WorkspaceInsertQueryBuilder with permission checks.
2. Replace override implementation of methods on WorkspaceEntityManager
that call createQueryBuilder at a nested internal layer of typeORM (i.e.
not directly in the initial implementation of EntityManager - unlike
findBy for instance -, but in calls done under the hood at a level which
would force us to override entire other classes to pass on our
permissionOptions. It is the case for methods which call typeORM's
EntityPersistExecutor for instance.), to validate permissions and then
allow the subsequent calls to be made without permission checks
3. adapt tests
---------
Co-authored-by: Charles Bochet <charles@twenty.com>
In the frame of https://github.com/twentyhq/core-team-issues/issues/924
- Rename dataSource -> workspaceDataSource when relevant to ease
understandability
- override workspaceDataSource.createQueryBuilder, because we don't want
developers to use it directly since it does not run permission checks at
this level. Indeed, we cannot do so because 1) datasources are shared
between roles so we would need to re-think its implementation to make
that possible, while for now we never call
workspaceDatasource.createQueryBuilder in our codebase 2)
workspaceEntityManager.createQueryBuilder, that we have overriden with
permission checks, then performs a call to
workspaceDataSource.createQueryBuilder so that would make two permission
checks.
Closes https://github.com/twentyhq/core-team-issues/issues/748
In the frame of the work on permissions we
- remove all raw queries possible to use repositories instead
- forbid usage workspaceDataSource.executeRawQueries()
- restrict usage of workspaceDataSource.query() to force developers to
pass on shouldBypassPermissionChecks to use it.
---------
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
## Context
- Introduced objectPermissions in currentUserWorkspace which uses role
permissions from cache so we can fetch granular permissions from the API
- Refactored cached role permissions to map permissions with object
metadata id instead of object metadata name singular to be more flexible
New Cache
<img width="574" alt="Screenshot 2025-05-27 at 11 59 06"
src="https://github.com/user-attachments/assets/1a090134-1b8a-4681-a630-29f1472178bd"
/>
GQL
<img width="977" alt="Screenshot 2025-05-27 at 11 58 53"
src="https://github.com/user-attachments/assets/3b9a82b0-6019-4a25-a6e2-a9e0fb4bb8a0"
/>
Next steps: Use the updated API in the FE to fetch granular permissions
and update useHasObjectReadOnlyPermission hook
- fix missing createBy injection in api createOne and createMany
endpoints
- add a command to fix null default value for createdBySource in
production entities
- tested on `1747159401197/` dump extract of production db without issue
Weiko