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 support for snowflake based int64 uuids #824

Merged
merged 2 commits into from
Jan 21, 2022
Merged

add support for snowflake based int64 uuids #824

merged 2 commits into from
Jan 21, 2022

Conversation

ssoroka
Copy link
Contributor

@ssoroka ssoroka commented Jan 20, 2022

  • Fixes all serialization problems with using uuids in request structs
  • serializes to db as int64
  • existing dbs with text ids will either have to migrate or drop/recreate.
  • Wrote appropriate unit tests
  • Considered security implications of the change
  • Updated associated docs where necessary
  • Updated associated configuration where necessary
  • Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
  • Nothing sensitive logged

@@ -283,3 +286,5 @@ require (
sigs.k8s.io/structured-merge-diff/v4 v4.1.2 // indirect
sigs.k8s.io/yaml v1.2.0 // indirect
)

replace github.com/gin-gonic/gin => github.com/infrahq/gin v1.7.2-0.20220120203023-0eaa562f3a8a
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 how we can work off the fork without changing the code everywhere.

@@ -0,0 +1,40 @@
package uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

package id ?

Copy link
Contributor

@jmorganca jmorganca left a comment

Choose a reason for hiding this comment

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

LGTM - consider calling this ID or UID to avoid users expecting the UUID format


// User struct for User
type User struct {
ID UUID `json:"id"`
Email string `json:"email" validate:"email,required"`
ID uuid.UUID `json:"id"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we change this to ID in general rather than UUID? UUID implies it matches the UUID spec to me, which I don't think snowflake IDs do.

@@ -275,7 +275,7 @@ func (a *API) ListGrants(c *gin.Context, r *api.ListGrantsRequest) ([]api.Grant,
}

func (a *API) GetGrant(c *gin.Context, r *api.Resource) (*api.Grant, error) {
grant, err := access.GetGrant(c, r.ID.ToUUID())
grant, err := access.GetGrant(c, r.ID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are much nicer to read now 👏

@ssoroka ssoroka merged commit 565804a into main Jan 21, 2022
@ssoroka ssoroka deleted the ids branch January 21, 2022 15:25
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.

3 participants