Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add batch save/update for groups and users #2245

Merged
merged 9 commits into from
Jul 15, 2024
Merged

Conversation

bcmmbaga
Copy link
Contributor

@bcmmbaga bcmmbaga commented Jul 8, 2024

Describe your changes

Issue ticket number and link

Checklist

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

The method SaveAccount in user.go and group.go files was split into two separate methods. Now, user-specific data is handled by SaveUsers and group-specific data is handled by SaveGroups method. This provides a cleaner and more efficient way to save user and group data.
@bcmmbaga bcmmbaga marked this pull request as ready for review July 9, 2024 12:26
management/server/group.go Show resolved Hide resolved

// SaveGroups saves the given list of groups to the database.
// It updates existing groups if a conflict occurs.
func (s *SqlStore) SaveGroups(account *Account) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not memory efficient. If I understand well the reason of the "G" postfix it is just used by the Gorm. So after that, we always need to clean up because the garbage collector will not do it. Nevertheless, we should not mix the layers. The account structure holds the account info and the SQL-related things. We should create separate struct definition just for the SQL. It is more code but at the end, it will be safer.

Now I recommend to:

  • change the parameters of the function *SaveGroups(accountID string, groups map[string]nbgroup.Group)
  • create a new slice []nbgroup.Group
  • copy the relevant data to there
  • change the db.clauses to use the new slice

So after it the GroupsG will be empty again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the "&gorm.Session{FullSaveAssociations: true}" keyword help to you

@@ -311,6 +311,32 @@ func (s *SqlStore) SavePeerLocation(accountID string, peerWithLocation *nbpeer.P
return nil
}

// SaveUsers saves the given list of users to the database.
// It updates existing users if a conflict occurs.
func (s *SqlStore) SaveUsers(account *Account) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same like groups

func (s *SqlStore) SaveGroups(accountID string, groups map[string]*nbgroup.Group) error {
groupsToSave := make([]nbgroup.Group, 0, len(groups))
for id, group := range groups {
group.ID = id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to do this operation? In this line I think we fill well the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily needed, but it was to make sure the group ID is the same as the index

@@ -653,6 +683,8 @@ func (s *SqlStore) GetStoreEngine() StoreEngine {
return s.storeEngine
}

//func (s *SqlStore) SaveGroups()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comment

@bcmmbaga bcmmbaga requested a review from pappz July 11, 2024 08:13
Copy link

sonarcloud bot commented Jul 11, 2024

@bcmmbaga bcmmbaga merged commit 1537b0f into main Jul 15, 2024
22 checks passed
@bcmmbaga bcmmbaga deleted the feature/user-group-bulk-op branch July 15, 2024 14:04
hurricanehrndz added a commit to hurricanehrndz/netbird that referenced this pull request Jul 15, 2024
* upstream/main:
  Add batch save/update for groups and users (netbirdio#2245)
  Limit GUI process execution to one per UID (netbirdio#2267)
  Add logging option for wg device (netbirdio#2271)
  fix 2260: fallback serial to Board (netbirdio#2263)
  Support DNS routes on iOS (netbirdio#2254)
  Fix parameter limit issue for Postgres store (netbirdio#2261)
  Bump google.golang.org/grpc from 1.64.0 to 1.64.1 (netbirdio#2248)
  Get client ui locale on windows natively (netbirdio#2251)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants