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

Support pointers in un/marshal functions #1277

Merged
merged 7 commits into from
Aug 13, 2020
Merged

Support pointers in un/marshal functions #1277

merged 7 commits into from
Aug 13, 2020

Conversation

lwc
Copy link
Member

@lwc lwc commented Aug 12, 2020

Background

When updating protobuf to new v2 API we started to see a bunch of linting errors like:

web/graph/types/timestamp.go:13:25: MarshalTimestamp passes lock by value: 
google.golang.org/protobuf/types/known/timestamppb.Timestamp 
contains google.golang.org/protobuf/internal/impl.MessageState 
contains sync.Mutex

We use custom scalars for third party types to bind protobuf timestamps, upon updating the un/marshal functions to act on pointers, I ran into the issue outlined in #1259.

Currently gqlgen assumes that all concrete binding can happen against value types and pointers will delegate through to the underlying value type.

This PR

Updates binding code to bind to support binding to pointers directly, supporting binding between pointers and values in both directions.

Fixes: #1259

I have:

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

@coveralls
Copy link

coveralls commented Aug 12, 2020

Coverage Status

Coverage decreased (-0.2%) to 66.267% when pulling bef9c8b on direct-pointer-binding into 39a12e0 on master.

@lwc lwc requested a review from vektah August 12, 2020 06:56
@lwc
Copy link
Member Author

lwc commented Aug 13, 2020

This branch has been running 99designs production workloads without issue so I'm inclined to merge it.

@vektah
Copy link
Collaborator

vektah commented Aug 13, 2020

It also removes a special case for binding to named types that wrap strings. This special case existed for a long since removed approach to supporting graphql enums, and the interaction between Target and CastType was proving too hard to reason about.

I hate to break it to you but I think this is going to break the prisma guys, judging from the issue that came up last time I tried to remove it 😂

@lwc
Copy link
Member Author

lwc commented Aug 13, 2020

I hate to break it to you but I think this is going to break the prisma guys, judging from the issue that came up last time I tried to remove it 😂

Bummer 😅

Oh well, after cleaning up my codegen changes it was easy enough to re-instate (I think)

@@ -167,6 +167,7 @@ type TypeReference struct {
Definition *ast.Definition
GQL *ast.Type
GO types.Type
Target types.Type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we leave a comment that tries to explain the difference between Go and Type?

The target is the type of the physical field in the struct, and GO is the derived type when you combine it with the pointeryness required by the ast?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - it's not even that, it's the difference between how to bind to the type, and how to traverse to the type from a field on another type, where sometimes that is due to gql AST requirements.

I wonder if there's an abstraction that represents both pointer juggling and casting sorta like “x is satisfiable by y if i do a, b and c” 🤔

ref.GO = obj.Type()
ref.IsMarshaler = true
default:
} else if underlying := basicUnderlying(obj.Type()); def.IsLeafType() && underlying != nil && underlying.Kind() == types.String {
// Special case for named types wrapping strings. Used by default enum implementations.
Copy link
Collaborator

@vektah vektah Aug 13, 2020

Choose a reason for hiding this comment

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

👍 I wonder if this needs a link to the issue, so that we dont almost delete it again

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. TBH I'd still like to remove this case before 1.0.

Copy link
Collaborator

@vektah vektah left a comment

Choose a reason for hiding this comment

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

Suh-weet.

The generated code looks saner. Little more in the template, but it's pretty mechanical now.

@lwc lwc force-pushed the direct-pointer-binding branch from c56848e to 86c9ea4 Compare August 13, 2020 06:47
@lwc lwc force-pushed the direct-pointer-binding branch from 86c9ea4 to 997efd0 Compare August 13, 2020 07:05
@lwc lwc merged commit 04f6a69 into master Aug 13, 2020
@lwc lwc deleted the direct-pointer-binding branch August 13, 2020 07:33
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
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.

Cannot map custom scalars to third party pointer types
3 participants