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

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

Open
coldfission opened this issue Aug 1, 2019 · 5 comments
Open

Comments

@coldfission
Copy link

coldfission commented Aug 1, 2019

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
Copy link
Owner

khrt commented Aug 20, 2019

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
Copy link
Author

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

  • Modify Raisin::Middleware::Formatter to detect data key and start encoding at that key's value. To avoid breaking-changes for other implementation, we could make this functionality optional by introducing an app config api_format_startkey ( ex: api_format_startkey 'data'; )
  • Modify Raisin::Plugin::Swagger to specify that data key and value is expected in the response

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
Copy link
Owner

khrt commented Aug 29, 2019

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
Copy link
Author

coldfission commented Aug 29, 2019

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
Copy link
Owner

khrt commented Aug 30, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants