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

readOnly and writeOnly should be at the same level as required in the Schema #1671

Closed
robertmassaioli opened this issue Aug 21, 2018 · 13 comments

Comments

@robertmassaioli
Copy link

robertmassaioli commented Aug 21, 2018

In the Schema, when you want to mark a property as required you do this:

{
  "type": "object",
  "required": [
    "name"
  ],
  "properties": {
    "name": {
      "type": "string"
    },
    "address": {
      "$ref": "#/components/schemas/Address"
    },
    "age": {
      "type": "integer",
      "format": "int32",
      "minimum": 0
    }
  }
}

And then, if you want to mark a property as readOnly then you would do this:

{
  "type": "object",
  "required": [
    "name"
  ],
  "properties": {
    "name": {
      "readOnly": true,
      "type": "string"
    },
    "address": {
      "$ref": "#/components/schemas/Address"
    },
    "age": {
      "type": "integer",
      "format": "int32",
      "minimum": 0
    }
  }
}

However, what if you want to mark address as readOnly. This would be invalid and the readOnly property on address would be ignored by a valid parser for example:

{
  "type": "object",
  "required": [
    "name"
  ],
  "properties": {
    "name": {
      "readOnly": true,
      "type": "string"
    },
    "address": {
      "readOnly": true,
      "$ref": "#/components/schemas/Address"
    },
    "age": {
      "type": "integer",
      "format": "int32",
      "minimum": 0
    }
  }
}

As far as I can tell, you only have two options:

  1. Inline #/components/schemas/Address into address and then add readOnly.
  2. Put the readOnly property into the root of the #/components/schemas/Address definition. However, this does not seem to be possible thanks to Clarify spec wrt readOnly and writeOnly in referenced schemas #1622.

What this means, is that it's impossible to make refs and readOnly work well together.

In my mind, the best solution would be to make readyOnly and writeOnly work like required. Like so:

{
  "type": "object",
  "required": [
    "name"
  ],
  "readOnly": [
     "name",
     "address"
  ],
  "properties": {
    "name": {
      "type": "string"
    },
    "address": {
      "$ref": "#/components/schemas/Address"
    },
    "age": {
      "type": "integer",
      "format": "int32",
      "minimum": 0
    }
  }
}

Is this a reasonable proposal? It is affecting our current REST API documentation.

Note: I'm NOT asking to deprecate the current readOnly and writeOnly fields but rather to add extra readOnly and writeOnly fields at the level of required. The readOnly property at the required level would have an OR relationship with the readOnly property at the current level (and the same would apply for writeOnly).

@robertmassaioli robertmassaioli changed the title readOnly and writeOnly should be at the same level as required readOnly and writeOnly should be at the same level as required in the Schema Aug 21, 2018
@MikeRalphson
Copy link
Member

You can wrap the $ref and readOnly in an allOf (tooling support may not be universal).

@hkosova
Copy link
Contributor

hkosova commented Aug 21, 2018

As @MikeRalphson suggested, you can use allOf like so:

    "address": {
      "readOnly": true,
      "allOf": [
         {"$ref": "#/components/schemas/Address"}
       ]
    },

@robertmassaioli
Copy link
Author

robertmassaioli commented Aug 21, 2018

That's actually a pretty good suggestion that I did not think of. A little clunky having a single element allOf, but I think it will work, which is the important thing.

I'll just test this out now but, if I don't respond within two weeks, then consider this issue resolved.

@MikeRalphson
Copy link
Member

Personally, I prefer the explicitness of putting the readOnly within its own element inside the allOf.

@robertmassaioli
Copy link
Author

robertmassaioli commented Aug 21, 2018

I guess that readOnly and writeOnly in the combinators (allOf, oneOf, anyOf) is possible but might be confusing for some. While it works for allOf it seems like the behaviour would be weird for the others. For example, what does this mean?

    "address": {
      "oneOf": [
         {"$ref": "#/components/schemas/Address"},
         {"readOnly": true}
       ]
    },

What is the readOnly behaviour here? Is it only readOnly if it's not an Address?

If you intend the behaviour to effect the whole object in all cases then I think I would still put it on the top level.

@handrews
Copy link
Member

(for those who don't know me, I'm the most active editor of the JSON Schema spec)

@robertmassaioli we discussed switching readOnly, writeOnly, and deprecated (which is proposed for JSON Schema but not formally added yet) to something like required, but that brings its own complications. The current arrangement, while somewhat inconsistent, seems to work out better.

Currently, the allOf is the recommended way of doing things. As of JSON Schema draft-07 (a.k.a. draft-handrews-json-schema-01), the spec explicitly discusses how to apply fields such as these when using allOf, oneOf, etc.

This is further clarified in the forthcoming draft-08. Additionally, {"$ref": "#/foo", "readOnly": true} is valid in draft-08, and does what you would expect (which is the same thing that happens with allOf ,but more concise, and closer to users' expectations).

@robertmassaioli
Copy link
Author

@handrews It's good to meet you. And thankyou, I did not realise that having a $ref and other properties in the one location was valid in draft-08. I should go read the specification to understand how the merging behaviour is supposed to work.

It's good to know that allOf is the expected way to do this btw. This will help when developing our platform and tooling.

Btw @MikeRalphson I just wanted to say that I'm a big fan of your work on: https://github.com/Mermade/oas-kit, specifically swagger2openapi.

I work for Atlassian on the developer.atlassian.com team and we are either consuming OAS 3.0 files (or converting Swagger 2.0 files to OAS 3.0 files) to then use them as the source of truth for our REST API documentation. For example, this is our largest (by far) Swagger file to document the Jira Cloud Platform REST API: https://developer.atlassian.com/cloud/jira/platform/rest/

I share this just to say thank you for the great work on Json Schema and Swagger respectively.

@handrews
Copy link
Member

@robertmassaioli draft-08 isn't out yet, although I think that part has been committed to master in the repo. I hope to have draft-08 published in October.

@robertmassaioli
Copy link
Author

I think it's safe to say: no action required. This issue is resolved.

@n0tel
Copy link

n0tel commented Oct 18, 2019

@handrews Hi. I'm sorry, but i read here #1622 that readOnly and writeOnly are only effective in the context of a property subschema

@handrews
Copy link
Member

@n0tel #1622 (comment)

@softm-gustavomelendez
Copy link

This post seems to have been a year since its last comment, and apparently it is still necessary to use the allOf for the refs:

This at the json schema level looks perfect, but the tool that builds the definition for me, with the controllers and the models, the tool at https://editor.swagger.io/. for ASP.NET Core projects. I have to tell you that those allOf classes it generates are terribly horrible out of context. How could this be different to allow ReadOnly and WriteOnly to be direct are allOF, or improve the tool so that it does not create those horrible classes.

How can I improve that

@MikeRalphson
Copy link
Member

@softm-gustavomelendez as tooling is out of scope for this repository, I can only suggest raising an issue on the relevant repo and linking back here.

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

No branches or pull requests

6 participants