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

JSON Body Editor (Master PR) #1589

Merged
merged 33 commits into from
Dec 16, 2015
Merged

Conversation

lepinayl
Copy link
Contributor

Feature proposal: JSON Body Editor

Previous history

This PR is a continuation of #1088
You will find earlier informations/comments there about the history of this PR.

Related work

This is a proposal to add a JSON editor for body requests in swagger UI.

This work is based on another open source project called Json-editor by Jeremy Dorn.

You will be able to find a demo of Jeremy's editor here to give you an idea of the great potential of this editor.

Configuration

The editor can be enabled/disabled using the swagger configuration key jsonEditor :

      window.swaggerUi = new SwaggerUi({
        jsonEditor:true
      });

The option is currently enabled by default (just because it is more practical for e2e tests).

Visual preview

This is how it looks like at the moment in a post request:
capture

Online preview

You can try the UI with the editor here: http://thomsonreuters.github.io/swagger-ui/
you will need to have CORS enabled on your endpoint

Backward compatibility

According to the way swagger now works, Specs 1.2 are now automaticaly converted to 2.0.

So it should work with 1.2 as well but I haven't not tested it yet.

Current state of the implementation

Please let me know how I could improve both the internal design (code, unit tests, configuration) and external design (css, user experience).

External dependencies

The following PR have been created on JSONEditor to implement this feature
jdorn/json-editor#461 (closed)
jdorn/json-editor#456 (open, need some rework)
jdorn/json-editor#512 (merged)

So at the moment this PR is using a custom JSONEditor version until both remaining PR will be accepted.

Tests

Tested on
http://petstore.swagger.io/v2/swagger.json
https://raw.githubusercontent.com/jabr9983/swagger.json/master/swagger.json

Not tested yet
http://ringcentral.github.io/api-explorer-beta/swagger-ring.json
https://api.swaggerhub.com/apis/fehguy/ringcentral-api/1.0

lepinayl and others added 20 commits March 9, 2015 05:37
Conflicts:
	dist/index.html
	dist/swagger-ui.js
	dist/swagger-ui.min.js
	src/main/coffeescript/view/MainView.coffee
	src/main/coffeescript/view/OperationView.coffee
	src/main/coffeescript/view/ParameterView.coffee
	src/main/coffeescript/view/ResourceView.coffee
	src/main/coffeescript/view/SignatureView.coffee
	src/main/html/index.html
Conflicts:
	dist/index.html
	dist/swagger-ui.js
	dist/swagger-ui.min.js
Conflicts:
	dist/swagger-ui.js
	dist/swagger-ui.min.js
…ditor

Conflicts:
	dist/swagger-ui.js
	dist/swagger-ui.min.js
	package.json
Conflicts:
	dist/swagger-ui.js
	dist/swagger-ui.min.js
Conflicts:
	dist/index.html
	dist/swagger-ui.js
	dist/swagger-ui.min.js
	src/main/html/index.html
	src/main/template/param.handlebars
	src/main/template/param_required.handlebars
Fix $ref strings treated as objects by JSONEditor
Conflicts:
	dist/swagger-ui.js
	dist/swagger-ui.min.js
	src/main/less/screen.less
@lepinayl lepinayl mentioned this pull request Sep 10, 2015
@webron
Copy link
Contributor

webron commented Sep 10, 2015

Thanks again for all the work. I'll test the functionality in the upcoming days and hopefully we'll manage to get it merged soon. I'm sure many users would be excited for it.

If any user wants to clone the fork and test the PR and provide feedback, that would be great too.

@jontonsoup
Copy link

@lepinayl Thanks for this PR! It looks really awesome.

I have a question about the functionality-- is the editor supposed to serialize automatically?

For example:

The user field is incorrectly set up when I load the page (it should be an object):

screen shot 2015-09-11 at 10 55 38 pm

But when I choose "object" it is serialized properly. It seems to me that the "user" field should be pre chosen as an object (maybe I shouldn't even have an option to send it as a boolean?)

screen shot 2015-09-11 at 10 56 24 pm

Its possible my swagger definition has an error in it, but this seems like confusing behavior to me.

Best,
Jon

@lepinay
Copy link

lepinay commented Sep 14, 2015

@jontonsoup thank you for the testing feedback, it would be great if you could share your json spec so that I can have a look to the issue because you are right it should serialize automatically, I haven't encountered this behaviour yet.

@jontonsoup
Copy link

I tried to clean it up to the relevant parts. Please let me know if its missing anything. @lepinay

paths:
  /orders:        
    post:
      tags:
        - order
      summary: Create a new order object
      description: ""
      operationId: create_order
      consumes:
        - application/json
      produces:
        - application/json
      parameters:
        - in: body
          name: order
          description: Information needed to create an order.
          required: true
          schema:
            $ref: "#/definitions/OrderRequest"
      responses:
        "200":
          description: successful operation
          schema:
            $ref: "#/definitions/OrderResponse"
        "400":
          description: Invalid input

      security:
        - api_key: []

definitions:
  User:
    required:
      - first_name
      - last_name
    properties:
      first_name:
        type: string
      last_name:
        type: string
      phone:
        type: string
  OrderRequest:
    required:
      - user
      - address
    properties:
      user:
        $ref: "#/definitions/User"
      address:
        $ref: "#/definitions/Address"
      location:
        $ref: "#/definitions/Location"
  OrderResponse:
    required:
      - user
      - address
      - status
    properties:
      id:
        type: integer
        format: int64
      user:
        $ref: "#/definitions/User"
      address:
        $ref: "#/definitions/Address"
      location:
        $ref: "#/definitions/Location"
      price:
       type: number
      status:
        type: string
        enum:
         - "address_submitted"
         - "in_progress"
         - "completed"
        description: Order Status

  Location:
    type: array
    items:
      type: number

  Address:
    required:
      - street
      - city
      - state
      - zipcode
    properties:
      street:
        type: string
        description: Street name with number
      apartment_number:
        type: string
        description: an apartment number if there is one
      city:
        type: string
        description: Address City
      state:
        type: string
        description: Address State
      zipcode:
        type: integer
        maxLength: 5
        minLength: 5
        description: zipcode

@lepinay
Copy link

lepinay commented Sep 16, 2015

@jontonsoup this was indeed a regression, I've tested it on your schema and it should be fixed now.

@jontonsoup
Copy link

Awesome! This is a great feature-- thanks for adding it.

On Wed, Sep 16, 2015 at 5:00 AM, foo bar code notifications@github.com
wrote:

@jontonsoup this was indeed a regression, I've tested it on your schema and it should be fixed now.

Reply to this email directly or view it on GitHub:
#1589 (comment)

@jontonsoup
Copy link

@lepinay I actually just checked out the new branch and I still have the error.

I'm wondering if it has something to do with my data type being undefined.

screen shot 2015-09-16 at 4 47 31 pm

Best,
Jon

Conflicts:
	dist/swagger-ui.js
	dist/swagger-ui.min.js
@lepinayl
Copy link
Contributor Author

@frol Thanks for pointing out the images issue, it's fixed now.

@frol
Copy link

frol commented Nov 18, 2015

@lepinayl I've been using your fork for the last 3 days and it has been working great even on semi-complex nested schemas! Great job!

I have spotted only one minor bug. JSON Editor ignores validation of required fields and sends empty string values instead of preventing sending the data. Here is my endpoint config:

"post": {
    "operationId": "post_users",
    "parameters": [
        {
            "in": "body",
            "name": "body",
            "required": true,
            "schema": {
                "properties": {
                    "username": {
                        "description": "Example: root",
                        "location": "json",
                        "type": "string"
                    },
                    "password": {
                        "description": "No rules yet",
                        "location": "json",
                        "type": "string"
                    }
                },
                "required": [
                    "username",
                    "password"
                ]
            }
        }
    ]
}

P.S. It would be nice if JSON Editor might use free space of "Description" column to be wider.

@lepinayl
Copy link
Contributor Author

@frol Thanks for the kind feedback, I'll look into that issue.

@frol
Copy link

frol commented Nov 22, 2015

@lepinayl I caught one more minor bug - re-adding (add -> remove -> add) a new item to a list results in ignoring (not-showing) non-required fields (see value field on the screenshot):

screenshot from 2015-11-22 18-33-36

Here is my endpoint specification:

"patch": {
    "operationId": "patch_user_by_id",
    "parameters": [
        {
            "in": "path",
            "name": "user_id",
            "required": true,
            "type": "integer"
        },
        {
            "in": "body",
            "name": "body",
            "required": true,
            "schema": {
                "items": {
                    "properties": {
                        "op": {
                            "enum": [
                                "replace",
                                "remove",
                                "test",
                                "add"
                            ],
                            "location": "json",
                            "type": "string"
                        },
                        "path": {
                            "enum": [
                                "/password",
                                "/email",
                            ],
                            "location": "json",
                            "type": "string"
                        },
                        "value": {
                            "location": "json",
                            "type": "string"
                        }
                    },
                    "required": [
                        "op",
                        "path"
                    ]
                },
                "location": "json",
                "type": "array"
            }
        }
    ]
}

@agongdai
Copy link

@lepinayl There is a minor UI bug after "Delete last item" of an array: the "Move Down" button of the last item remains.

The steps to reproduce: "Add item" to "Item 3" -> "Delete Last item".

2015-11-24_105751

@grokify
Copy link

grokify commented Dec 16, 2015

Here's an updated implementation of the JSON Body Editor with the RingCentral API Spec:

  1. http://ringcentral.github.io/api-explorer-beta/
  2. http://ringcentral.github.io/api-explorer-beta/#!/Messages/post_v1_0_account_accountId_extension_extensionId_sms

We found that our property descriptions were long enough that they used too much vertical space so we've implemented an initial approach to convert these descriptions to tooltips.

Conflicts:
	dist/swagger-ui.js
	dist/swagger-ui.min.js
	src/main/javascript/view/OperationView.js
	src/main/less/screen.less
@lepinay
Copy link

lepinay commented Dec 16, 2015

@agongdai I've been trying to reproduce the issue on http://thomsonreuters.github.io/swagger-ui/ but with no success, could you double check if you have the issue there ?

@lepinay
Copy link

lepinay commented Dec 16, 2015

@grokify I like the idea of the tooltip, if you have time to make a PR on https://github.com/thomsonreuters/swagger-ui/tree/JSONEditorMaster with a feature toggle in swagger options (some people might prefer to use the current version) then I will merge it.

Update: If you are up to it, you can now make you PR directly into swagger's master

@frol
Copy link

frol commented Dec 16, 2015

@lepinay I have easily reproduced the issue reported by @agongdai. Note that the issue can be reproduced only with "Delete Last Item" button, while it works as expected when using regular "Delete Item" button.

@lepinay
Copy link

lepinay commented Dec 16, 2015

@frol Thanks got it now

@fehguy
Copy link
Contributor

fehguy commented Dec 16, 2015

First off, thank you again for keeping this PR going. The delay merging has been on our side.

Next, I'm setting the editor to off by default, and we'll update the documentation.

Finally, it's getting merged into master now. Thanks again!

@fehguy fehguy merged commit 2c431d7 into swagger-api:master Dec 16, 2015
@lepinay
Copy link

lepinay commented Dec 16, 2015

@fehguy Thanks for the merge, it will definitively help other interested into that feature to contribute and help manage bug fixes more efficiently. I'll now focus on getting the pending PR I have at jdorn/json-editor to be merged so that we can rely on the official version instead of the custom built one coming with this PR.

@fehguy
Copy link
Contributor

fehguy commented Dec 16, 2015

Yes, again many thanks for the patience, and apologies for the delay on merging. I did default it to false in the index.html for now, which is breaking the tests, but that will be fixed soon.

@grokify
Copy link

grokify commented Dec 20, 2015

@lepinay Thanks for the suggestion for a tooltip PR. I think it's a good idea and would like to do that. First, there's a dependency on getting a PR into jdorn/json-editor. Once that's been done we can work on a PR for the JSON body editor.

@fehguy fehguy modified the milestone: v2.1.4 Jan 6, 2016
@lucasweb78
Copy link

Would it be possible to be able to toggle between the json-editor and text field, rather either having to use one or the other? In some cases the editor is simpler to use and in others the text field is easier.

@webron
Copy link
Contributor

webron commented Apr 4, 2016

@lucasweb78 It's hard trying to make everything configurable at that level. This is not only about code, but about UX. Having you and others wanting this feature doesn't mean others would want it as well. However, this being an open source project with a permissive license allows you to customize the code to your own needs. We're not offended by it, we even encourage you to do so.

@atdiff
Copy link

atdiff commented Apr 22, 2016

With this update, is the payload now getting validated within the editor as well? It'd be cool if it compared the JSON payload returned to the Swagger schema.

@lepinay
Copy link

lepinay commented Apr 25, 2016

@atdiffthis unfortunately no...

@atdiff
Copy link

atdiff commented Apr 25, 2016

Bummer. Thanks @lepinay. Do you know if there are plans to integrate that as an option at some point? I might log it as an issue if not.

@lepinay
Copy link

lepinay commented Apr 26, 2016

@atdiff No there are no plans to share functionality between the two. I think it might be a good idea to raise that as a separated issue for the standard editor only.

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

Successfully merging this pull request may close these issues.

None yet