Replies: 5 comments 3 replies
-
It feels to me that you're arguing against a misinterpretation of what the WG consensus is, so discussion definitely feels warranted to remove any ambiguities in communication. Allow me to address just a few of your initial points with my understanding:
I highly doubt that there was any WG consensus behind this statement, at least as currently stated.
The GraphQL spec is advancing, so I guess this is one of the contradictions in your logic that has helped you notice your understanding of the WG's consensus must be flawed, hence seeking this clarification. To start addressing this, I suggest that you carefully read the guiding principles themselves - they're what a lot of the GraphQL specification discussions centre around, directly or indirectly - and further note that they are guides rather than rules. In general: the benefit of any new feature must outweigh the cost. To address the quote directly: adding information to the introspection schema should generally not break any old tools because old tools only query fields that they knew about at the time, and we add new information via new fields so as to not break old tools. This is one of the reasons why the struct RFC maybe should enhance the Scalar type rather than the Input Object type - existing tools that see a Scalar will, in the vast majority of cases, not even look at the "fields" result, and thus adding information there should not confuse/break existing clients.
It isn't backwards incompatible on existing operations - existing operations are unaffected since they do not use recursive fragments (as you note). It's backwards incompatible on GraphQL schemas, because if the GraphQL library that powers a GraphQL schema is updated to have this restriction removed, suddenly the schema can receive queries that it was not expecting and go into an infinite loop. Previously the schema was relying on the "no recursive fragments" rule to ensure that it was safe, removing this rule would be a backwards incompatible change for the schema, and could introduce a serious DoS attack vector. This may be the case for your "GraphQL apps - (end apps) [...] Project is done, pushed to prod, team of consultants is disbanded" example, where a maintenance crew comes in and updates all the dependencies. Because of this risk to existing schemas, this is a significant consideration, and if we were to make this change we'd have to signpost it very heavily to ensure that as few people as possible were negatively impacted by it. |
Beta Was this translation helpful? Give feedback.
-
As far as I recall the discussion with regards to recursive fragments, the conclusion was that any execution engine would presumably include its own depth limit, and that it would probably pay to formalize the depth limit as a directive on the fragment. What that proposal was waiting for was a champion to advance an implementation and then the remaining question was whether this increased complexity would be worth it considering u could always add this as a “macro” where operations with recursive fragments with a depth limit were transformed into a non recursive version. For what it’s worth, I think that this feature is worth adopting, I just didn’t have time or really so much personal interest in championing it! I am not weighing in on your larger point — sort of on purpose. The point of this comment is just to clarify the state of recursive fragments. |
Beta Was this translation helpful? Give feedback.
-
One thing to note: we attempt to preserve compatibility in both directions. Existing documents should be able to run against new servers that implement the spec. And also new documents can be written with the current version of the spec that run against old, un-upgraded servers (potentially with an error thrown for new features that the old server does not implement). |
Beta Was this translation helpful? Give feedback.
-
I want to re-iterate on important point about my experiment with Input-as-Output change, that made Graphiql fail. This got a bit misunderstood I think, and at the end we were discussing it as breaking change that breaks introspection queries for old clients and old apps. And Matt was proposing some hypothetical auto-fixes like automatically generating identical input and output types etc. The Graphiql introspection query failed when it encountered an actual app model that uses Input type as output field type. For all old models/apps that do not use this explicitly, the updated server works OK with Graphiql. All introspection queries return identical data as before; I mentioned previously that Input and Object types still live in separate collections in introspection; nothing changes for old models and clients. Therefore, no old clients or apps will be broken if servers start supporting these features, everything works the same. It is only app dev starts using it in his/her model, it might break Graphiql - but then, no choice; if you want to use new features in your server app you gotta upgrade server framework and clients; and let know external clients about the change you are doing - but this is a conscious choice of the app developer. So I believe that Input-as-Output feature is still OK, doable, without breaking backward compatibility. |
Beta Was this translation helpful? Give feedback.
-
thank you. Now it's really clear for me. Really. And that explains a lot. I think with this interpretation of backward compatibility - no chance for any meaningful progress. I am outa here. Good luck with structs. Thank you again. |
Beta Was this translation helpful? Give feedback.
-
Introduction - why discuss such a basic thing?
One of the established rules for GraphQL spec development is "preserve backwards compatibility", and nobody argues against that.
It might seem that the term does not need any explanation - everybody knows what it means. However, based on few occasions here in GraphQL WG, in discussions and posts, it looks like different people have different interpretations of what it means and when it applies. I think sometimes we do not apply this rule correctly and misjudge the suggested change - people claim it breaks the 'backward compatibility', while it does not.
As an example, I was told allowing recursive fragments would break backward compatibility. "We remove the restriction no-recursion and it is backward incompatible". - Really? I am not sure. I will bring another example from our meeting discussions down below.
The GraphQL spec itself I think uses the term incorrectly in one place; more on this later.
In expectation of coming discussions of new features (recursive fragments, using input types as outputs, others), and expecting backwards compatibility considerations raised in these discussions, I believe we need to discuss this subject separately and upfront, to avoid arguing about this in the midst of other topics.
I suggest MY understanding of the term in this post, and how it should be applied in WG work Please comment and give your opinions. But I do believe that we need some SHARED understanding of the subject and how and where it applies.
Backward compatibility - software in general.
Old app code continues to work on newer versions of the underlying infrastructure and dependencies
This always presumes some limited forward compatibility on behalf of the app itself - a tolerance to some minor unexpected extra things. Don't break if you see some extra, just ignore it. And you cannot do anything about an app that just refuses to work with anything but a specific version (that was the hell with WS and SOAP, exact namespaces and specific versions of any single type, remember that?)
GraphQL backward compatibility - "narrow" definition
Backwards compatible spec change from versions V1 to V2 means that any GraphQL document(schema or query) valid for V1 is valid for V2.
Therefore:
Runtime and modifying standard structures (Introspection types, Error type)
Moving server infrastructure from V1 to V2 of the spec can cause some automatic changes in behavior, even for old V1 queries. Server responses might change. Example: introspection queries, new specifiedByUrl field. Another example, Error object: we might add a 'code' field (as had been suggested before), so old clients and apps might suddenly start seeing a new unknown field in errors with probably some default value.
In this case, GraphQL (WG) presumes the above mentioned limited forward compatibility of implementations - they should not error out if they encounter something extra in standard types.
Target areas of concern - apps, libraries, tools
Regarding tools, an interesting case from the past. On one of the meetings we were discussing some addition to introspection, I think it was "appliedDirectives", but I'm not sure. One of the objections was "Look, if we add this, but user uses some older version of a tool to inspect schema, the tool won't show this new field, so user will be unaware of some information, and this is really bad, so this change is not backwards compatible". And this kind of closed the discussion. Needless to say, I strongly disagreed. But starting arguing about 'what backward compatibility really is' was out of question, out of scope for the meeting, so this argument "won".
And this is why I am bringing it up here, as a separate discussion, to hopefully settle the issue for the future.
Now about the issue with tool itself. First, the rule is - use up-to-date tools, period. At least matching the version of API or server you are looking at. It is NOT WG's concern that somebody uses old tool. Secondly, the hypothetical case presented is NOT about backward compatibility, it is more about lack of reasonable 'forward compatibility of the tool". When encountering some unexpected element in the introspection type, the tool should not have ignored it, but should try to show the info to the user in some way, at least alert him/her that there's more. This is more like a fault on tool developers.
And third, following this logic, it is impossible to do any progress in the spec at all, because for any change there might be some old tool installed somewhere that does not know about this new thing.
Summary: suggested basic principles for advancing the spec
Improper use of the term backwards compatibility in the spec
Section Deprecation tells us that @deprecated directive is "to support the management of backwards compatibility...".
I think this is incorrect, deprecated is not about compatibility at all! It is about coming INCOMPATIBILITY. With backward compatibility the old code continues to work, you do NOT need to change/adjust it. In contrast, Deprecated means that old code will STOP working, an early warning that you'd better change your code soon, or it will start failing.
I suggest to change this paragraph to something like:
Being pedantic: Backward or Backwards?
Interesting explanation here: https://www.quickanddirtytips.com/articles/backward-versus-backwards/
So it looks like it should be always "Backward compatibility" without 's' for an adjective, and either with or without it for an adverb, both forms are OK: "Backward compatible" or "Backwards compatible"
Real life case: Inputs as Outputs implementation and encountered challenges
Some time ago I was involved in a GraphQL service project that had many simple types, data containers with a few properties. These objects were submitted to server by a producer service, and then queried by other multiple clients. So the types were used as both input and output types. That by the way is a very common case I believe.
I started with defining identical input and output types, lots of them. The schema bloated, and it just did not make sense. So I decided to just implement in NGraphQL this long requested and discussed feature - allowing returning input types as field output types. The basic idea for implementation: "Input types are just restricted Object types, so any Input type is also Object type." Restrictions are - no arguments on fields, and only primitive or Input types for fields. So there are no separate collections of Input and Object types, just Object types, and some have IsInputType flag set. But introspection returns them in separate collections, as before. They are still declared as Input and Object types in the schema and everywhere.
So I did implement it, and here are the findings; they may be interesting in the context of this discussion
My statement: this change is backwards compatible, and troubles with Graphiql are NOT a break of backward compatibility. The tool(s) can be fixed if this feature is put into the spec.
Change my mind :)
Beta Was this translation helpful? Give feedback.
All reactions