[management] Refactor group to use store methods

[bcmmbaga]

Describe your changes

Remove calls to get and save whole account in group operations.

Issue ticket number and link

Checklist

• [ ] Is it a bug fix
• [ ] Is a typo/documentation fix
• [x] Is a feature enhancement
• [x] It is a refactor
• [ ] Created tests that fail without the change (if possible)
• [ ] Extended the README / documentation, if necessary

[ismail0234]

@bcmmbaga When you use ExecuteInTransaction, storeEngine always returns empty value.

[bcmmbaga]

@bcmmbaga When you use ExecuteInTransaction, storeEngine always returns empty value.

I didn’t quite understand this. Are you getting an empty value when calling transaction.(methodName), or is it happening in a different case?

[ismail0234]

@bcmmbaga When you use ExecuteInTransaction, storeEngine always returns empty value.

I didn’t quite understand this. Are you getting an empty value when calling transaction.(methodName), or is it happening in a different case?

For example, if you call “s.storeEngine” in this method, it returns an empty value. Because this method is called from a transaction.

https://github.com/netbirdio/netbird/blob/669904cd060186d0ea760057699aa3f4076ac13b/management/server/sql_store.go#L983-L993

[ismail0234]

This can cause many problems in the future. When you try to access this value in the methods called by the transaction, it will return empty.

[bcmmbaga]

This can cause many problems in the future. When you try to access this value in the methods called by the transaction, it will return empty.

You're right. However, the Store methods should be generic across all databases and shouldn't include unique implementations for each supported database. Therefore, we shouldn't reference the specific engine in the Store implementation. There's currently a TODO in GetGroupByName that will be removed in this PR

[ismail0234]

This can cause many problems in the future. When you try to access this value in the methods called by the transaction, it will return empty.

You're right. However, the Store methods should be generic across all databases and shouldn't include unique implementations for each supported database. Therefore, we shouldn't reference the specific engine in the Store implementation. There's currently a TODO in GetGroupByName that will be removed in this PR

This is needed for MySQL Integration. Because key is a reserved word for MySQL. You cannot use it in queries without enclosing it in double quotes. But when you use it with double quotes, an error occurs because Sqlite and Postgres queries do not support it. In this case, we should check for Mysql and Postgres and use the correct one.

https://doc.ispirer.com/sqlways/Output/SQLWays-1-205.html

The “queryCondition” value must take the correct value according to the current engine.

 func (s *SqlStore) GetSetupKeyBySecret(ctx context.Context, lockStrength LockingStrength, key string) (*SetupKey, error) { 

    var queryCondition := "key"

    if s.storeEngine == MysqlStoreEngine {
        queryCondition = "`key`";
    }

    var setupKey SetupKey 
    result := s.db.Clauses(clause.Locking{Strength: string(lockStrength)}). 
        First(&setupKey, queryCondition, key) 
    if result.Error != nil { 
        if errors.Is(result.Error, gorm.ErrRecordNotFound) { 
            return nil, status.Errorf(status.NotFound, "setup key not found") 
        } 
        return nil, status.NewSetupKeyNotFoundError(result.Error) 
    } 
    return &setupKey, nil 
}

[bcmmbaga]

@ismail0234 In that case, you can pass the store engine to this initialization, allowing the transaction to be aware of the current engine in use

https://github.com/netbirdio/netbird/blob/d58cf501276ba4712406849f8e8795fe11aced05/management/server/sql_store.go#L1117-L1121

func (s *SqlStore) withTx(tx *gorm.DB) Store {
    return &SqlStore{
        db:          tx,
        storeEngine: s.storeEngine,
    }
}

[ismail0234]

@ismail0234 In that case, you can pass the store engine to this initialization, allowing the transaction to be aware of the current engine in use

https://github.com/netbirdio/netbird/blob/d58cf501276ba4712406849f8e8795fe11aced05/management/server/sql_store.go#L1117-L1121

func (s *SqlStore) withTx(tx *gorm.DB) Store {
  return &SqlStore{
      db:          tx,
      storeEngine: s.storeEngine,
  }
}

Thanks. Will you add this?

I am currently querying the database name to overcome this problem.

func GetKeyQueryCondition(s *SqlStore) string {

    if s.storeEngine == MysqlStoreEngine {
        return mysqlKeyQueryCondition
    }

    if s.storeEngine == "" && s.db.Name() == "mysql" {
        return mysqlKeyQueryCondition
    }

    return keyQueryCondition
}

[bcmmbaga]

Thanks. Will you add this?

Yes. I’ll add that.

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud