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

Code assist for format keyword inserts empty quotes #46

Closed
tedepstein opened this issue Dec 17, 2015 · 14 comments
Closed

Code assist for format keyword inserts empty quotes #46

tedepstein opened this issue Dec 17, 2015 · 14 comments

Comments

@tedepstein
Copy link
Collaborator

The format key allows certain values according to the Swagger specification, and I expected to see these values from code assist. But it just inserted blank quotes, which is not so useful.

@tedepstein tedepstein added this to the Release 1.0 milestone Jan 7, 2016
@tfesenko
Copy link
Member

tfesenko commented Jan 7, 2016

Just verified it on the latest build - it's still there.

@ghillairet
Copy link
Member

The problem is that the json-schema for swagger does not provide any values for this property. See https://github.com/ModelSolv/SwagEdit/blob/master/com.reprezen.swagedit/src/com/reprezen/swagedit/json/schema.json#L932

@tedepstein
Copy link
Collaborator Author

I'd recommend fixing this (and a few other issues) by amending the schema to add an appropriate enum restriction in each case.

In this particular case, I think we could submit our schema change as a pull request back to the swagger-spec project. But regardless of whether our PR is accepted, it makes sense for SwagEdit to own and control its own version of the schema.

(Note: In other cases, we'd maintain our changes locally because they aren't strictly required. For example, the spec doesn't require schemes to be specified; it defaults to a value of http; but the Swagger UI doesn't recognize this default, and therefore requires schemes to be explicitly specified. We might want to require it in API Studio for this reason, and a schema change might be the most expedient way to do that.)

@ghillairet
Copy link
Member

I took the formats from here http://swagger.io/specification/#dataTypeFormat, and implemented like below in schema.json file. I left the default string type, so that users can still define their own formats.

 "format": {
    "oneOf": [
      {
        "type": "string"
      },
      {
        "type": "string",
        "enum": [
          "int32",
          "int64",
          "float",
          "double",
          "byte",
          "binary",
          "date",
          "date-time",
          "password"
        ]
      }
    ]
  }

Completion for format

screen shot 2016-01-15 at 11 46 58

@tedepstein
Copy link
Collaborator Author

@ghillairet , good idea. I didn't realize that Swagger allowed custom formats.

But IIUC, JSON Schema's oneOf will fail validation if a value matches more than one of the provided schemas. Would anyOf work better here?

@tedepstein
Copy link
Collaborator Author

@ghillairet
Copy link
Member

Not sure about this.

oneOf > validates successfully against exactly one schema
anyOf > validates successfully against at least one schema

I would say oneOf is ok.

@tedepstein
Copy link
Collaborator Author

Please try it with a JSON schema validator. I think the difference between oneOf and anyOf is as I described. oneOf has to match exactly one of the provided schemas. It's an XOR, whereas anyOf is an inclusive OR.

@ghillairet
Copy link
Member

Both cases don't make any difference for the validator, since the first schema matches all string and the second matches a enum of strings. It's always the first schema that will be taken into account for validation. If it's not a string, then validation will fail.

So I believe for this case anyOf or oneOf are both valid.

tfesenko added a commit that referenced this issue Jan 15, 2016
[#46] Code assist for `format` keyword inserts empty quotes
@tedepstein tedepstein reopened this Jan 16, 2016
@tedepstein
Copy link
Collaborator Author

Reopening, as I see a few problems remaining:

  1. The definition is repeated several times in the schema. It should be specified once, in the definitions section, and referenced there. In this milestone, even if we don't address any of the other points below, we should at least address that one.
  2. There are no unit tests to validate that the code assist suggestions are working correctly.
  3. I checked the JSON Schema Test Suite and tested in JSON-Schema Lint. It's clear that anyOf is the correct validation to use here.

Using the schema as specified, with oneOf:

{
  "oneOf": [
    {
      "type": "string"
    },
    {
      "type": "string",
      "enum": [
        "int32",
        "int64",
        "float",
        "double",
        "byte",
        "binary",
        "date",
        "date-time",
        "password"
      ]
    }
  ]
}

Here's JSON Schema Lint on a non-string value, as a baseline test:

image

Here's a random string, which matches the first schema, but not the second, and therefore satisfies oneOf:

image

Here's "double", which matches both schemas, and therefore fails oneOf

image

Change the first schema to number, and it passes validation:

image

Or change it back to string but use anyOf instead of oneOf. Again, it passes:

image

So I have to conclude that the JSON Schema validator we're using in SwagEdit / YEdit is not implementing oneOf semantics correctly. This is not caused any other problems that we've noticed so far, but it's something to be aware of. And we don't want our schema to start reporting validation errors if we upgrade to a new validator that fixes this problem.

ghillairet added a commit that referenced this issue Jan 21, 2016
@ghillairet
Copy link
Member

I changed to use anyOf instead.

Note for later that validation fails also here with oneOf http://json-schema-validator.herokuapp.com/
This online validator uses the validation library we are using in SwagEdit, so there is maybe a bug in the library.

@tedepstein
Copy link
Collaborator Author

@ghillairet , this change looks good. Please open a PR for this.

@tfesenko
Copy link
Member

6c1a1d9 was committed directly to master.

@tfesenko
Copy link
Member

There is still no unit tests for this functionality, created #84 "Tests for code assist for format".
Closing this issue.

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

3 participants