-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Return null
instead of zero value uuid
#2794
Conversation
Thanks! I appreciate this. Looking forward to more good work from you in the future! |
no ... I think this is bug.... uuid is value type , like int. so this is bug! |
@it512 Yeah, but I think nobody checking for zero value uuid. It's the same as |
time.Time is struct , not a point, can not set to null! *time.Time can set to nil ! time.Time is not *time.Time |
For me, it still doesn't make sense to return zero value uuid. And you are right From what I see returning a zero value uuid is a bug unless you expected that and mostly no one expected the value |
There's always a problem translating between Go's "useful zero values" concept and GraphQL consumed by other languages.
You cannot assign a
You also cannot assign a
When a JavaScript client makes a GraphQL call using the GraphQL Basic scalar types of String, Int, Float, Boolean, and ID, we expect the response to be useful values. I think that an int value of This PR made it impossible for gqlgen to respond with the valid, but unlikely to be useful, UUID value of Can you explain how or where @a8m @giautm @x80486 I would appreciate your opinion on whether this PR should be reverted or not. |
This is correct. Null values in Bottom line, in |
I think we can assume that |
According to RFC 4122 Nil UUID is a valid UUID https://datatracker.ietf.org/doc/html/rfc4122#section-4.1.7 It can usually be used as a placeholder in many scenarios, such as default values of fields in databases, etc. If Nil uuid == null, the Nil uuid field will return nil. . . this will be a disaster If the UUID can be null, use a pointer type. In graphql, you can use UUID( no !) -------------- Chinese version 根据 RFC 4122 Nil UUID 本就是一种有效的UUID https://datatracker.ietf.org/doc/html/rfc4122#section-4.1.7 通常在很多场景下都可以作为占位符使用,例如数据库中字段默认值等 如果 Nil uuid == null, Nil uuid 字段将返回 nil 。。。 这将是一个灾难 如果UUID 可以为空,请使用指针类型。 在graphql 中,你可以使用 UUID,没有! |
code in goole.uuid https://github.com/google/uuid/blob/v1.3.1/sql.go#L15 I added some comments // Scan implements sql.Scanner so UUIDs can be read from databases transparently.
// Currently, database types that map to string and []byte are supported. Please
// consult database-specific driver documentation for matching types.
func (uuid *UUID) Scan(src interface{}) error {
switch src := src.(type) {
case nil: // case 1 if field is null such as varchar(36) null ...
return nil // *UUID = nil (golang nil)
case string:
// if an empty UUID comes from a table, we return a null UUID
if src == "" {
return nil // case 2 empty string if field type is string and value is ""(empty string). return uuid is nil (golang nil)
}
// see Parse for required string format
u, err := Parse(src) // case 3 field has value if src is Nil uuid (128 bit 0) , uuid = Nil UUID, ///// ** #2794 return golang nil BUG!!! **
if err != nil {
return fmt.Errorf("Scan: %v", err)
}
*uuid = u
case []byte:
// if an empty UUID comes from a table, we return a null UUID
if len(src) == 0 {
return nil
}
// assumes a simple slice of bytes if 16 bytes
// otherwise attempts to parse
if len(src) != 16 {
return uuid.Scan(string(src))
}
copy((*uuid)[:], src)
default:
return fmt.Errorf("Scan: unable to scan type %T into UUID", src)
}
return nil
} |
I think returning null first is the best way And for me returning a zero value uuid without a clue will be a disaster more than |
If I want to return Nil UUID how can I do it? no way in #2794 !!! In fact, any language distinguishes between zero values and null values. but golang has made clear regulations on this such as java, int and Integer in database if using int , null int field was return 0 (int default value), return null if using Integer UUID in gqlgen is usedef type,Implemented according to RFC4122, base on google.uuid I think returning null according to usage, UUID! return zero value, UUID (no !) is *UUID default return nil (golang nil) |
@StevenACoffman I'll leave this to you because it can go both ways. Because the implementation of |
The behavior of json.Unmarshaler depends on the type. If it is a pointer type, null will be used. In other cases, zero value will be used. |
Yes, that is the default. But for the |
For valid uuid NullUUID returns the correct UUID (valid is true), Nil (zero uuid) is valid, so the return is still Nil NullUUID should only be used in ambiguous scenarios, or as a simple tool graphql has a definition for nullability, use (!) If available, please use UUID (no!) |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/99designs/gqlgen](https://togithub.com/99designs/gqlgen) | require | patch | `v0.17.36` -> `v0.17.38` | --- ### Release Notes <details> <summary>99designs/gqlgen (github.com/99designs/gqlgen)</summary> ### [`v0.17.38`](https://togithub.com/99designs/gqlgen/releases/tag/v0.17.38) [Compare Source](https://togithub.com/99designs/gqlgen/compare/v0.17.37...v0.17.38) #### What's Changed - Ability to use forceGenerate and extraFields together by [@​atzedus](https://togithub.com/atzedus) in [https://github.com/99designs/gqlgen/pull/2788](https://togithub.com/99designs/gqlgen/pull/2788) - Add new changelog by [@​StevenACoffman](https://togithub.com/StevenACoffman) in [https://github.com/99designs/gqlgen/pull/2787](https://togithub.com/99designs/gqlgen/pull/2787) - Fix rand int docs link in Getting Started by [@​stevenschobert](https://togithub.com/stevenschobert) in [https://github.com/99designs/gqlgen/pull/2789](https://togithub.com/99designs/gqlgen/pull/2789) - Make it possible to pass UI headers to GraphiQL by [@​marcusthelin](https://togithub.com/marcusthelin) in [https://github.com/99designs/gqlgen/pull/2793](https://togithub.com/99designs/gqlgen/pull/2793) - Return `null` instead of zero value uuid by [@​0x221A](https://togithub.com/0x221A) in [https://github.com/99designs/gqlgen/pull/2794](https://togithub.com/99designs/gqlgen/pull/2794) - Update gqlparser to 2.5.10 by [@​StevenACoffman](https://togithub.com/StevenACoffman) in [https://github.com/99designs/gqlgen/pull/2798](https://togithub.com/99designs/gqlgen/pull/2798) #### New Contributors - [@​stevenschobert](https://togithub.com/stevenschobert) made their first contribution in [https://github.com/99designs/gqlgen/pull/2789](https://togithub.com/99designs/gqlgen/pull/2789) - [@​marcusthelin](https://togithub.com/marcusthelin) made their first contribution in [https://github.com/99designs/gqlgen/pull/2793](https://togithub.com/99designs/gqlgen/pull/2793) - [@​0x221A](https://togithub.com/0x221A) made their first contribution in [https://github.com/99designs/gqlgen/pull/2794](https://togithub.com/99designs/gqlgen/pull/2794) **Full Changelog**: 99designs/gqlgen@v0.17.37...v0.17.38 ### [`v0.17.37`](https://togithub.com/99designs/gqlgen/blob/HEAD/CHANGELOG.md#v01737---2023-09-08) [Compare Source](https://togithub.com/99designs/gqlgen/compare/v0.17.36...v0.17.37) - <a href="https://togithub.com/99designs/gqlgen/commit/ccae370e96cbca6ce5deaabf28a6d57e3b181b3b"><tt>[`ccae370`](https://togithub.com/99designs/gqlgen/commit/ccae370e)</tt></a> release v0.17.37 - <a href="https://togithub.com/99designs/gqlgen/commit/6505f8be0d99593376d6c9a0dea00af5e3c018ea"><tt>[`6505f8b`](https://togithub.com/99designs/gqlgen/commit/6505f8be)</tt></a> Update gqlparser (<a href="https://togithub.com/99designs/gqlgen/pull/2785">[#​2785](https://togithub.com/99designs/gqlgen/issues/2785)</a>) <dl><dd><details><summary><a href="https://togithub.com/99designs/gqlgen/commit/153ec470d993a39c1656fc45432740f1d3dd10ea"><tt>153ec470</tt></a> add uuid type (<a href="https://togithub.com/99designs/gqlgen/pull/2751">#​2751</a>) (closes <a href="https://togithub.com/99designs/gqlgen/issues/2749"> #​2749</a>)</summary> - add uuid type - add uuid example - add uuid scalar doc - strconv.Quote - Apply suggestions from code review - fix *** </details></dd></dl> <dl><dd><details><summary><a href="https://togithub.com/99designs/gqlgen/commit/fa4711801c59d27db887a50e1f8393006268194e"><tt>fa471180</tt></a> ForceGenerate parameter to [@​goModel](https://togithub.com/goModel) added. (<a href="https://togithub.com/99designs/gqlgen/pull/2780">#​2780</a>)</summary> - forceGenerate to docs added *** </details></dd></dl> <dl><dd><details><summary><a href="https://togithub.com/99designs/gqlgen/commit/11bb9b1890fd9e5909f06feba7b2985bf09785c9"><tt>11bb9b18</tt></a> codegen: add support for `go_build_tags` option in gqlgen.yaml (<a href="https://togithub.com/99designs/gqlgen/pull/2784">#​2784</a>)</summary> - codegen: support go_build_tags option in gqlgen.yaml - chore: added test - docs/content: update config example - chore: more comment </details></dd></dl> <dl><dd><details><summary><a href="https://togithub.com/99designs/gqlgen/commit/bee47dcf1f3edb95e7a77e612ea27d85a417b12c"><tt>bee47dcf</tt></a> fix flaky test TestSubscriptions (<a href="https://togithub.com/99designs/gqlgen/pull/2779">#​2779</a>)</summary> - fix flaky test TestSubscriptions - update other copy of the test </details></dd></dl> - <a href="https://togithub.com/99designs/gqlgen/commit/a41f4daad66cb0c102bfa09d7bff5a057d378197"><tt>[`a41f4da`](https://togithub.com/99designs/gqlgen/commit/a41f4daa)</tt></a> docs: short-lived loader (<a href="https://togithub.com/99designs/gqlgen/pull/2778">[#​2778](https://togithub.com/99designs/gqlgen/issues/2778)</a>) - <a href="https://togithub.com/99designs/gqlgen/commit/cc4e0ba28375e0ec39c586485fa138c28e5bfcba"><tt>[`cc4e0ba`](https://togithub.com/99designs/gqlgen/commit/cc4e0ba2)</tt></a> ensure HasOperationContext checks for nil (<a href="https://togithub.com/99designs/gqlgen/pull/2776">[#​2776](https://togithub.com/99designs/gqlgen/issues/2776)</a>) <dl><dd><details><summary><a href="https://togithub.com/99designs/gqlgen/commit/a1ca220477092398efc8e587dd653f5cb967d0e9"><tt>a1ca2204</tt></a> fix typo in TESTING.md server path (<a href="https://togithub.com/99designs/gqlgen/pull/2774">#​2774</a>)</summary> following TESTING.md instructions, I got an error: "stat ./server/server.go: no such file or directory" server.go path is: integration/server/cmd/integration/server.go </details></dd></dl> <dl><dd><details><summary><a href="https://togithub.com/99designs/gqlgen/commit/1cde8c3fab65847a6e8d7f423215b786b47c19df"><tt>1cde8c3f</tt></a> return internal types in schema introspection (<a href="https://togithub.com/99designs/gqlgen/pull/2773">#​2773</a>)</summary> according to graphql spec: types: return the set of all named types contained within this schema. Any named type which can be found through a field of any introspection type must be included in this set. source: https://github.com/graphql/graphql-spec/blob/main/spec/Section%204%20--%20Introspection.md#the-\__schema-type some clients libs (like HotChocolate for C#) depends on this behavior. </details></dd></dl> - <a href="https://togithub.com/99designs/gqlgen/commit/065aea3efa27f0662eb2f62ef2dd131e153009ba"><tt>[`065aea3`](https://togithub.com/99designs/gqlgen/commit/065aea3e)</tt></a> Fix gqlgen truncates tag value with colon (<a href="https://togithub.com/99designs/gqlgen/pull/2759">[#​2759](https://togithub.com/99designs/gqlgen/issues/2759)</a>) - <a href="https://togithub.com/99designs/gqlgen/commit/d6270e4f4fd951c71ee2a2b997d7098b04e07976"><tt>[`d6270e4`](https://togithub.com/99designs/gqlgen/commit/d6270e4f)</tt></a> Update subsciptions documentation to correctly close channel (<a href="https://togithub.com/99designs/gqlgen/pull/2753">[#​2753](https://togithub.com/99designs/gqlgen/issues/2753)</a>) - <a href="https://togithub.com/99designs/gqlgen/commit/2d8673a691deffe6ddd1f4d5f013a52dc91aef91"><tt>[`2d8673a`](https://togithub.com/99designs/gqlgen/commit/2d8673a6)</tt></a> Add Model references to Interface (<a href="https://togithub.com/99designs/gqlgen/pull/2738">[#​2738](https://togithub.com/99designs/gqlgen/issues/2738)</a>) - <a href="https://togithub.com/99designs/gqlgen/commit/790d7a7571865b8b8324557e1a565f40c23217c8"><tt>[`790d7a7`](https://togithub.com/99designs/gqlgen/commit/790d7a75)</tt></a> Allow GraphiQL headers to be set when creating the playground handler (<a href="https://togithub.com/99designs/gqlgen/pull/2740">[#​2740](https://togithub.com/99designs/gqlgen/issues/2740)</a>) (closes <a href="https://togithub.com/99designs/gqlgen/issues/2739"> [#​2739](https://togithub.com/99designs/gqlgen/issues/2739)</a>) - <a href="https://togithub.com/99designs/gqlgen/commit/0eb95dc4315fc5f74431df03e008283cf9ec0c35"><tt>[`0eb95dc`](https://togithub.com/99designs/gqlgen/commit/0eb95dc4)</tt></a> v0.17.36 postrelease bump <!-- end of Commits --> <!-- end of Else --> <!-- end of If NoteGroups --> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/infratographer/x). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44My4wIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Describe your PR and link to any relevant issues.
Return
null
instead of the zero value uuid. I think this will be better and easier for the end user for null checking.I have: