[resolvergen] make rewrite default for single-file mode #3243
Conversation
hailyngx
force-pushed
the
enable_rewrite_for_single_file
branch
from
1f993f2 to
d7ffe0c
Compare
|
Refactor unit tests -- Sorry I got mixed up some file changes in my last commit that failed 2 tests. Should passed all unit tests now with this new commit! |
| filename: testdata/singlefile_enable_rewrite/out/generated.go | ||
| resolver: | ||
| filename: testdata/singlefile_enable_rewrite/out/resolver.go | ||
| type: CustomResolverType |
There was a problem hiding this comment.
Does this work without this option?
There was a problem hiding this comment.
@clayne11 This is the unit test for rewrite features with single-file, which follows exactly the available single-file feature. As I wrote a new function, it's good to have coverage.
On another note, without rewrite, yes, it does work in our system with Bazel. However, it does not support any functional overwriting. The developer will generate the stubs, implement however they like after the stubs. If they happened to make large schema changes and want the new corresponding resolver stubs, they have to delete resolver.go in the directory. The main difference is that single-file only focus on generating the stubs, not keeping the implementation.
Rewrite aims to support large schema changes while retaining the implementation of single-file mode.
|
|
||
| type Resolver struct{} | ||
|
|
||
| type QueryResolver interface { |
There was a problem hiding this comment.
Why are these interfaces being generated here? Aren't these normally inside the server executable?
There was a problem hiding this comment.
@clayne11 You are right! The test are supposed to include only one model.go with necessary interfaces (that are usually in the server.go). They supply it this way only for the purpose of a compact unit test I think.
|
BTW, I very much appreciate you working on this, as it helps to narrow the behavioral gap between single file and multi-file (where rewrite is currently supported). I don't believe anyone is relying on the behavior of single file NOT supporting rewrite, so I think it would be backwards compatible to use the existing |
|
Makes sense, let's remove the flag then and just make it default behavior. Thanks for reviewing @StevenACoffman. |
hailyngx
force-pushed
the
enable_rewrite_for_single_file
branch
from
ab1c02a to
df14c09
Compare
|
Updated based on your suggestion @StevenACoffman 🙏 Thank you Steve! |
hailyngx
changed the title
hailyngx
force-pushed
the
enable_rewrite_for_single_file
branch
3 times, most recently
from
22d8ef5 to
c355813
Compare
| ) | ||
|
|
||
| type CustomResolverType struct{} | ||
|
|
||
| // Resolver is the resolver for the resolver field. | ||
| func (r *queryCustomResolverType) Resolver(ctx context.Context) (*Resolver, error) { | ||
| panic("not implemented") | ||
| panic(fmt.Errorf("not implemented: Resolver - resolver")) |
There was a problem hiding this comment.
The string change is welcome, but why are you changing from:
panic("a problem")
to:
panic(fmt.Errorf("a problem"))
It seems unhelpful?
There was a problem hiding this comment.
Looks like I have to go back to change the default stubs of rewrite single-file to panic("not implemented"). panic(fmt.Errorf("not implemented: Resolver - resolver")) is currently the template for the default rewrite follow-schema. That's why the change is necessary for a git diff check, but if I make this change, the lint check does not like it. Also, looks like deleting resolver config is not a solution. Let me know if you have any suggestions!
There was a problem hiding this comment.
Not a big deal. We you would need to backtrack and change the template, but it's not actually important.
There was a problem hiding this comment.
@StevenACoffman Made a new commit that works for all tests. Coverage and security should be good to go even though they fail right? Thanks Steve for being so responsive!
There was a problem hiding this comment.
Yes! Thanks for sticking with it! Sorry this was such a long journey!
hailyngx
force-pushed
the
enable_rewrite_for_single_file
branch
from
c355813 to
61bcb03
Compare
|
Thanks for this work! |
Signed-off-by: Steve Coffman <[email protected]>
Signed-off-by: Steve Coffman <[email protected]>
BREAKING CHANGE
Resolvers generated using
layout: single-filewill now have their resolver files overwritten on generate.Why this change?
Make rewrite the default for single-file mode as no one is relying on the behavior of single file NOT supporting rewrite. Thus, it would be backwards compatible to use the existing
rewriteinstead of introducing the newenable_rewrite_for_single_fileflag.Relevant Discussion: #3205
I have: