Struct `Group` is not goroutine safe

[ghost]

Describe the bug?

Hi, trying to pass Group to several goroutines, which some of them try to Marshal it, results in fatal error: concurrent map writes. It seems like the cause is this, under GroupProfile.MarshalJSON():

if a.Name != "" {
        a.GroupProfileMap["name"] = a.Name
}

Thanks!

What is expected to happen?

Safely marshal a struct in parallel

What is the actual behavior?

Transient fatal error: concurrent map writes

Reproduction Steps?

Just marshal in parallel until it happens

Additional Information?

No response

Golang Version

go version go1.18.1 darwin/amd64

SDK Version

v2.12.1

OS version

No response

[monde]

Apologies @boazshalom , I missed this issue. I will look into it.

[ghost]

All good @monde, thank you!

[github-actions[bot]]

This issue has been marked stale because there has been no activity within the last 14 days. To keep this issue active, remove the stale label.

[ghost]

Hi @monde, any update please?

[monde]

I'll try to carve out some time for this @boazshalom , thanks for the ping.

[monde]

@boazshalom I started looking at this and working on a PR. Is your code writing to GroupProfileMap as well? I'm not seeing how the existing implementation in MarshalJSON() would be causing this unless you were making many concurrent API calls with the same group. Seems like you wouldn't want to be calling the API like that, but I'd have to see your implementation to improve my impression. However, read on for history and an explanation of my PR:

We added the map named GroupProfileMap to GroupProfile per this feedback from @ymylei in #268 . GroupProfileMap is a true golang map, and as a developer/user of the okta-sdk-golang you'd have to write your own concurrency protection if you were writing to GroupProfileMap directly in your code.

If I were to refactor GroupProfileMap into a sync.Map it would look like the test example in PR #318 (e.g. profile.GroupProfileMap.Store(key, value) instead of profile.GroupProfileMap[key] = value. But then everyone would have a breaking change on the next minor release of the SDK.

https://github.com/okta/okta-sdk-golang/blob/718268404f7daf4b31a2e317e8332eed04f8e6a7/tests/integration/group_profile_test.go

All that said, perhaps I should just put a mutex on a.GroupProfileMap["name"] = a.Name and a.GroupProfileMap["description"] = a.Description in MarshalJSON() and then that would truly put the onus on anyone writing to GroupProfileMap in their implementation to also make use of a mutex.

Would like to get your feedback @boazshalom , hopefully @ymylei can give feedback also.

[monde]

@boazshalom @ymylei I think my #268 PR is over kill. I think I will put up a new PR with mutex around https://github.com/okta/okta-sdk-golang/blob/master/okta/groupProfile.go#L60-L65 and also leave a comment about developers making use of GroupProfiles's GroupProfilesMap needing to also put a mutex around their implementation. cc: @duytiennguyen-okta

[ymylei]

@monde Based on my reading of https://pkg.go.dev/sync#Map this does seem overkill for this data set. Particularly, I don't see how this data class in particular would need to be made go routine safe by itself, versus this is something which needs larger attention in the library (such as altering this for the UserProfile object as well).

@boazshalom I'd be curious if you'd be able to share the use case you're doing which causes you to send the group object into a set of go routines. While I do personally think this type of multi-threaded use behaviour is going to need to be developer managed, I am curious if there's something about the behaviour when we configure the map to merge into the rest of the GroupProfile object which is problematic.