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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw exception on multi-level interface #420

Merged
merged 2 commits into from Oct 16, 2019

Conversation

smyrick
Copy link
Contributor

@smyrick smyrick commented Oct 2, 2019

馃摑 Description

According to the spec, an interface can not implement another interface. This is not caught by our schema generator but instead just returns the first level interface as the type in the schema. We are now throwing an exception to help translate this understanding of GraphQL through the Kotlin code

Is this a breaking change? Someone may be using 1.0.0 today and just by updating their version they will have to fix something.

馃敆 Related Issues

Resolves #419

@smyrick smyrick added the type: enhancement New feature or request label Oct 2, 2019
@smyrick
Copy link
Contributor Author

smyrick commented Oct 2, 2019

I would also be fine not merging this and just keeping the implementation today that only includes the first level

@codecov-io
Copy link

codecov-io commented Oct 2, 2019

Codecov Report

Merging #420 into master will decrease coverage by 0.06%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #420      +/-   ##
============================================
- Coverage     97.81%   97.74%   -0.07%     
- Complexity      325      326       +1     
============================================
  Files           105      106       +1     
  Lines          1236     1242       +6     
  Branches        204      207       +3     
============================================
+ Hits           1209     1214       +5     
  Misses            7        7              
- Partials         20       21       +1
Impacted Files Coverage 螖 Complexity 螖
...up/graphql/exceptions/InvalidInterfaceException.kt 100% <100%> (酶) 1 <1> (?)
...p/graphql/generator/extensions/kClassExtensions.kt 100% <100%> (酶) 0 <0> (酶) 猬囷笍
...agroup/graphql/generator/types/InterfaceBuilder.kt 100% <100%> (酶) 2 <0> (酶) 猬囷笍
...oup/graphql/generator/filters/superclassFilters.kt 80% <50%> (-20%) 0 <0> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update f0b3590...449b198. Read the comment docs.

@smyrick smyrick added status: do not merge Do not merge until this is removed and removed status: do not merge Do not merge until this is removed labels Oct 3, 2019
Resolves ExpediaGroup#419

According to the spec, an interface can not implement another interface. This is not caught by our schema generator but instead just returns the first level interface as the type in the schema. We are now throwing an exception to help translate this understanding of GraphQL through the Kotlin code
@smyrick smyrick added the status: do not merge Do not merge until this is removed label Oct 16, 2019
@smyrick
Copy link
Contributor Author

smyrick commented Oct 16, 2019

Question for the team? Do we need to look multiple levels deep when resolving interfaces. Currently we only look one level from the class

@dariuszkuc
Copy link
Collaborator

Question for the team? Do we need to look multiple levels deep when resolving interfaces. Currently we only look one level from the class

I think supporting generation of the schema based on class -> common abstract (ignored) -> interface pattern might be useful. Currently we don't support that as we only look at direct ancestors.

@smyrick smyrick added changes: minor Changes require a minor version and removed status: do not merge Do not merge until this is removed labels Oct 16, 2019
@smyrick
Copy link
Contributor Author

smyrick commented Oct 16, 2019

@dariuszkuc Updated to return multiple levels deep now

@dariuszkuc dariuszkuc merged commit ab2acff into ExpediaGroup:master Oct 16, 2019
@smyrick smyrick deleted the multi-level-interface branch September 3, 2020 21:55
dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this pull request Aug 5, 2022
* Throw exception on multi-level interface

Resolves ExpediaGroup#419

According to the spec, an interface can not implement another interface. This is not caught by our schema generator but instead just returns the first level interface as the type in the schema. We are now throwing an exception to help translate this understanding of GraphQL through the Kotlin code

* Return superclasses from multiple levels deep
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: minor Changes require a minor version type: enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Interfaces of Interfaces is invalid GraphQL
3 participants