fix: side effects in tag references #2032
Conversation
Signed-off-by: Vincent Biret <[email protected]>
baywet
requested review from
irvinesunday,
darrelmiller,
gavinbarron,
millicentachieng,
MaggieKimani1 and
andrueastman
as code owners
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Vincent Biret <[email protected]>
|
I'm not sure I agree with this. Tag references are very similar to using $id to reference a Schema, where the target object is implicitly created. I agree that it was a bit of a stretch when $refs could only use locators, but now that we need to support $refs as identifiers, I think there is a strong parallel. |
|
The previous model results in tons of side effects. A simple example:
Same goes for the description, the tag name etc... |
|
Isn't that the expected behaviour? |
From a proxy design pattern, sure. But from a functional point of view, I'd say it's pretty confusing/could lead to nasty production bug since the object model is the same, and the behaviour was different. The message I'm trying to convey here is that we're in control of what properties (or operations) can be proxied, IMHO we should allow reads, and then ask people to go through the Target property for writes. That of course leaves a big question for collections. |
| @@ -10,21 +12,24 @@ namespace Microsoft.OpenApi.Models.References | |||
| /// <summary> | |||
| /// Tag Object Reference | |||
| /// </summary> | |||
| public class OpenApiTagReference : OpenApiTag | |||
| public class OpenApiTagReference : OpenApiTag, IOpenApiReferenceable | |||
There was a problem hiding this comment.
Why does this need to implement IOpenApiReferenceable? None of the other proxy objects do.
There was a problem hiding this comment.
Apparently it is a bug in every other reference class because the walker expects all references to implement IOpenApiReferencable /cc @MaggieKimani1
|
Sorry, I was being slow. I agree that editing the reference should not edit the target. I agree with your changes. |
That just means the vacations were good! |
| OpenApiTag resolved = new OpenApiTag(_target); | ||
| if (!string.IsNullOrEmpty(_description)) resolved.Description = _description; | ||
| return resolved; | ||
| _target ??= Reference.HostDocument?.Tags.FirstOrDefault(t => StringComparer.Ordinal.Equals(t.Name, Reference.Id)); |
There was a problem hiding this comment.
Is there any opposition to retaining the fallback logic that sets the target to an OpenApiTag when there isn't a tag to reference in the document?
This affects an old ASP.NET Core scenario where we create an "orphaned" OpenApiOperation that gets attached to a document later. To workaround this, I have to create a temporary document object just to store the tags.
There was a problem hiding this comment.
Thanks for the feedback!
References still need a LOT of love at this point #1998
Relaxing the instantiation of references without NRT support #1202 makes it extremely difficult not to mess something up. Effectively I want the compiler to yell at us (devs on OAI.NET) when some part of the code forgot to check for nullability etc...
It'll also provide a much better experience to consumers like you by providing feedback when something was forgotten, etc...
Eventually, when that gets implemented, and the reference proxy design pattern is corrected, I think it'll be much easier to relax the constraints I hardened with this (and other) PRs in preview5. And eventually provide a walker that sets the document all over the place when absent for references before serialization.
What do you think of this "plan"?
This fixes multiple issues:
We should still break the inheritance, and use a base abstract class as well.