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

Update 99designs/[email protected] and vektah/gqlparser/[email protected] #609

Merged
merged 14 commits into from
Jan 2, 2025

Conversation

StevenACoffman
Copy link
Contributor

@StevenACoffman StevenACoffman commented Dec 30, 2024

Update gqlgen and gqlparser

This updates gqlgen and gqlparser to the latest versions github.com/99designs/[email protected] and github.com/vektah/gqlparser/[email protected]


In https://github.com/ent/contrib/pull/607/files github.com/99designs/gqlgen was moved from v0.17.48 to v0.17.51.

I see that in that PR it has a comment that says that gqlgen version v0.17.52 has an issue with ent field collection. It blames commit: 99designs/gqlgen@f81239e

The blamed gqlgen commit was merged 99designs/gqlgen#3287
This was to fix 99designs/gqlgen#3285 by reverting 99designs/gqlgen#3203

Currently gqlgen v0.17.61 is the latest. It is not clear to me if there remains any problem with gqlgen and ent field collection.
Please point me to an open issue in gqlgen (or PR) describing (or fixing) the ent field collection problem.

Signed-off-by: Steve Coffman <[email protected]>
@StevenACoffman
Copy link
Contributor Author

StevenACoffman commented Dec 31, 2024

Hmmm, I'm not sure if my local environment is entirely what is expected. I appear to be finding the required local setup by process of elimination. Sorry for all the git commit amend and force pushing!

I'll try to instead to do this step by step and record what I'm doing in case it is helpful to some later person (e.g. future me). So far:

export GOBIN=~/go/bin
go install google.golang.org/protobuf/cmd/[email protected]
go install google.golang.org/grpc/cmd/[email protected]
cd entproto/cmd/protoc-gen-entgrpc
go install
cd ../../..
cd entproto/cmd/protoc-gen-ent
go install
cd ../../..
go generate ./...

This still results in a difference because protoc v3.19.4 is not the same as protoc v3.4.0

So I downloaded and decompressed protoc v3.19.4, prefixed my PATH environment variable with that bin directory, and tried again at this point.

this still results in one local test failing:

--- FAIL: TestPrintAddImport (0.83s)
    printer_test.go:103:
        	Error Trace:	/Users/steve/Documents/git/ent-contrib/schemast/printer_test.go:103
        	Error:      	Not equal:
        	            	expected: 0
        	            	actual  : 1
        	Test:       	TestPrintAddImport

@StevenACoffman
Copy link
Contributor Author

Ok, I am getting go generate failures when I run nektos/act to simulate the remote GitHub action failures:

act -j generate

This provides this minor output tweak:

| diff --git a/entgql/internal/todogotype/generated.go b/entgql/internal/todogotype/generated.go
| index dd27c93..d9738af 100644
| --- a/entgql/internal/todogotype/generated.go
| +++ b/entgql/internal/todogotype/generated.go
| @@ -20630,7 +20630,7 @@ func (ec *executionContext) marshalOInt2ᚖint(ctx context.Context, sel ast.Sele
|  	return res
|  }
|
| -func (ec *executionContext) unmarshalOMap2map(ctx context.Context, v any) (map[string]any, error) {
| +func (ec *executionContext) unmarshalOMap2map(ctx context.Context, v any) (map[string]interface{}, error) {
|  	if v == nil {
|  		return nil, nil
|  	}
| @@ -20638,7 +20638,7 @@ func (ec *executionContext) unmarshalOMap2map(ctx context.Context, v any) (map[s
|  	return res, graphql.ErrorOnPath(ctx, err)
|  }
|
| -func (ec *executionContext) marshalOMap2map(ctx context.Context, sel ast.SelectionSet, v map[string]any) graphql.Marshaler {
| +func (ec *executionContext) marshalOMap2map(ctx context.Context, sel ast.SelectionSet, v map[string]interface{}) graphql.Marshaler {
|  	if v == nil {
|  		return graphql.Null
|  	}
[Continuous Integration/generate]   ❌  Failure - Main Check generated files
[Continuous Integration/generate] exitcode '1': failure
[Continuous Integration/generate]   🐳  docker exec cmd=[/opt/acttoolcache/node/18.20.5/arm64/bin/node /var/run/act/workflow/hashfiles/index.js] user= workdir=
[Continuous Integration/generate] 🏁  Job failed

Signed-off-by: Steve Coffman <[email protected]>
@StevenACoffman StevenACoffman changed the title Update 99designs/[email protected] and vektah/gqlparser/[email protected] Update 99designs/[email protected] and vektah/gqlparser/[email protected] Dec 31, 2024
@StevenACoffman
Copy link
Contributor Author

Ok, moved to gqlgen v0.17.62 to make any/interface consistent

Signed-off-by: Steve Coffman <[email protected]>
Signed-off-by: Steve Coffman <[email protected]>
Signed-off-by: Steve Coffman <[email protected]>
@StevenACoffman
Copy link
Contributor Author

Running go test ./... causes the go.mod to add github.com/google/addlicense, but go mod tidy (which is in the go generate ./... ci job) removes it from a direct dependency (and moves it to an indirect). This causes a race condition if you locally run the ci github actions in the wrong order using nektos/act. Previously, I committed the go.mod and was puzzled over why the CI checks kept flagging changing go.mod / go.sum. To work around that, I'm adding a tools.go file (which will be useless in Go 1.24).

The other discrepancy is that my locally running go generate ./... seems to do something different for me using Go 1.23.4 on darwin/arm64 vs. when I run act -j generate --container-architecture linux/amd64. The remaining difference is between any and interface{} which I find surprising.

diff --git a/entgql/internal/todogotype/generated.go b/entgql/internal/todogotype/generated.go
index 76712c9..0e24fdf 100644
--- a/entgql/internal/todogotype/generated.go
+++ b/entgql/internal/todogotype/generated.go
@@ -20258,7 +20258,7 @@ func (ec *executionContext) marshalOInt2ᚖint(ctx context.Context, sel ast.Sele
 	return res
 }
 
-func (ec *executionContext) unmarshalOMap2map(ctx context.Context, v any) (map[string]any, error) {
+func (ec *executionContext) unmarshalOMap2map(ctx context.Context, v any) (map[string]interface{}, error) {
 	if v == nil {
 		return nil, nil
 	}
@@ -[20](https://github.com/ent/contrib/actions/runs/12564902369/job/35044421968?pr=609#step:11:21)266,7 +20266,7 @@ func (ec *executionContext) unmarshalOMap2map(ctx context.Context, v any) (map[s
 	return res, graphql.ErrorOnPath(ctx, err)
 }
 
-func (ec *executionContext) marshalOMap2map(ctx context.Context, sel ast.SelectionSet, v map[string]any) graphql.Marshaler {
+func (ec *executionContext) marshalOMap2map(ctx context.Context, sel ast.SelectionSet, v map[string]interface{}) graphql.Marshaler {
 	if v == nil {
 		return graphql.Null
 	}

@StevenACoffman
Copy link
Contributor Author

StevenACoffman commented Jan 2, 2025

So there was a slight discrepancy in the go.sum between my local go mod tidy and the nektos/act -j generate one, so I ran go clean --modcache && go mod tidy and that seemed to clear it up.

Now my nektos -j generate passes locally, as does nektos -j test so hopefully tomorrow when someone approves this it will just work in GitHub action runner as well.

@a8m a8m merged commit 6ecb3d5 into ent:master Jan 2, 2025
4 checks passed
@a8m
Copy link
Member

a8m commented Jan 2, 2025

Thanks for your contribution, @StevenACoffman 🙏🏻

@StevenACoffman StevenACoffman deleted the update_gqlgen_v0_17_61 branch January 2, 2025 14:36
@michaelcaulley
Copy link
Contributor

@StevenACoffman, adding an additional thanks for helping with this. I really appreciate all your work on gqlgen!

@@ -9,6 +9,7 @@ schema:

resolver:
layout: follow-schema
preserve_resolver: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we want this to be true or false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I picked true to be safe, because it depends. If there is customized code in those resolvers, then we want to preserve them (true), so we do not want to overwrite them. If there is nothing customized, then I guess it's harmless to preserve them, but we could set it to false.

I took this to be more of an example, as you people aren't actively changing these (or the schema) on a regular basis.

@StevenACoffman
Copy link
Contributor Author

@michaelcaulley Hey, so we should probably bump gqlgen again after this, or else swap the models to use any instead of interface{} or there is a potential for inconsistent results prior to v0.17.63

@michaelcaulley
Copy link
Contributor

michaelcaulley commented Jan 9, 2025

If it's set to true, new ent schemas will not be added when running go generate .

If helpful, I can create a PR to update preserve_resolver to false and bump gqlgen.

@a8m or @giautm , can you confirm my thinking for preserve_resolver: ?

Edit: Here's the PR #610

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.

Dataloaders broken since v0.17.50 and merge of #3203
3 participants