-
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
Fix nil pointer dereference when an invalid import is bound to a model #1676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks 🙏
I think this is a very sensible change, though keen for @giuliano-oliveira and @StevenACoffman to have a look before merging this in. |
Totally accidental, sorry!
if errors.Is(err, config.ErrTypeNotFound) { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what other types of Errors can this err variable be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giuliano-oliveira I'm not totally sure all the errors! The primary error I'm trying to avoid here is one where the type exists, but the field does not exist on the type.
For example, maybe you have a schema like this:
type Query {
me: User!
}
type User {
id: ID!
email: String!
hasTwoFactorAuthenticator: Boolean!
}
And you have a model in Go:
type User struct {
ID string
Email string
}
During the codegen planning phase, we determine that because hasTwoFactorAuthenticator
is missing, a method will be created on the generated meResolver
called HasTwoFactorAuthentication
. This is in addition to the Me
method created on the queryResolver
.
If we return all errors from this function, the behavior changes - the caller will also return an error and code generation will not occur. To fix it, you would need to add variables/methods to the models themselves. In some cases, it makes more sense to use a resolver (e.g. because you need to make an additional DB call) rather than defining the logic on the model.
The problem with the specific error of "the type does not exist" is that it's difficult (perhaps impossible?) to generate or validate code for an unknown type. So the solution is to bail out of the codegen process if this is detected. I think it'd be even nicer if we could somehow avoid that specific codegen tree without halting codegen entirely, but that seems like a larger change that requires a little more knowledge than I have of the codegen process.
This is great! Thanks to both @giuliano-oliveira and @johnmaguire |
Fixes #1625, supersedes #1626. It includes the same fix but retains previous behavior generating resolvers for missing fields.
Basically I am just differentiating between the two types of errors: Is there a specified type to bind that can't be found? Or is there simply a missing implementation that we should fill in?
In the former case, we bail out - we know that this path will only lead us to panics and sadness.
In the latter case, we stay the course - we know that the codegen will fit in.
While this fixes the panic and improves things, there may be other oddities that could occur due to the fact that the order is different from run to run.
My biggest concern would be a case where we can't find the referenced type but still generate some code. However, I don't think this is possible - first we build a codegen plan, then we execute it. We should bail out prior to execution.
edit - still, the errors printed to the console may differ between runs, as these are generated during the planning phase which will not complete if there's a missing type.
I have: