search

Azure / data-api-builder

Data API builder provides modern REST and GraphQL endpoints to your Azure Databases and on-prem stores.
https://aka.ms/dab/docs
MIT License
786 stars 142 forks source link

Support for OpenAPI description in a Infragistics AppBuilder integration use case #2212

Open chrohrbach opened 1 month ago

chrohrbach commented 1 month ago

What happened?

It seems that DAB does not generate enough information when an endpoint is called by Infragistics AppBuilder that requires some details to be able to use the endpoint correctly, see the description here. I searched for possibilities to configure the expressivness of DABs openapi interface but did not find any information about such a possibility. In a Infragistics forum post, see here, the problem is described and solved with access to the underlying endpoint source code, i don't feel confident to go to the source code of DAB to check this. Thanks for your time in helping me out with this topic.

Version

latest

What database are you using?

Azure SQL

What hosting model are you using?

Local (including CLI)

Which API approach are you accessing DAB through?

REST

Relevant log output

No response

Code of Conduct

chrohrbach commented 1 month ago

It has been determined that Infragistics AppBuilder is looking for a missing property in the schema, see below image It is unclear if this is required by the OpenAPI specs or if it is a miss interpretation by Infragistics.

pmoleri commented 1 month ago

Here's the code that creates the Schema with "properties" but no "type". https://github.com/Azure/data-api-builder/blob/466c18a9c2eb9ccc808020319749f7681b13cfed/src/Core/Services/OpenAPI/OpenApiDocumentor.cs#L938-L941

pmoleri commented 1 month ago

I think this needs to be considered as a bug. The lack of the "type" permits any other type as valid: image

The mere presence of "properties" is not enough to specify type object.

chrohrbach commented 1 month ago

Thanks make sense. How do we go forward from here ?

seantleonard commented 1 month ago

Thank you for reporting, @chrohrbach. I'm working on a fix.

I found a relevant thread that discusses whether type is a required property. Consensus is that type isn't required:

Ultimately, different tooling handles the presence of the type property differently. Some may try to guess the type when not present:

That said, there is no reason why DAB shouldn't add this so that Infragistics tooling can work. I'm curious whether this is the only error that Infragistics emits when evaluating the OpenAPI document generated by DAB.

chrohrbach commented 1 month ago

Thx for you work

pmoleri commented 1 month ago

Hi @seantleonard ,

Thank you for picking up this. While it's true that type is not required, I think it's incorrect in the context of DAB response.

DAB response is always an object and it's best described as such as it will improve compatibility. I'm glad you agreed, I just wanted to provide additional input for the decision. Thanks again

seantleonard commented 1 month ago

Hi @seantleonard ,

Thank you for picking up this. While it's true that type is not required, I think it's incorrect in the context of DAB response.

  • type + properties means "it must be an object and the properties are as described
  • properties only means "it can be any type, when it's an object the properties are as described (see my validation example with a plain integer in the previous post).

DAB response is always an object and it's best described as such as it will improve compatibility. I'm glad you agreed, I just wanted to provide additional input for the decision. Thanks again

Sorry to imply that I was trying to justify whether to do this or not. I fully intend to do so.

I intended to figure out whether type was required because I wanted to identify why https://github.com/microsoft/OpenAPI.NET didn't complain about missing type. An error if type were required would have helped prevent this becoming an issue in the first place. Thanks for bringing this to my attention.

JerryNixon commented 2 weeks ago

Related #2262