Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Orgid uint #885

Merged
merged 2 commits into from
Apr 13, 2018
Merged

Orgid uint #885

merged 2 commits into from
Apr 13, 2018

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Apr 11, 2018

this helps us save some memory.
math.MaxUint32 == 4294967295

I don't anticipate we will ever have more orgs than that.

when loading from cassandra, if <0 -> assign OrgIdPublic

should result in memory savings in the index
when loading for cassandra, if <0 -> assign OrgIdPublic
@Dieterbe Dieterbe requested review from replay and woodsaj April 11, 2018 12:27
@@ -40,10 +40,10 @@ func getOrg(req *http.Request, multiTenant bool) (int, error) {
return 0, nil
}
org, err := strconv.Atoi(orgStr)
if err != nil {
if err != nil || org < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

now the public org id is configurable, right? so if somebody sets it to a high value, would 0 as an actual non-public org id be invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for authentication. when authenticating as an org-id, the org id must be >= 1 otherwise it is invalid.
this was true when public org id was -1, and remains true now that public org id can be any value >=1
(you would typically not specify the public org id for authentication, but rather a concrete, real org id)

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

LGTM with a comment

@Dieterbe Dieterbe merged commit 52d5e51 into master Apr 13, 2018
@Dieterbe Dieterbe deleted the orgid-uint branch April 20, 2018 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants