Multitenancy #231

thelissimus-work · 2025-03-27T00:15:25Z

No description provided.

Copilot

Pull Request Overview

This PR introduces multitenancy support across multiple modules by adding tenant context propagation, repository, service, and domain logic.

Adds tenant context to the authentication middleware
Implements tenant service and repository for CRUD operations
Updates domain models and repository mappers to incorporate tenant IDs

Reviewed Changes

Copilot reviewed 13 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/middleware/auth.go	Adds tenant lookup in authentication flow
pkg/constants/middleware.go	Introduces a TenantKey constant
pkg/composables/tenant.go	Provides helper methods for using tenant data
modules/core/services/tenant_service.go	Implements tenant service methods
modules/core/module.go	Registers tenant and tab services; duplicate tab service registration observed
modules/core/infrastructure/persistence/user_repository.go	Adds tenant_id handling in user repository records
modules/core/infrastructure/persistence/tenant_repository.go	Implements tenant repository with query methods
modules/core/infrastructure/persistence/models/models.go	Adds tenant fields to various models
modules/core/infrastructure/persistence/core_mappers.go	Maps tenant_id for user entity conversions
modules/core/domain/entities/tenant/*	Defines tenant entity and repository interface
modules/core/domain/aggregates/user/user.go	Introduces tenantID in the user aggregate

Files not reviewed (7)

modules/bichat/infrastructure/persistence/schema/bichat-schema.sql: Language not supported
modules/core/infrastructure/persistence/schema/core-schema.sql: Language not supported
modules/crm/infrastructure/persistence/schema/crm-schema.sql: Language not supported
modules/finance/infrastructure/persistence/schema/finance-schema.sql: Language not supported
modules/hrm/infrastructure/persistence/schema/hrm-schema.sql: Language not supported
modules/logging/infrastructure/persistence/schema/logging-schema.sql: Language not supported
modules/warehouse/infrastructure/persistence/schema/warehouse-schema.sql: Language not supported

Comments suppressed due to low confidence (1)

modules/core/module.go:67

Duplicate registration of TabService is detected; consider removing the extra registration to avoid potential unexpected behavior.

services.NewTabService(persistence.NewTabRepository()),

pkg/middleware/auth.go

diyor28 · 2025-03-27T14:44:36Z

@thelissimus-work Use UUID for tenants table. We will be migrating to UUID pretty soon, so why not start using UUID for new tables already

Co-authored-by: Copilot <[email protected]>

cypress · 2025-03-31T18:39:26Z

Default Project Run #393

Run Properties: Passed #393 • 6aba8250fd: Merge branch 'staging' into sdk-213

Project	`Default Project`
Branch Review	`sdk-213`
Run status	`Passed #393`
Run duration	`00m 23s`
Commit	`6aba8250fd: Merge branch 'staging' into sdk-213`
Committer	`Bektosh Madaminov`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`3`
View all changes introduced in this branch ↗︎

Extract tenant id and set it for the user in repository update Issues: #102

Issues: #102

mirolimsgd · 2025-04-10T19:27:57Z

modules/core/infrastructure/persistence/upload_repository.go

@@ -126,8 +137,14 @@ func (g *GormUploadRepository) Count(ctx context.Context) (int64, error) {
 	if err != nil {
 		return 0, err
 	}
+
+	tenant, err := composables.UseTenant(ctx)


I would suggest to require tenantId as an explicit parameter in function signatures where it is mandatory, instead of implicitly relying on its presence within a context object (ctx).

Error Prevention: If tenantId is mandatory for the function's logic, requiring it in the signature prevents runtime errors that occur when it's missing from the ctx. This shifts error detection earlier in the development cycle.

Reduced Boilerplate: This approach eliminates the repetitive code currently needed in many methods and repositories to extract and validate the tenantId from the context.

Improved Clarity & Explicitness: Making tenantId an explicit parameter clearly communicates the function's dependency, making the code easier to understand and maintain, thereby reducing the potential for runtime errors.

Handling Multi-Tenant vs. All-Tenant Queries:
For scenarios where a method needs to retrieve records across all tenants (i.e., be tenant-independent), we could handle this by passing a designated value for the tenantId parameter (e.g., an empty string, null, or a specific constant). The method implementation would then check this parameter's value to decide whether or not to include the tenant filtering condition in its database query.

That is indeed correct. I pointed this out to Diyor as well, for some reason tenant id propagation is done like this throughout the whole repo.
I'm reworking this whole thing

@mirolimsgd Regarding this issue.
It's been quite some time since this PR is hanging and because it's massive, the longer it halts, the more problems like merge conflicts and it's getting hard maintaining this PR. So maybe I could rework tenant id propagation in a different PR. I'd create a separate issue for it.
It's just that it's taking some time to do this. Besides, speeding up this with AI turned out to be a bit of problem, as it turns out AI still struggles with big refactorings across many files.

@bektosh-iota This wasn't implemented by me, so implement it as you see fit.

I see the problem now. Tenant is already part of domain entities and db models, so passing it through context makes no sense

@mirolimsgd Regarding this issue. It's been quite some time since this PR is hanging and because it's massive, the longer it halts, the more problems like merge conflicts and it's getting hard maintaining this PR. So maybe I could rework tenant id propagation in a different PR. I'd create a separate issue for it. It's just that it's taking some time to do this. Besides, speeding up this with AI turned out to be a bit of problem, as it turns out AI still struggles with big refactorings across many files.

It is just suggestions. I am not blocking pr. The problem I see if it is merged it then it will be used and if not immediately fixed will create more work later so I suggest to change it now. It will reduce amount of code in this pr and later there is no need to spend more time.
Both options work for me.

@mirolimsgd Regarding this issue. It's been quite some time since this PR is hanging and because it's massive, the longer it halts, the more problems like merge conflicts and it's getting hard maintaining this PR. So maybe I could rework tenant id propagation in a different PR. I'd create a separate issue for it. It's just that it's taking some time to do this. Besides, speeding up this with AI turned out to be a bit of problem, as it turns out AI still struggles with big refactorings across many files.

It is just suggestions. I am not blocking pr. The problem I see if it is merged it then it will be used and if not immediately fixed will create more work later so I suggest to change it now. It will reduce amount of code in this pr and later there is no need to spend more time. Both options work for me.

I agree

diyor28 · 2025-04-14T11:38:04Z

modules/core/infrastructure/persistence/session_repository.go

 	sessionUpdateQuery = `
        UPDATE sessions
        SET expires_at = $1,
            ip = $2,
            user_agent = $3
-        WHERE token = $4`
-	sessionDeleteQuery = `DELETE FROM sessions WHERE token = $1`
+        WHERE token = $4 AND tenant_id = $5`


Why is this necessary? Token is already a primary key

diyor28 · 2025-04-14T11:38:15Z

modules/core/infrastructure/persistence/session_repository.go

-        WHERE token = $4`
-	sessionDeleteQuery = `DELETE FROM sessions WHERE token = $1`
+        WHERE token = $4 AND tenant_id = $5`
+	sessionDeleteQuery = `DELETE FROM sessions WHERE token = $1 AND tenant_id = $2`


Here as well

diyor28 · 2025-04-14T11:39:20Z

modules/core/infrastructure/persistence/session_repository.go

 	var count int64
-	if err := tx.QueryRow(ctx, sessionCountQuery).Scan(&count); err != nil {
+	if err := tx.QueryRow(ctx, sessionCountQuery+" WHERE tenant_id = $1", tenant.ID).Scan(&count); err != nil {


For constructing sql queries in the runtime use methods from the repo package. Reference other usages of this package to get an idea on how to use it

diyor28 · 2025-04-14T11:44:30Z

modules/core/domain/entities/tenant/tenant.go

+	"github.com/google/uuid"
+)
+
+type Tenant struct {


Is there a particular reason Tenant isn't an interface like other domain entities?

diyor28 · 2025-04-14T11:50:34Z

modules/core/infrastructure/persistence/setup_test.go

-		if err := tx.Commit(ctx); err != nil {
-			t.Fatal(err)
+		// Rollback instead of commit to ensure clean state
+		// This is safer as it ensures tests don't affect each other


They already don't affect each other since each test runs in a separate db. Committing changes is useful to be able to introspect the db state after a failed test

diyor28 · 2025-04-14T11:55:35Z

pkg/composables/tenant.go

+	ErrNoTenantFound = errors.New("no tenant found in context")
+)
+
+type Tenant struct {


What is this duplicate type for?

ali added 8 commits March 26, 2025 20:05

add: tenant id to schemas

5306bb2

update: user and user_repository to include tenant_id

d09e74d

update: core mappers

9a364bf

update: models to include tenant_id

a878234

add: tenant composable and middleware

ac23e2f

add: tenant service, repository and update auth to get tenant entity

118ea84

Merge branch 'staging' into sdk-213

be0064f

Merge branch 'staging' into sdk-213

231cee2

thelissimus-work changed the title ~~Sdk 213~~ Multitenancy Mar 27, 2025

diyor28 requested a review from Copilot March 27, 2025 14:36

Copilot AI reviewed Mar 27, 2025

View reviewed changes

pkg/middleware/auth.go Outdated Show resolved Hide resolved

ali and others added 18 commits March 28, 2025 13:40

add: tenant id to permission, role and tab repositories

bcce1b1

add: tenant id to authlog and passport repo

063ae66

add: tenant id to session, upload and unit repos

046d6bc

add: core repos tenant_id

5056e79

add: bichat module tenant_id

7346cd7

add: crm module tenant_id

1f1013f

add: finance module tenant_id

5c3b817

add: hrm module tenant_id

777fa26

add: warehouse module tenant_id

8513f0e

add: missing tenant_ids in Go code

e5f6f79

Update pkg/middleware/auth.go

e697173

Co-authored-by: Copilot <[email protected]>

fix: missing log import

499207e

add: core missing tenant_id in Go code

b7b99b2

fix: NewWithID call sites

4d25587

fix: seed

dcff0fc

format: warehouse-schema.sql

0423802

format: logging-schema

2c50151

format: hrm-schema

2ec103c

ali added 2 commits March 31, 2025 04:41

format: schemas

7f49594

fix: tests

171d741

ali added 4 commits April 1, 2025 01:42

fix: login error display

35b3d25

fix: login to work across tenants

66d0e6d

fix: auth tenant context

ca47337

fix: auth conflict

6a29584

bektosh-iota self-assigned this Apr 1, 2025

bektosh-iota added 11 commits April 2, 2025 13:20

fix(user update): fix e2e tests

a73ecbb

Extract tenant id and set it for the user in repository update Issues: #102

remove package-lock.json

460e76c

refactor(tenant): change id from uint to uuid

bbc5a7d

Issues: #102

Merge branch 'staging' into sdk-213

9327ff7

fix migration typestamp

6ed8920

fix migrate collect

04a235a

fix sql format for the migration

5ed5b6b

go format

495c828

Merge branch 'staging' into sdk-213

2ddac2d

run make migrate collect

6260d99

migration fix after merge conflict

6e1f790

mirolimsgd reviewed Apr 10, 2025

View reviewed changes

Merge branch 'staging' into sdk-213

6aba825

bektosh-iota marked this pull request as ready for review April 14, 2025 11:25

bektosh-iota requested review from diyor28 and mirolimsgd April 14, 2025 11:31

diyor28 reviewed Apr 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multitenancy #231

Multitenancy #231

thelissimus-work commented Mar 27, 2025

Copilot AI left a comment

diyor28 commented Mar 27, 2025

cypress bot commented Mar 31, 2025 •

edited

Loading

mirolimsgd Apr 10, 2025

bektosh-iota Apr 11, 2025

bektosh-iota Apr 14, 2025

diyor28 Apr 14, 2025

diyor28 Apr 14, 2025

mirolimsgd Apr 14, 2025

diyor28 Apr 15, 2025

diyor28 Apr 14, 2025

diyor28 Apr 14, 2025

diyor28 Apr 14, 2025

diyor28 Apr 14, 2025

diyor28 Apr 14, 2025 •

edited

Loading

diyor28 Apr 14, 2025

Multitenancy #231

Are you sure you want to change the base?

Multitenancy #231

Conversation

thelissimus-work commented Mar 27, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

diyor28 commented Mar 27, 2025

cypress bot commented Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diyor28 Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Mar 31, 2025 •

edited

Loading

diyor28 Apr 14, 2025 •

edited

Loading