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

Introduce support for Universally Unique Identifiers #2749

Closed
wants to merge 2 commits into from
Closed

Introduce support for Universally Unique Identifiers #2749

wants to merge 2 commits into from

Conversation

x80486
Copy link

@x80486 x80486 commented Aug 5, 2023

A Universally Unique Identifier (UUID) is a 128-bit used for information in computer systems. The term Globally Unique Identifier (GUID) is also used, mostly in Microsoft systems.

Because Go (still) does not have built-in support for UUID types, module gofrs/uuid is imported. It's the most complete, correct (and starred) module nowadays.

This pull request introduces support for Universally Unique Identifiers (UUID) within gqlgen.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

Closes #2748

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Aug 5, 2023

Hi! So can you verify that with your PR someone can still override this type and use https://github.com/google/uuid ?

I would note that google/uuid is still more widely used than gofrs/uuid per this:
https://www.grank.io/pkg/github.com/google/uuid.html

I don't mind picking a default implementation, but I would like to provide a working example that successfully overrides with an alternative since there are a number of popular choices that people are already using.

This is how sqlc works: https://docs.sqlc.dev/en/stable/reference/config.html#type-overriding

@coveralls
Copy link

coveralls commented Aug 5, 2023

Coverage Status

coverage: 75.765% (-0.01%) from 75.777% when pulling d479268 on x80486:issue-2748 into 2d8673a on 99designs:master.

@x80486
Copy link
Author

x80486 commented Aug 5, 2023

Hi! So can you verify that with your PR someone can still override this type and use https://github.com/google/uuid ?

I will have to figure it out how to test that 😬

I would note that google/uuid is still more widely used than gofrs/uuid per this: https://www.grank.io/pkg/github.com/google/uuid.html

I don't mind picking a default implementation, but I would like to provide a working example that successfully overrides with an alternative since there are a number of popular choices that people are already using.

The Google implementation is the most popular indeed, but it looks like it's not actively maintained.

On the other hand, what are those failures in the pipeline? As far as I can tell it's complaining about gofrs/uuid not in go.sum, but it's there. I did try go generate ./... locally but it doesn't change anything.

@x80486
Copy link
Author

x80486 commented Aug 5, 2023

Nevermind. I was lurking in some other pull requests, and I saw the go generate ./... trick inside _examples.

@x80486
Copy link
Author

x80486 commented Aug 5, 2023

If the relevant changes are done in id.go, and there is another case clause for UUID types...it is possible to use UUID transparently just by declaring it in the Go models, right?

@StevenACoffman StevenACoffman mentioned this pull request Aug 7, 2023
@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Aug 7, 2023

If the relevant changes are done in id.go, and there is another case clause for UUID types...it is possible to use UUID transparently just by declaring it in the Go models, right?

I'm sorry, but I'm kind of confused by the wording of your question. Can you please rephrase that?

This PR is just like PR #2751 but here the choice is github.com/gofrs/uuid instead of there it is github.com/google/uuid

Many people will have already chosen one of those two (or something else) and so will have something like this in their gqlgen.yaml config:

models:
  UUID:
    model:
      - github.com/google/uuid

Please add to this PR a similar example that verifies that people can still override the default choice after your PR is applied.

@x80486
Copy link
Author

x80486 commented Aug 7, 2023

I didn't realize there was another pull request for the same work. I'm fine with either UUID type. I'll discard this one so you don't have to review the same work twice.

@x80486 x80486 closed this Aug 7, 2023
@StevenACoffman
Copy link
Collaborator

Actually, yours was better quality, and it passed the tests! 😞

@x80486 x80486 reopened this Aug 8, 2023
@x80486
Copy link
Author

x80486 commented Aug 8, 2023

I have another pull request to be able to use ID with UUIDs as well. That's why I was asking if those changes should be done under id.go. I see a switch clause that's able to handle multiple types. I'm also planning to have a working example as well for that one.

As it stands, the UUID type must be defined like:

type CustomType {
  id: ID!
  uuidID: UUID!
}

The end goal is to be able to use ID and if declared correctly, the models could use UUID as well. My understanding is that this is possible after updating id.go to understand UUID types, and having something like this in the configuration file:

models:
  ID:
    model:
      - github.com/99designs/gqlgen/graphql.ID
      - github.com/99designs/gqlgen/graphql.UUID # Probably this is not even necessary

return Null
}
return WriterFunc(func(w io.Writer) {
_, _ = io.WriteString(w, t.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

writeQuotedString(w, t.String()) ?
uuid type marsher is "00000000-0000-0000-0000-00 0000000000"

must Quoted

Copy link
Author

Choose a reason for hiding this comment

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

I don't fully understand why is that needed here. I think that might be the case for arbitrary string values that should be double-quoted because they could have escaped/control characters and non-printable ones, but the uuid.UUID will never have those.

StevenACoffman added a commit that referenced this pull request Sep 8, 2023
* add uuid type

* add uuid example

* add uuid scalar doc

* strconv.Quote

* Apply suggestions from code review

* fix

* Adjust documentation to match PR #2749

Signed-off-by: Steve Coffman <steve@khanacademy.org>

---------

Signed-off-by: Steve Coffman <steve@khanacademy.org>
Co-authored-by: Steve Coffman <StevenACoffman@users.noreply.github.com>
Co-authored-by: Steve Coffman <steve@khanacademy.org>
@StevenACoffman
Copy link
Collaborator

I really appreciate this contribution, as it had excellent documentation (and great tests!). I ended up applying your documentation and using #2751 as it used github.com/google/uuid which my company and most of the other maintainers use at work, despite the fact that gofrs/uuid is more complete and is actively maintained.

It took a long time to discuss (argue) about which PR to merge, and we were pretty conflicted, so I'm very sorry for the delay (and that we did not pick merging this PR as is).

I hope that doesn't dissuade you from future contributions, as this was excellent quality.

@x80486 x80486 deleted the issue-2748 branch September 8, 2023 14:57
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.

Built-in support for UUID type(s)
4 participants