Add Open API document for OSB API #389

n3wscott · 2017-12-02T00:00:14Z

Relates to #160

Ready.

cfdreddbot · 2017-12-02T00:00:15Z

Hey n3wscott!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

…/headers path.

rhodie27

When validating I get:

Errors
Schema error at components.parameters['APIVersion']
should NOT have additional properties
additionalProperty: default, schema
Jump to line 379
Schema error at components.parameters['APIVersion'].in
should be equal to one of the allowed values
allowedValues: path, query, cookie
Jump to line 381

Also, as an FYI, this would be a breaking change for service authors that currently use the 2.12 swagger spec to generate code since you changed parameters and objects slightly. I'm not (necessarily) saying that's a problem.

I'll have more time to review next week.

n3wscott · 2017-12-05T20:06:39Z

@rhodie27 Yes, there is an open bug with swagger editor not pulling in parameters from components correctly. swagger-api/swagger-ui#3976 swagger-api/swagger-ui#3558

But I think what is in this PR is correct based on the Open API spec. The swagger UI works correctly.

I will double check the spec... I think I am correct and the tools have not caught up to the new stuff in 3.0.0. https://swagger.io/docs/specification/components/

mattmcneeney · 2017-12-12T17:43:54Z

Rendered version: http://petstore.swagger.io/?url=https://raw.githubusercontent.com/n3wscott/servicebroker/9bc40f42ecaa3ab89fe632404a3eb56f15fb03b8/openapi.yaml

fmui · 2017-12-14T13:57:32Z

Here are some random nit-picking comments.

In the spec we several places with wording like this: "MUST be a non-empty string". "minLength: 1" should cover this.
In the schema definition Service -> properties -> requires, items should be an array of enums (syslog_drain, route_forwarding, volume_mount).
The schema of the status code '409' of 'PUT /v2/service_instances/{instance_id}' could be an Error, not an Object.
The status code '400' of 'PUT /v2/service_instances/{instance_id}/service_bindings/{binding_id}' is lacking a content definition.
The definitions of "route" and "*_url" properties and could be enhanced by "format: url".
The conventions for service and plan metadata are not in the spec, but we link to them from the spec. Should we do something similar here?

n3wscott · 2017-12-18T17:11:34Z

@fmui

In the spec we several places with wording like this: "MUST be a non-empty string". "minLength: 1" should cover this.

I would vote for no validation in the first version of the swagger spec. It might be useful to not have the validation in to allow folks to poke at bad brokers. If we still want validation, could it be a followup exercise?

The schema of the status code '409' of 'PUT /v2/service_instances/{instance_id}' could be an Error, not an Object.

The spec says MAY for an error and expected to be {}:

| 409 Conflict | MUST be returned if a Service Instance with the same id already exists but with different attributes. The expected response body is `{}`, though the description field MAY be used to return a user-facing error message, as described in [Service Broker Errors](#service-broker-errors).|

The conventions for service and plan metadata are not in the spec, but we link to them from the spec. Should we do something similar here?

I opted to only use the spec.

I have updated based on feedback, adding the missing return types and enums, but I am hesitating to add more validation to the doc just because it is hard to know when to stop. I think it could be a valuable addition to the Open API document, but it is still possible to do as a followup and we could do it one resources

avade · 2017-12-19T11:06:07Z

openapi.yaml

+            schema:
+              $ref: '#/components/schemas/ServiceInstanceUpdateRequest'
+      responses:
+        '200': 


This is missing the operation field that is valid in the response of PATCH on Service Instances

ah yes, true. Ref: https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#body-5

Looking at the spec, for PATCH it says this:

200 --> MUST be returned if the request's changes have been applied. The expected response body is {}.

So the spec does not say the operation field is allowed.

@n3wscott I agree; operation shouldn't be returned here as a 200 implies the update has completed synchronously.

avade · 2017-12-19T11:06:46Z

openapi.yaml

+            type: boolean
+      responses:
+        '200':
+          description: OK


This is missing the operation field that is valid in the response of DELETE on Service Instances

yes yes: https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#body-9

fmui · 2017-12-19T12:13:12Z

So, what's the meaning of this excerpt:

| 409 Conflict | MUST be returned if a Service Instance with the same id already exists but with different attributes. The expected response body is {}, though the description field MAY be used to return a user-facing error message, as described in Service Broker Errors.|

You can add a description, but you better shouldn't. The platform doesn't expect it.
This a broker error, but you don't have to provide a description.

n3wscott · 2017-12-20T00:02:19Z

@fmui it means that 409 must at least return {} and optionally return a service broker error.

n3wscott · 2017-12-20T00:06:18Z

@avade thanks for the review! I will fix those missing return params up when I next get a chance.

n3wscott

Added AsyncOperation. @avade please remove conflict label.

n3wscott · 2017-12-20T00:27:09Z

openapi.yaml

+            type: boolean
+      responses:
+        '200':
+          description: OK


n3wscott · 2017-12-20T00:27:36Z

openapi.yaml

+            schema:
+              $ref: '#/components/schemas/ServiceInstanceUpdateRequest'
+      responses:
+        '200': 


n3wscott

Thanks a lot @mattmcneeney for the review!

Check out the new version with the updates here: Live

n3wscott · 2018-01-04T17:11:22Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/AsyncOperation'


This was a mistake, AsyncOperation was suppose to be used for only the update, not the create.

n3wscott · 2018-01-04T17:12:47Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/ServiceInstanceProvision'


This is true. My argument is it might not hurt anything to allow an optional field be defined in the Open API doc for the 201 case. the 200 and 202 cases are the same object.

n3wscott · 2018-01-04T17:13:36Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/ServiceInstanceProvision'


The 200 could apply to the sync or async cases.

n3wscott · 2018-01-04T17:20:17Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Object'


This is what the spec says:

400 -> MUST be returned if the request is malformed or missing mandatory data.

And then

Service Brokers can include a user-facing message in the description field;

I struggled with this TBH, the spec says some responses should {} some should be in the { "error":"" , "description":""} form and some it does not say. The line below the table says the quote above but it is not clear is that applies to all or some.

So I opted to define the response as Object at least, and where the spec calls it out, define the response as Error.

Perhaps this is something we update after the doc has some usage if it is not working for folks or is unclear.

n3wscott · 2018-01-04T17:25:21Z

openapi.yaml

+            schema:
+              $ref: '#/components/schemas/ServiceInstanceUpdateRequest'
+      responses:
+        '200': 


Looking at the spec, for PATCH it says this:

200 --> MUST be returned if the request's changes have been applied. The expected response body is {}.

So the spec does not say the operation field is allowed.

n3wscott · 2018-01-04T17:38:07Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Object'


I made an issue to clear this up.

n3wscott · 2018-01-04T17:40:03Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'


This is not how I read the spec.

I made an issue to clear this up.

The Error object has both fields optional.

n3wscott · 2018-01-04T17:50:28Z

openapi.yaml

+              - volume_mount
+        bindable:
+          type: boolean
+        metadata:


Not a bad idea. Added.

n3wscott · 2018-01-04T17:51:46Z

openapi.yaml

+        - service_id
+      properties:
+        context:
+          $ref: '#/components/schemas/Object'


n3wscott · 2018-01-04T17:52:27Z

openapi.yaml

+        - plan_id
+      properties:
+        context:
+          $ref: '#/components/schemas/Object'


mattmcneeney

Thanks for the speedy turnaround @n3wscott 👏

mattmcneeney · 2018-01-05T09:20:19Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/ServiceInstanceProvision'


Ah. I hadn't considered returning the id of a completed async operation (when a service instance has successfully been provisioned async).

mattmcneeney · 2018-01-05T09:21:47Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/ServiceInstanceProvision'


Yeah, but that could make platform authors write logic to handle polling when they get a 201 which should never be used.

mattmcneeney · 2018-01-05T09:24:57Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/AsyncOperation'


mattmcneeney · 2018-01-05T09:33:44Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Object'


Sounds good. If we can use this PR as an opportunity to clear up ambiguities in the spec, then I'd love for any error to be able to return the defined error object :)

mattmcneeney · 2018-01-05T09:36:05Z

openapi.yaml

+            schema:
+              $ref: '#/components/schemas/ServiceInstanceUpdateRequest'
+      responses:
+        '200': 


@n3wscott I agree; operation shouldn't be returned here as a 200 implies the update has completed synchronously.

mattmcneeney · 2018-01-05T09:39:03Z

openapi.yaml

+            application/json:
+              schema:
+                $ref: '#/components/schemas/AsyncOperation'
+        '400': 


Awesome, thanks!

mattmcneeney · 2018-01-05T09:39:25Z

openapi.yaml

+            application/json:
+              schema:
+                $ref: '#/components/schemas/AsyncOperation'
+        '400':


mattmcneeney · 2018-01-05T09:39:41Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Object'


mattmcneeney · 2018-01-05T09:40:51Z

openapi.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Object'


Tracking in #403

mattmcneeney · 2018-01-05T09:41:41Z

openapi.yaml

+        plan_id:
+          type: string
+        context:
+          $ref: '#/components/schemas/Object'


Did you spot this one too @n3wscott ?

luksa · 2018-01-05T10:30:55Z

openapi.yaml

+          items:
+            $ref: '#/components/schemas/Service'
+
+    Service:


I believe this and all others need to include the type: object attribute, otherwise you're saying they can also be arrays or primitives. At least that's how it was in OpenAPI 2.0.

Nice catch!

luksa · 2018-01-05T10:41:29Z

openapi.yaml

+# This is the Open API interface for Open Service Broker API. Every attempt will
+# be made to make the Open API version of OSB API accurately represent the
+# written specification. If the spec and this document conflict, the spec is
+# the authroity.


s/authroity/authority/

avade · 2018-01-11T11:16:00Z

I propose that this is now merged and further refinements are new PRs

LGTM

angarg12 · 2018-01-11T14:37:04Z

Would it make sense to put a link to the rendered swagger on the repo README so that it is easily accessible by people?

angarg12 · 2018-01-11T14:54:18Z

LGTM

n3wscott · 2018-01-11T22:47:22Z

Would it make sense to put a link to the rendered swagger on the repo README so that it is easily accessible by people?

Sounds like a good follow-up. Maybe even a symlink to a git tag when next release happens.

duglin · 2018-01-12T15:29:24Z

openapi.yaml

+paths:
+  /v2/catalog:
+    get:
+      summary: get the catalog of services that the service broker offers


Can we start these with capital letters? Get the catalog...
It looks better to me when generated.

We can in a follow-up!

duglin · 2018-01-12T15:33:32Z

I'm new to OAPI but is there a way in the "Example Value" that it generates to have it do something like add a "?" when a field is optional? Showing that example is really helpful, but I'm missing the required/optional aspect of each field.

duglin · 2018-01-12T15:36:59Z

@maximilien ^^

n3wscott · 2018-01-12T15:51:21Z

is there a way in the "Example Value" that it generates to have it do something like add a "?" when a field is optional

@duglin This is a quirk of the default swagger ui, click the tab next to "example value" called "Model" and you will see the object in table format with the required fields with descriptions if the model has them.

duglin · 2018-01-12T16:01:34Z

Ah, I didn't realize the "Model" thing was a link - nice

vaikas · 2018-01-12T21:21:05Z

LGTM

duglin · 2018-01-13T00:37:58Z

@n3wscott did you want to fix the two spots @mattmcneeney commented on?

n3wscott · 2018-01-15T23:55:55Z

did you want to fix the two spots @mattmcneeney commented on?

@duglin the error pr is not in yet I think

duglin · 2018-01-16T00:11:32Z

Sorry, not sure what you mean by "error pr"

duglin · 2018-01-16T14:21:22Z

Matt are your issues resolved? github shows you're still requesting changes

mattmcneeney · 2018-01-16T16:24:21Z

Yes - but if we can get #409 merged then it'd be awesome to update the OpenAPI doc with that change before merging it in (as it makes many of the errors much simpler)

duglin · 2018-01-16T16:31:58Z

@n3wscott I think there are a few edits that people want to see before we merge this - any chance of getting those in?

mattmcneeney · 2018-01-16T17:55:33Z

LGTM

Adding first pass open api doc of the spec.

970624f

Scott Nichols added 9 commits December 4, 2017 09:56

Missing parameters field, fixed.

c0f7268

Headers in parameters seem to not be able to come from the components…

c8b3682

…/headers path.

No, headers in their own seciond should be correct.

42ce978

use link correctly for parameters that happen to also be headers.

70bfcdd

The spec does say to use an empty object for all reponses.

fc3d3df

adding missing required flag for deprovision.

aad3259

last op also had {} missing.

a23d870

Adding localhost:3000 for pointing to the osb checker mock server.

8d29073

Working.

9bc40f4

n3wscott changed the title ~~[WIP] Add Open API document for OSB API~~ Add Open API document for OSB API Dec 4, 2017

rhodie27 reviewed Dec 5, 2017

View reviewed changes

avade added the 1 - reviewing proposal label Dec 12, 2017

avade added this to the 2.14 milestone Dec 12, 2017

Adding to the contrib guide to mention updating the openapi document.

aadd4b0

Updated based on feedback, thanks

dbe79cf

avade suggested changes Dec 19, 2017

View reviewed changes

avade added the needs rebase/work label Dec 19, 2017

Scott Nichols added 2 commits December 19, 2017 16:26

Adding AsyncOperation def.

21c26ba

Adding AsyncOperation def.

a2e2c6c

n3wscott commented Dec 20, 2017

View reviewed changes

n3wscott commented Jan 4, 2018

View reviewed changes

mattmcneeney mentioned this pull request Jan 5, 2018

The Spec is unclear about what errors should look like. #403

Closed

mattmcneeney reviewed Jan 5, 2018

View reviewed changes

mattmcneeney mentioned this pull request Jan 5, 2018

Own the swagger definition for the Service Broker API #160

Closed

luksa reviewed Jan 5, 2018

View reviewed changes

duglin added the needs rebase/work label Jan 9, 2018

Add type: object to be correct.

de20117

duglin removed the needs rebase/work label Jan 10, 2018

duglin reviewed Jan 12, 2018

View reviewed changes

angarg12 merged commit 7f22373 into openservicebrokerapi:master Jan 16, 2018

mattmcneeney mentioned this pull request Jan 16, 2018

No operation in OpenAPI synchronous service instance provision response #414

Merged

n3wscott deleted the openapi branch April 2, 2018 15:21

Add Open API document for OSB API #389

Add Open API document for OSB API #389

Conversation

n3wscott commented Dec 2, 2017 • edited Loading

cfdreddbot commented Dec 2, 2017

rhodie27 left a comment

Choose a reason for hiding this comment

n3wscott commented Dec 5, 2017

mattmcneeney commented Dec 12, 2017

fmui commented Dec 14, 2017

n3wscott commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmui commented Dec 19, 2017

n3wscott commented Dec 20, 2017

n3wscott commented Dec 20, 2017

n3wscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3wscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmcneeney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avade commented Jan 11, 2018 • edited by chipchilders Loading

angarg12 commented Jan 11, 2018

angarg12 commented Jan 11, 2018 • edited by chipchilders Loading

n3wscott commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Jan 12, 2018

duglin commented Jan 12, 2018

n3wscott commented Jan 12, 2018 • edited Loading

duglin commented Jan 12, 2018

vaikas commented Jan 12, 2018 • edited by avade Loading

duglin commented Jan 13, 2018

n3wscott commented Jan 15, 2018

duglin commented Jan 16, 2018

duglin commented Jan 16, 2018

mattmcneeney commented Jan 16, 2018

duglin commented Jan 16, 2018

mattmcneeney commented Jan 16, 2018 • edited by avade Loading

n3wscott commented Dec 2, 2017 •

edited

Loading

avade commented Jan 11, 2018 •

edited by chipchilders

Loading

angarg12 commented Jan 11, 2018 •

edited by chipchilders

Loading

n3wscott commented Jan 12, 2018 •

edited

Loading

vaikas commented Jan 12, 2018 •

edited by avade

Loading

mattmcneeney commented Jan 16, 2018 •

edited by avade

Loading