In !36209 (merged) a compatibility layer was introduced to allow us to change the types
of arguments from ID_TYPE to GlobalIDType[T]. This requires us to be careful and add
lots of explicit coercions.
This issue is for the removal of the compatibility layer once
the deprecation cycle has been run, and all stakeholders informed.
In the description, we can read all stakeholders informed. Has this already happened, or do we need to do something else? Maybe we have to add an entry in https://docs.gitlab.com/ee/update/deprecations.html.
Do we need to work on this issue exactly at 15.0, or can we do it early, eg. 14.10?
That is the extent of the breakage - types named in query signatures might need updating. Values included inline will not need to be changed, which means this query:
The reason I say that breakage is unlikely is that users would have literally no reason to use ID. If you look at the docs for these features, they all list the correct argument types, eg.
gql> :type Query.issueIssueFind an issue.arguments:┌───────┬─────────────┬─────────────────────────────────────┬──────────────────┐│name │type │description │deprecated? │├───────┼─────────────┼─────────────────────────────────────┼──────────────────┤│id │IssueID! │Global ID of the issue. │No │└───────┴─────────────┴─────────────────────────────────────┴──────────────────┘
and they have done for about over a year. Any user building a query will use the correct types. All our own queries have been updated.
If we are very worried, we can delay doing this until we put in some monitoring to determine how frequently users are passing in the wrong type to get a sense of the scale of any disruption. I would estimate the effort of introducing this monitoring as about backend-weight2 (we have very obvious places to put these logs).
That's super helpful @alexkalderimis. My vote is that we can remove early and we can communicate the minor risk ahead of time unless @arturoherrero objects.
The former blocks the latter, since we have to update some front-end
queries that still have the old types in them (and we need to sequence
these changes for safety).
We also have to determine if a compatibility feature for iteration cadences
can be removed - if not then this will require a schema change and require
moving !83457 (merged) to %15.0.
Are we still on track with this for %15.0? I've started to run across a couple places where this is making the graphql upgrade &7734 (closed) a little more difficult. Is it possible to remove early in this release?
@arturoherrero I don't think we should let this block us. %15.0 is the big deprecation release. @alexkalderimis can correct me if I'm wrong, but we should be able to remove all uses of the compatibility layer, except maybe in this one instance if absolutely necessary.
But everything else can be removed. Though I really hope we can prioritize !83457 (comment 927994159) and get everything removed this release.
@digitalmoksha - it really depends on what we mean by 'everything else can be removed'.
We can remove the calls to coerce_isolated_input, for sure, but if we don't also remove the compatibility layer itself in GlobalIDType then we will introduce different failure modes, depending on the way the user passes values.
Now
We remove the GlobalIDType subtype equality hack
We only remove coercions
The user uses a literal value
OK, invalid values are rejected
OK, invalid values are rejected
OK, invalid values are rejected
The user uses a variable, typed as SpecificID
OK, invalid values are rejected
OK, invalid values are rejected
OK, invalid values are rejected
The user uses a variable, typed as ID
OK, invalid values are rejected
The query is rejected as invalid (1)
We return 500 errors (2)
As we can see, the big difference is for queries of the form:
query($id:ID){thing(id:$id){importantInfo}}
where the field is actually Query.thing(id: SpecificID).
We want to get to (1), where this query is rejected outright, which a sensible error message.
If we remove the coercions before removing the subtype equality hack, then we will receive strings
in resolvers where we expect GlobalID instance, try to do things like call id.model_id and
it will blow up with a NoMethodError, or a TypeError, and we will return 500 Internal Server Error.
Now this is not totally unacceptable (I'll leave the level of unacceptability to @g.hickman), since:
in either case the query will result in an error
the big difference is the presence of a nice error message
500 errors will pollute our logs and sentry
This change is announced as deprecated, so users should not have any expectation that it will work
It is far from ideal to knowingly start producing 500 errors, but we could go down that route if we
cannot fix the blocking frontend query.
Would it be possible to target the compatibility layer to a specific global id type, for only a MilestoneID and IterationID? Anything else would get pushed into normal validation and give us the error message rather than a 500?
Better to get !83457 (comment 927994159) fixed, but in case that's not possible, either a targeted check or I'm fine with the 500s.
Would it be possible to target the compatibility layer to a specific global id type, for only a MilestoneID and IterationID?
yes, but that would leave us in the odd position that MilestoneID and IterationID would unify with ID but nothing else would. It would require keeping all coercions for these two ID types. I think overall that is messier than accepting some 500 errors.
Hello @alexkalderimis I have SaaS customer that is waiting to coordinate this change with theirs. Other than setting up notifications for this issue, is it possible to get a heads up of when this deprecation will take place in SaaS?
We are aiming to remove as much of the deprecated functionality as possible in %15.0.
We encourage all customers to remove any reliance on deprecated functionality as soon as possible, there is no need to wait here for the removal. I would also encourage interested parties to follow the progress of !86195 (merged) and !83457 (merged).
@alexkalderimis - Based on above, can we mark the issue Closed or do we need to update the milestone to %15.1? Or, we could split out the remaining task if that makes sense as well.