Comments (29)
This is so bad, I have several TS interfaces and I expect my results to match those interfaces, if I start aliasing the unions then they won't match and I'm going to have to convert the results to match the TS interfaces.
I'm going to use just a plain JSONResolver
for the conflicting fields. Graphql should support unions for scalars, and if that goes against the spec, then the spec is probably wrong and should be fixed. There should be at least a way to relax this or provide a solution that does not imply aliasing or having to interpret the results after in any way.
Union types are ambiguous by definition, you either support them or you don't.
from graphql-js.
Name one other type system that has a union type that requires fields of the unioned types to match...this was a nasty surprise and it makes me angry. And the fact that it was a deliberate decision even moreso.
In my case I had two types that unioned just fine until today, when I needed to make a field on one of them optional. Now I have to do extra work to de-alias the conflicting field in the query result 🙁
from graphql-js.
Imagine if "title" returned "foo" and "title(language: fi" returned "bar" which value would the "title" key be?
Whichever one corresponds to the __typename
of the resolved object...Flow and TS are perfectly capable of representing union types where the unioned objects have fields with the same name and different types:
type Program = {
__typename: 'Program',
title: 'foo',
}
type Article = {
__typename: 'Article',
title: 'bar',
}
type CuratedListItem = Program | Article
And as I mentioned in my above comment, I don't know of any language whose type system is incapable of representing a union where field types mismatch.
Even if you're not querying the __typename
field there would often be ways to do type refinements in Flow or TS with runtime checks, but if that proved difficult you would just solve it by including __typename
in the query. I fail to see how the different field types would actually cause problems.
from graphql-js.
I think we could probably improve this if two fields are known to never "merge". In this case, the two types are Object types and in no condition would both "title" fields be queried for the same object.
We should be a bit smarter about this (likely common) case
from graphql-js.
I don't understand why querying for a field with different types for some members of a union should result in an error iff you include the __typename
in each type's fragment. There can be no ambiguity.
from graphql-js.
@IvanGoncharov They do have predictable types depending on the parent type, so I do not really understand your statement. I will also add that the behavior was already changed for aliases to allow this very thing, so I do not see how this is any different.
from graphql-js.
@mkochendorfer as @IvanGoncharov said, this is correct by the spec. Your example is actually one of few remaining areas of the spec that rubs me the wrong way; I definitely see its utility in the context of implementing parsers in certain languages, but with only a cursory analysis I found the restriction unnecessarily, given that client limitations shouldn't limit the abilities of the server (instead, they can just add aliases to their queries when they enter such a scenario, as all clients must do now).
I've currently got two projects I'm trying to champion for GraphQL (implementation of interfaces by interfaces and big TS changes), but as soon as one of these wraps up, I'm going to try to formalize a case for removing this restriction, and present it to the working group.
from graphql-js.
Any reason why this /src/validation/rules/OverlappingFieldsCanBeMerged.js@master#L603 type check cannot be moved up inside of the if (!areMutuallyExclusive) { conditional?
@mkochendorfer Response fields should have predictable type it should be the single type not the combination of string
and ComponentTwoBackground
. Also implemented according to GraphQL Specification: https://graphql.github.io/graphql-spec/draft/#SameResponseShape()
from graphql-js.
How is this still a problem 8 years later this is really frustrating
from graphql-js.
@leebyron So it appears this issue was addressed for aliases but never for actual type definitions. Consider this example schema:
type Query {
components: [Component]
}
interface Component {
id: ID
}
type ComponentOne implements Component {
id: ID
background: String
}
type ComponentTwo implements Component {
id: ID
background: ComponentTwoBackground
}
type ComponentTwoBackground {
color: String
image: String
}
With this schema the following query would be very legitimate:
query {
components {
id
... on ComponentOne {
background
}
... on ComponentTwo {
background {
color
image
}
}
}
}
However, this will always give you an error about the types of the background fields not matching . Any reason why this https://github.com/graphql/graphql-js/blob/master/src/validation/rules/OverlappingFieldsCanBeMerged.js#L603 type check cannot be moved up inside of the if (!areMutuallyExclusive) {
conditional?
from graphql-js.
Yeah, I guess what I am fundamentally saying is that based on #53 (comment) and the resulting PR #229 it seems like the spec is already our of sync with this implementation. That original PR to address this issue seems like it would have worked for my case actually as it just exits immediately if the parent types differ. That code has since been changed causing my issue to resurface. https://graphql.github.io/graphql-spec/draft/#example-54e3d seems like a fundamentally wrong counter example based on all of the above, so if that means the spec needs to be updated to get this corrected, then I am all for it.
Is there some reason behind this restriction in the spec?
from graphql-js.
Ugly workaround: If you patch graphql and remove the rule everything works fine (except linter in explorer, that will still complain) and its a 1-liner. So maybe we can have an option for disabling that rule (or replace it with a less strict version)?
diff --git a/validation/specifiedRules.js b/validation/specifiedRules.js
index 014d23b0aa30a76dba8ec812fbc76cfadc4e7100..dda780fd838378bf789e9b13ed77b0fb6d6a81c9 100644
--- a/validation/specifiedRules.js
+++ b/validation/specifiedRules.js
@@ -31,8 +31,6 @@ var _NoUnusedFragmentsRule = require('./rules/NoUnusedFragmentsRule.js');
var _NoUnusedVariablesRule = require('./rules/NoUnusedVariablesRule.js');
-var _OverlappingFieldsCanBeMergedRule = require('./rules/OverlappingFieldsCanBeMergedRule.js');
-
var _PossibleFragmentSpreadsRule = require('./rules/PossibleFragmentSpreadsRule.js');
var _PossibleTypeExtensionsRule = require('./rules/PossibleTypeExtensionsRule.js');
@@ -132,7 +130,6 @@ const specifiedRules = Object.freeze([
_ValuesOfCorrectTypeRule.ValuesOfCorrectTypeRule,
_ProvidedRequiredArgumentsRule.ProvidedRequiredArgumentsRule,
_VariablesInAllowedPositionRule.VariablesInAllowedPositionRule,
- _OverlappingFieldsCanBeMergedRule.OverlappingFieldsCanBeMergedRule,
_UniqueInputFieldNamesRule.UniqueInputFieldNamesRule,
]);
/**
from graphql-js.
Running into this as well. Pretty major limitations for union types and interfaces. From my perspective, if we've used resolveType
to disambiguate the parent type and provide it the correct __typename
, then I don't see why any children properties of that top level type need to share properties...
from graphql-js.
This just bit me. I designed an entire section of schema around a union thinking that query fragments would be sufficient to disambiguate the possible values of a field whose name is shared by all members of the union but whose return type differs. In my case the field types are different enum
s, and of course GraphQL can't union scalars or enum
types, nor does it have a "constant" or "literal" type like Typescript, which will happily let you declare type Foo = 'foo'
or type Foo = { name: 'Foo' }
. I can't use an interface because its type requirements are even more strict than a union.
from graphql-js.
If you wish to query for both you should alias at least one of the fields to be unambiguous.
from graphql-js.
Can you explain what part you do consider as shooting you into feet?
The precedence thing might be such, but I don't think my original example with program and article title is that unreasonable.
from graphql-js.
Dang, just stumbled upon this issue while trying to use fragments on a union-typed field with types that share a field name but each is its own type:
fragment myFragment on Entity {
... on Player{
type
...playerFragment
}
... on Coach{
type
...coachFragment
}
}
GraphQLDocumentError: Fields "type" conflict because they return conflicting types "PlayerType" and "CoachType".
Use different aliases on the fields to fetch both if this was intentional.
Any way to ignore this error?
from graphql-js.
If you're querying different fields of the unioned types, a given field isn't guaranteed to be present in the response. So why does a field need to have a guaranteed type in the response either? Even if field types merge, they could have different semantic meaning that might be ambiguous if the query doesn't include __typename or other distinguishing fields.
from graphql-js.
This is currently working as designed, but feedback is welcome.
The design goal here is to make the resulting response unambiguous.
If for example, Article and Program were both Interfaces that one type adhered to, it would be unclear which variant of "title" was being requested.
from graphql-js.
This is very deliberate. Any time that two fields that "merge" can potentially return different results we do not allow this.
Imagine if "title" returned "foo" and "title(language: fi" returned "bar" which value would the "title" key be?
from graphql-js.
Is there any possibility for confusion when using the ...on TypeName { ... }
query, even if interfaces were used instead of union types?
If there was a SearchResult interface that defined a field pictureUrl(width: Int)
and a title
. The interface is implemented by Page
and Image
. Wouldn't it be a valid use case to do something like this:
...on Page { title, pictureUrl(width: 120) }
...on Image { title, pictureUrl(width: 600) }
This would also result in error with current implementation.
from graphql-js.
Actually, even if the query was something like
title,
pictureUrl(width: 120),
...on Image { pictureUrl(width: 600) }
that'd still be pretty easy to reason about: select 120px picture url for anything that isn't an Image, and 600px for Image.
from graphql-js.
The only problem I can think of is with case where an interface defines a field with some set of args and it's implemented with different set of args. For example, if Article
and Program
implemented a HasTitle
interface with a title
field w/o args, it would clearly be an error for Program
to have one with language
arg.
In this case it would be pretty hard to respond to query like ...on HasField { title }
, because you can only apply that query to Article
- Program
needs the language
arg.
But this should be a compile time check to ensure you can't create such schema in the first place. Instead of it happening on query time.
from graphql-js.
title,
pictureUrl(width: 120),
...on Image { pictureUrl(width: 600) }
This case definitely presents an issue, since if you have an object which is of type Image
, then it's unclear what width your pictureUrl
field will be prepared with, both the field with 120
and with 600
would apply. This is exactly the ambiguity we're trying to avoid.
from graphql-js.
@leebyron, I think it's quite intuitive way of saying just give me 120px preview image for search results that aren't an Image
and 600px for Image
s.
You could think of it as ...on Image { ... }
"block" giving a precedence over anything else in this query, because it applies to a specific type. So, for Image
objects the pictureUrl(width: 600)
would have higher precedence because it was requested inside an ...on Image { ... }
block.
from graphql-js.
We shouldn't add more ways to shoot ourself into our feet! We already have very powerful features in the language to allow very flexible queries!
In your SearchResult example the different result types must be treated in different ways anyway (why else would they return images of different sizes if not to be displayed differently), so you have to do checks when iterating over the results. Using an alias in the ... on Image { ... }
fragment increases readability and reduces unpredictable behavior.
from graphql-js.
I'll have this out in the next npm release soon
from graphql-js.
it should be the single type not the combination of string and ComponentTwoBackground
Well GraphQL is also weird to me in that it only allows object types to be unioned. This issue seems like an unintended consequence of that design decision.
All other type systems with unions that I'm aware of allow primitive types to be unioned as well, if numbers and strings aren't first-class objects to begin with.
If GraphQL allowed primitive types to be unioned there would be nothing weird about the field being a combination of string and ComponentTwoBackground.
from graphql-js.
Why is this issue closed?
from graphql-js.
Related Issues (20)
- IHeyReally.org
- Suggestion: Bundling in v17, ESM, CJS, and the dual package hazard HOT 9
- Just to give my 2c, I'm not sure if `exports.development` and `exports.production` target conditionsa are widely supported, so some `import.env` shenanigans may still be necessary until that's widely adopted. HOT 1
- Collection of libraries and how they import from `graphql` HOT 20
- `process.env`, `globalThis`, and `typeof process` HOT 21
- Introspection queries don't support `@oneOf` HOT 2
- Tutorial data HOT 2
- astFromValue fails with a custom scalar serializing to an object value HOT 5
- In a response to a query about an Issue, the URL and other info is missing for links created with Reference editor button HOT 5
- author/committer -> user fields returning NULL for commits committed by user
- [email protected]
- npm link with graphql package breaks application HOT 2
- IHeyReally.com HOT 1
- Can I ask what is the progress here? Is there a solution being worked on? Do we have some timeline? Or progress with issue? Thanks!
- IHeyReally.com
- Typescript error with 16.9.0 (re ThunkObjMap) HOT 3
- Notice: default branch is now `16.x.x` HOT 2
- <a href="https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/12261526/38307428/4135/#/?version=2"><img src="https://s3.amazonaws.com/cla-project-logo-prod/cla-not-signed.svg" alt="CLA Not Signed" align="left" height="28" width="328"></a><br/><br /><ul><li><a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/12261526/38307428/4135/#/?version=2' target='_blank'>:x:</a> - login: @Heyitsquoracom / name: [email protected] . The commit (b6081f914f0c5c22ee48d26aff4b473bf17627ce) is not authorized under a signed CLA. <a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/12261526/38307428/4135/#/?version=2' target='_blank'>Please click here to be authorized</a>. For further assistance with EasyCLA, <a href='https://jira.linuxfoundation.org/servicedesk/customer/portal/4' target='_blank'>please submit a support request ticket</a>.</li></ul><!-- Date Modified: 2024-06-28 04:40:51.630768 -->
- Error with version v16.9.0: `Can't resolve '@apollo/subgraph/dist/directives''` HOT 1
- Type is incorrect in graphql ExecutionResult with new type GraphQLFormattedError HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from graphql-js.