-
Notifications
You must be signed in to change notification settings - Fork 734
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: Schema name conflict errors #2589
Conversation
✅ Deploy Preview for apollo-ios-docs canceled.
|
Just a typo and minor addition to the schema conflict error message. But otherwise this LGTM! |
Still wanting to add the entity vs. list behaviours to this PR too. |
Co-authored-by: Anthony Miller <anthonymdev@gmail.com>
5f87ef8
to
ba78293
Compare
if (isListType(fieldType) || (isNonNullType(fieldType) && isListType(fieldType.ofType))) { | ||
validateFieldName(selectionNode, validationOptions.disallowedEntityListFieldNames) | ||
} else if (isCompositeType(unwrappedFieldType)) { | ||
validateFieldName(selectionNode, validationOptions.disallowedEntityFieldNames) |
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.
Arguably this check could go into the below if (isCompositeType(unwrappedFieldType)) {
statement, let me know if you feel strongly about it. I prefer to have the if else
and keep the logic together.
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.
Also, if we move the check into the if statement below then the validation will be done twice because a field can be both a list type and and a composite type.
} | ||
} | ||
|
||
func test__validateDocument__givenFieldNameIsSchemaName_throwsError() throws { |
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.
This test now becomes part of the compilation stage tests.
@AnthonyMDev this PR now has the entity and list disallow logic. I still need make the change to condense the ValidationOptions into a sub struct but the rest is ready for another pass. |
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.
Thanks so much @calvincestari, I know this one was a lot harder than we expected!
PR 2 of 3 for #2546
This PR ensures that the user-given schema name: