khrt / Raisin

Raisin - a REST API micro framework for Perl 🐫 🐪
61 stars 29 forks source link

Swagger Schema Expected Response and Actual Response Do Not Match #74

Open coldfission opened 4 years ago

coldfission commented 4 years ago

I've noticed with the music-app example, the response is contained within an object with key "data". But, when interpreting the Swagger Schema ( visually and with swagger-ui tool ), there is no hint that the top-level object with key "data" should ever exist. See below image for swagger-ui schema showing the mismatch in expected schema response data structure and actual server response data structure. Do you have any ideas on how to make schema-expected and actual responses match when using Entities? I have some rough ideas on how to solve this with a change to Raisin source, but could potentially introduce breaking-changes for others. Here is one idea:

Replace Line 57 in Raisin::Middleware::Formatter with:
if ( defined $r->body->{data} and ref $r->body->{data} ) { $r->body($s->serialize($r->body->{data})); } else { $r->body($s->serialize($r->body)); }

Screen Shot 2019-08-01 at 3 18 56 PM
khrt commented 4 years ago

Hi @coldfission, there is a bug in Swagger implementation.

If you take a look at https://github.com/khrt/Raisin/blob/master/lib/Raisin/API.pm#L196 and https://github.com/khrt/Raisin/blob/master/lib/Raisin/Plugin/Swagger.pm#L225 you'll see that Raisin knows nothing about the data key which wraps everything.

If it brakes production code and causes a serious issues on your side you can workaround it by creating a new entity which which will wrap everything with data.

Something like this:

expose 'data', desc => 'Base object', using => 'MusicApp::Entity::Artist';
coldfission commented 4 years ago

Thanks for your response @khrt . Thinking about this for quite a while now. There are two approaches that I can see:

I'm leaning toward the first approach with the optional api_format_startkey config. What do you think? I can work on a Pull Request based on our decision.

khrt commented 4 years ago

First approach looks like a nice solution if you don't want to brake existing code and make it optional. And that should work fine until all your endpoints use data as a top level object. But if different endpoint have different top level object then it won't work with a globacl api_format_startkey.

Second approach looks more valid as it makes result match to reality, fixing a bug, but potentially is a braking change as there might be software relaying on existing behaviour.

In a long-term perspective I think second approach is more beneficial as it actually fixes a bug, while the first approach is mitigating a problem.

coldfission commented 4 years ago

should work fine until all your endpoints use data as a top level object

What is your reason for this? I see that JSON API Spec shows that all responses contain a top-level data object. But, there is no mention of this requirement in the OpenAPI Spec. Keep in mind that the Kubernetes OpenAPI Schema does not use a top-level data object. This is just one example of a popular OpenAPI Schema implementation.

khrt commented 4 years ago

What is your reason for this?

My reason for this, is that as long as you're free to return any data back api_format_startkey won't reflect reality.

Imagine you define:

api_format_startkey 'data';

get '/one' => { { 'data' => { .. } };
get  '/two' => { 'result' => { .. } };

But, there is no mention of this requirement in the OpenAPI Spec. Keep in mind that the Kubernetes OpenAPI Schema does not use a top-level data object. This is just one example of a popular OpenAPI Schema implementation.

I believe the reason all those from the above have success responses wrapped with data is because they are good guys which are compliant to JSON API and it has no connection to OpenAPI.