-
Notifications
You must be signed in to change notification settings - Fork 296
Prevent introspection field __typename from being evaluated as an Entity Field #1086
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
Conversation
…ity field. This is a reserved GraphQL field name used for introspection and is automatically handled by HotChocolate.
src/Service/Resolvers/Sql Query Structures/SqlQueryStructure.cs
Outdated
Show resolved
Hide resolved
|
Can a database table actually have a column with name __typename ? Does it mean using graphql we cannot query a column in a table which has a reserved name? |
Good point, per GraphQL Specification, we should extend this logic to type references with double underscore (__).
|
|
SQL allows underscores in column names, so we may need to require aliases be used in our runtime config to enable those fields to be used in our GraphQL Endpoint https://learn.microsoft.com/en-us/sql/odbc/microsoft/column-name-limitations?view=sql-server-ver16 |
src/Service/Resolvers/Sql Query Structures/SqlQueryStructure.cs
Outdated
Show resolved
Hide resolved
aaronburtle
left a comment
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.
looks good just a nit and possible suggestion.
|
I think it is fine to require that exposed database columns or documents properties do not start with a double underscore. Make sure this requirement is documented. This shouldn't be big deal as using the mapping feature it will always be possible to change the name of a column or a property to something else. |
…d comments and test displaynames for associated integration tests.
src/Service/Resolvers/Sql Query Structures/SqlQueryStructure.cs
Outdated
Show resolved
Hide resolved
Aniruddh25
left a comment
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.
LGTM, thanks for the quick fix!
…sses no longer ignoring the enabled field for both global rest / graphql
aaronburtle
left a comment
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.
LGTM! Just some nits, and a couple comments about clarifying some large if statements.
…doing and correct grammar in comment.
…om/Azure/data-api-builder into dev/seleonar/typenameCompatibility
Why make this change?
What is this change?
How was this tested?
Sample Request(s)
Request
Response
{ "data": { "publishers": { "__typename": "PublisherConnection", "items": [ { "__typename": "Publisher", "name": "The First Publisher" }, { "__typename": "Publisher", "name": "Big Company" }, { "__typename": "Publisher", "name": "Policy Publisher 01" }, { "__typename": "Publisher", "name": "Policy Publisher 02" }, { "__typename": "Publisher", "name": "TBD Publishing One" }, { "__typename": "Publisher", "name": "TBD Publishing Two Ltd" }, { "__typename": "Publisher", "name": "Small Town Publisher" } ] } } }