-
Notifications
You must be signed in to change notification settings - Fork 251
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: side effects in tag references #2032
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT license. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using Microsoft.OpenApi.Interfaces; | ||
using Microsoft.OpenApi.Writers; | ||
|
||
|
@@ -10,21 +12,24 @@ | |
/// <summary> | ||
/// Tag Object Reference | ||
/// </summary> | ||
public class OpenApiTagReference : OpenApiTag | ||
public class OpenApiTagReference : OpenApiTag, IOpenApiReferenceable | ||
{ | ||
internal OpenApiTag _target; | ||
private readonly OpenApiReference _reference; | ||
private string _description; | ||
|
||
private OpenApiTag Target | ||
/// <summary> | ||
/// Reference. | ||
/// </summary> | ||
public OpenApiReference Reference { get; set; } | ||
|
||
/// <summary> | ||
/// Resolved target of the reference. | ||
/// </summary> | ||
public OpenApiTag Target | ||
{ | ||
get | ||
{ | ||
_target ??= Reference.HostDocument?.ResolveReferenceTo<OpenApiTag>(_reference); | ||
_target ??= new OpenApiTag() { Name = _reference.Id }; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any opposition to retaining the fallback logic that sets the target to an 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... What do you think of this "plan"? |
||
return _target; | ||
} | ||
} | ||
|
||
|
@@ -37,50 +42,43 @@ | |
{ | ||
Utils.CheckArgumentNullOrEmpty(referenceId); | ||
|
||
_reference = new OpenApiReference() | ||
Reference = new OpenApiReference() | ||
{ | ||
Id = referenceId, | ||
HostDocument = hostDocument, | ||
Type = ReferenceType.Tag | ||
}; | ||
|
||
Reference = _reference; | ||
} | ||
|
||
internal OpenApiTagReference(OpenApiTag target, string referenceId) | ||
/// <summary> | ||
/// Copy Constructor | ||
/// </summary> | ||
/// <param name="source">The source to copy information from.</param> | ||
public OpenApiTagReference(OpenApiTagReference source):base() | ||
{ | ||
_target = target; | ||
|
||
_reference = new OpenApiReference() | ||
{ | ||
Id = referenceId, | ||
Type = ReferenceType.Tag, | ||
}; | ||
Reference = source?.Reference != null ? new(source.Reference) : null; | ||
_target = source._target; | ||
|
||
} | ||
|
||
private const string ReferenceErrorMessage = "Setting the value from the reference is not supported, use the target property instead."; | ||
/// <inheritdoc/> | ||
public override string Description | ||
{ | ||
get => string.IsNullOrEmpty(_description) ? Target?.Description : _description; | ||
set => _description = value; | ||
} | ||
public override string Description { get => Target.Description; set => throw new InvalidOperationException(ReferenceErrorMessage); } | ||
|
||
/// <inheritdoc/> | ||
public override OpenApiExternalDocs ExternalDocs { get => Target?.ExternalDocs; set => Target.ExternalDocs = value; } | ||
public override OpenApiExternalDocs ExternalDocs { get => Target.ExternalDocs; set => throw new InvalidOperationException(ReferenceErrorMessage); } | ||
|
||
/// <inheritdoc/> | ||
public override IDictionary<string, IOpenApiExtension> Extensions { get => Target?.Extensions; set => Target.Extensions = value; } | ||
public override IDictionary<string, IOpenApiExtension> Extensions { get => Target.Extensions; set => throw new InvalidOperationException(ReferenceErrorMessage); } | ||
|
||
/// <inheritdoc/> | ||
public override string Name { get => Target?.Name; set => Target.Name = value; } | ||
public override string Name { get => Target.Name; set => throw new InvalidOperationException(ReferenceErrorMessage); } | ||
|
||
/// <inheritdoc/> | ||
public override void SerializeAsV3(IOpenApiWriter writer) | ||
{ | ||
if (!writer.GetSettings().ShouldInlineReference(_reference)) | ||
if (!writer.GetSettings().ShouldInlineReference(Reference)) | ||
{ | ||
_reference.SerializeAsV3(writer); | ||
return; | ||
Reference.SerializeAsV3(writer); | ||
} | ||
else | ||
{ | ||
|
@@ -91,10 +89,9 @@ | |
/// <inheritdoc/> | ||
public override void SerializeAsV31(IOpenApiWriter writer) | ||
{ | ||
if (!writer.GetSettings().ShouldInlineReference(_reference)) | ||
if (!writer.GetSettings().ShouldInlineReference(Reference)) | ||
{ | ||
_reference.SerializeAsV31(writer); | ||
return; | ||
Reference.SerializeAsV31(writer); | ||
} | ||
else | ||
{ | ||
|
@@ -105,10 +102,9 @@ | |
/// <inheritdoc/> | ||
public override void SerializeAsV2(IOpenApiWriter writer) | ||
{ | ||
if (!writer.GetSettings().ShouldInlineReference(_reference)) | ||
if (!writer.GetSettings().ShouldInlineReference(Reference)) | ||
{ | ||
_reference.SerializeAsV2(writer); | ||
return; | ||
Reference.SerializeAsV2(writer); | ||
} | ||
else | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to implement IOpenApiReferenceable? None of the other proxy objects do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently it is a bug in every other reference class because the walker expects all references to implement IOpenApiReferencable /cc @MaggieKimani1