Skip to content
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

With this package installed, we cannot view the Umbraco Delivery API in Swagger #48

Closed
markadrake opened this issue Nov 20, 2024 · 6 comments · Fixed by #49
Closed

With this package installed, we cannot view the Umbraco Delivery API in Swagger #48

markadrake opened this issue Nov 20, 2024 · 6 comments · Fixed by #49
Labels
bug Something isn't working

Comments

@markadrake
Copy link

markadrake commented Nov 20, 2024

Which version of the package are you using?

13.1.0

Which Umbraco version are you using? For example: 12.2.0 - don't just write v12

13.5.2

Bug summary

If this package is installed, we cannot inspect the Umbraco Delivery API definition, but we can access other definitions in the browser.

Definitions:

  • Default API works.
  • Umbraco Delivery API does not work.
  • Umbraco Forms API works.
Swashbuckle.AspNetCore.SwaggerGen.SwaggerGeneratorException: Failed to generate Operation for action - Umbraco.Cms.Api.Delivery.Controllers.Content.QueryContentApiController.Query (Umbraco.Cms.Api.Delivery). See inner exception
 ---> Swashbuckle.AspNetCore.SwaggerGen.SwaggerGeneratorException: Failed to generate schema for type - Umbraco.Cms.Api.Common.ViewModels.Pagination.PagedViewModel1[Umbraco.Cms.Core.Models.DeliveryApi.IApiContentResponse]. See inner exception
 ---> System.InvalidOperationException: Can't use schemaId "$TokenType" for type "$System.Reflection.TypeNameParser+TokenType". The same schemaId is already used for type "$System.TokenType"
   at Swashbuckle.AspNetCore.SwaggerGen.SchemaRepository.RegisterType(Type type, String schemaId)
...

image

image

Steps to reproduce

I'm sorry, but I cannot outline the steps to reproduce this error. However, I'm happy to follow any steps you give me to provide more detail and insight into the situation. Although I'm not a backend engineer, I would gladly follow your direction to help fix the issue.

Expected result / actual result

No response

@markadrake markadrake added the bug Something isn't working label Nov 20, 2024
@lauraneto
Copy link
Contributor

Hi @markadrake ,
Could you provide me with the list of packages that you have installed?
And, do you have any custom data types of your own that might be dealing with types?
It's a bit weird that the code is trying to process those types. 🤔

@markadrake
Copy link
Author

@lauraneto, thank you for your reply!

Packages I have installed:

It is not a complete list; here are the ones that stand out from any typical installation.

Humble.Umbraco.CustomBackofficeIcons
Our.Umbraco.Community.Contentment
Our.Umbraco.GMaps
Our.Umbraco.TheDashboard
Umbraco.Cloud (Cms, Cms.PublicAccess, Identity, StorageProviders)
Umbraco.UIBuilder

Forgive me if I overlook something! I don't believe any of my custom property editors or extensions to Contentment are using anything non-standard. It's always a basic string value coming back. I have a few custom API endpoints, but they render correctly in Swagger. I will look closer!

Because of the upcoming holiday, things are sporadic here. I do appreciate your time and effort on the package. Please let me know if there are any steps you'd recommend to get more details.

@lauraneto lauraneto linked a pull request Nov 30, 2024 that will close this issue
@vsilvar
Copy link
Contributor

vsilvar commented Nov 30, 2024

Hi @markadrake,
We've done some improvements to ensure that any errors while generating the schema for a property are simply logged and don't cause the whole swagger to not be generated.

Can you give version 13.1.1-beta.1 a try and check the logs so we know exactly what property is causing the issue?
We can then also try to reproduce the issue with the affected property and fix it if possible or report it to the package maintainer.

@markadrake
Copy link
Author

@vsilvar I am so sorry for the delay.

Here is my environment without the package:

  • I can view the Umbraco Delivery API page.
  • I have ~10 Schemas.

image
image

Here are things after installing the beta:

  • Initially, I'm off to a better start because I can load the Umbraco Delivery API page.
  • Attempting to expand any of the endpoints crashes the browser. (The latest version of Brave, and I have privacy mode off for localhost.)
  • A TON of new Schemas.

image
image
image
image

I'll have more time this Friday or weekend. I want to test the DeliveryApiExtensions package alongside my Umbraco 13 minimal demo. I created this after noticing issues with Contentment and the Content Delivery API,, which is also used on this site from which I'm testing/reporting.

@vsilvar
Copy link
Contributor

vsilvar commented Dec 3, 2024

@markadrake
The issue with the browser crashing with too many schemas (#30) is not something we can fix.
It's an issue with the Swagger UI implementation which is just not performant enough.
As mentioned in the issue above, if you don't need the polymorphism options, you might want to try using Compatibility mode instead.
Or consider using alternative UI's like Scalar or ReDoc which don't have the issue.

As for the main problem that is causing a LOT of schemas to be generated, that is indeed due to Contentment not properly supporting the delivery API, and their property converters not implementing IDeliveryApiPropertyValueConverter.
This means that instead of returning a serializable model, it defaults to returning the normal models builder model, which has both cycles and just a lot of properties you wouldn't want to output in an API.
This causes both your serialization issues and also the issues you are reporting here.

Ideally Contentment would be updated so their property editors and converters support the delivery API and also so that the IDataSourceValueConverter interface would be extended to allow consumers to define both a method to return a modelsBuilder model but also a new one to allow you to return an alternative delivery API model.

In the mean time, I've provided you an alternative solution, by copying and overriding the built-in contentment value converters with a custom one modified to support the delivery api in:
markadrake/contentment-demo-issue#1

@markadrake
Copy link
Author

markadrake commented Dec 3, 2024

@vsilvar, you're a godsend! I appreciate all your effort on the bug report and your going above and beyond with the PR. Let me know if I can buy you a coffee (remotely)!

I trust your assessment. This issue derives from issues with Contentment, not your package.

I will close this issue.

Edit: and thank you, @lauraneto!

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.

3 participants