Skip to content

StackOverflow Exception on circular reference #54

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

Open
Frankyfrankly opened this issue Feb 22, 2021 · 12 comments · Fixed by #138
Open

StackOverflow Exception on circular reference #54

Frankyfrankly opened this issue Feb 22, 2021 · 12 comments · Fixed by #138
Labels
bug Something isn't working

Comments

@Frankyfrankly
Copy link

Frankyfrankly commented Feb 22, 2021

Here is a code sample to reproduce the issue:

    public class Function1
    {
        private const string ContentType = "application/json";
        public List<Book> Lib { get; set; } = new List<Book>();

        [FunctionName(nameof(AddBook))]
        [OpenApiOperation(operationId: nameof(AddBook), Visibility = OpenApiVisibilityType.Important)]
        [OpenApiRequestBody(contentType: ContentType, bodyType: typeof(Book), Required = true)]
        [OpenApiResponseWithBody(statusCode: HttpStatusCode.OK, contentType: ContentType, bodyType: typeof(List<Book>))]
        public IActionResult AddBook([HttpTrigger(AuthorizationLevel.Function, "post", Route = null)][FromBody] Book req, ILogger log)
        {
            Lib.Add(req);
            return new OkObjectResult(Lib);
        }
    }

    public class Book
    {
        public string Name { get; set; }
        public DateTime PublicationDate { get; set; }
        public Author Author { get; set; }
    }

    public class Author
    {
        public string Name { get; set; }
        public List<Book> Books { get; set; }
    }

It seems like recursion happens in Visit method of ObjectTypeVisitor class.

The same can be reproduced in Microsoft.Azure.WebJobs.Extensions.OpenApi.FunctionApp.V3IoC project by adding a new class Owner:

       public class Owner
    {
        public string Name { get; set; }
        public List<Pet> Pets { get; set; }
    }

And introducing new property "public Owner Owner { get; set; }" in Pet class

@justinyoo justinyoo self-assigned this Feb 22, 2021
@justinyoo justinyoo removed their assignment Mar 5, 2021
@justinyoo justinyoo added bug Something isn't working and removed investigating labels Mar 5, 2021
@justinyoo justinyoo added resolved and removed bug Something isn't working labels Apr 2, 2021
@justinyoo
Copy link
Contributor

@Frankyfrankly Thanks for the issue. I looked at the issue, but if you change the models' structure it will solve your issue:

public class Book
{
    ...
    public string AuthorId { get; set; }
}

public class Author
{
    ...
    public List<Book> Books { get; set; }
}

To me, the circular reference issue is about how data structure looks like, rather than the issue of this extension. If you still want to keep the Author in your Book class, I would recommend adding the [OpenApiIgnore()] decorator on top of it like:

public class Book
{
    ...
    public string AuthorId { get; set; }

    [OpenApiIgnore()]
    public Author Author { get; set; }
}

By doing so, you will avoid the circular reference issue.

@justinyoo
Copy link
Contributor

@Frankyfrankly I'll close the issue for now. If you'd like to discuss more on this, please open a new issue.

@justinyoo justinyoo reopened this Apr 15, 2021
@justinyoo justinyoo added bug Something isn't working investigating and removed resolved labels Apr 15, 2021
@justinyoo
Copy link
Contributor

Related to: aliencube/AzureFunctions.Extensions#134

@vlaskal
Copy link

vlaskal commented Apr 17, 2021

Hi @justinyoo ,

I run into same problem as is described aliencube/AzureFunctions.Extensions#134
In my case I use health check library in Azure function project. And the response type HealthReport contains additional types including Exception type.
I guess this is root cause of a bug and circular reference.
As you see, these types cannot be changed as you suggest above.
Also I attached call stack when exception is thrown
image
I am open to help in investigation of the issue.

@john-patterson
Copy link

...

    [OpenApiIgnore()]
    public Author Author { get; set; }
}

By doing so, you will avoid the circular reference issue.

This is not working for me as OpenApiIgnoreAttibute seems to only be valid for AttributeTargets.Method.

@justinyoo
Copy link
Contributor

@john-patterson Oops my bad. I meant JsonIgnore.

@john-patterson
Copy link

@justinyoo ah. Okay, yeah I see in the code where that gets selected out. Unfortunately JsonIgnore isn't a great general solution as my other serializers handle my class just fine (my class Reviewer has an array of reviewers for whom they vote, based on the Azure DevOps type IdentityRefWithVote).

Is there opposition to a PR that generalizes OpenApiIgnore a bit to target properties and fields as well along with a patch to the RecursiveObjectTypeVisitor, ObjectTypeVisitor, and TypeExtensions? This should be backward compatible as JsonIgnore will still result in ignoring the attribute and OpenApiIgnore on methods will continue to behave the same.

If this works from a product management perspective, I can get a PR thrown together later today.

@justinyoo
Copy link
Contributor

@vlaskal Thanks for your suggestion! What I'm suspecting where the StackOverflowException or CircularReferenceException occurs is these two parts:

foreach (var schema in schemasToBeAdded.Where(p => p.Key != "jObject" && p.Key != "jToken"))
{
if (instance.RootSchemas.ContainsKey(schema.Key))
{
continue;
}
instance.RootSchemas.Add(schema.Key, schema.Value);
}

Adding the reference schemas should also be done before traversing each type's properties.

var instance = acceptor as OpenApiSchemaAcceptor;
if (instance.IsNullOrDefault())
{
return;
}
// Processes properties.
var properties = type.Value
.GetProperties(BindingFlags.Public | BindingFlags.Instance)
.Where(p => !p.ExistsCustomAttribute<JsonIgnoreAttribute>())
.ToDictionary(p => p.GetJsonPropertyName(namingStrategy), p => p);
this.ProcessProperties(instance, name, properties, namingStrategy);

It would be great if you can have a look. Once it's fixed, @john-patterson you wouldn't need to extend the OpenApiIgnore decorator.

@vlaskal
Copy link

vlaskal commented Apr 23, 2021

@justinyoo Thanks for hints. I will try too look at it and will reach you back.

@AndeyLapkoMobiDev
Copy link

I have the same issue and a possible solution in #467

@a-wallen
Copy link

I'm also getting this error without any luck using the IgnoreJson attribute on circular references.

@MehdiMohseni82
Copy link

This issue still exists, and I’m wondering why we can't configure the serialization depth in the OpenAPI configuration. Neither changing the model nor using JsonIgnore is a good solution, since in the API, based on the endpoint the consumer is calling, I want to have one or two layers of depth — but not more. For example, when the consumer calls get-book, the API should expose the author of the book but stop there and not include the books of the author. When the consumer calls get-author, the API should expose the books of the author but not the author of those books.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants