Skip to content

Should "readOnly" (and "writeOnly") be an array like "required"? #364

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

Closed
handrews opened this issue Aug 22, 2017 · 22 comments
Closed

Should "readOnly" (and "writeOnly") be an array like "required"? #364

handrews opened this issue Aug 22, 2017 · 22 comments

Comments

@handrews
Copy link
Contributor

In the discussions for "deprecated", @awwright brought up that rather than make it a boolean in subschemas, it probably makes more sense to follow the pattern of "required" and make it an array or object alongside of "properties". Similar to #117 ("patternRequired") we would need an analogous "patternReadOnly" (and "additionalReadOnly"?)

Despite the likely need to add separate pattern/additional support, it feels like we should have a consistent design pattern of either specifying these things at the object level, or at the subschema level. Having worked with object level "required" for several years, I feel that it is the better approach.

@handrews handrews added this to the draft-07 (wright-*-02) milestone Aug 22, 2017
@handrews
Copy link
Contributor Author

handrews commented Aug 31, 2017

I just thought of a pretty compelling reason to move "readOnly" to an array keyword at the object levell:

Validation schemas fundamentally describe a type, in the form of an acceptable set of values. "readOnly" not only does not impact that, but is highly context-dependent. It is likely to vary for the same type depending on where and how it is used.

This is part of why "readOnly" is given as a justification for "$use". With "allOf", it's not clear how to turn read-only-ness off. If any branch is of an "allOf" is read-only, then the result would have to be read-only.

Making "readOnly" an array removes that problem, and removes the most substantial justification (for me) when it comes to "$use". Letting applications decide how to handle conflicting titles, descriptions, and even defaults is at least plausible. But there was no other way I could think of to handle "readOnly" and re-use. Except now this.

@handrews
Copy link
Contributor Author

@geemus this would affect PRMD if you were to move it to (or add support for) draft-07.

@jdesrosiers
Copy link
Member

This exact thing occurred to me recently as well while writing hyper-schemas. I think making readOnly a single array like required would be nice.

However, there is one use case I would loose. I sometimes use readOnly at the top level of a hyper-schema to indicate to a client that the entire resource is managed by the server. The client would know that it shouldn't expect any requests that modifies the resource to be successful. I'm willing to lose this use case in order to make the more general case easier to work with.

@handrews
Copy link
Contributor Author

handrews commented Sep 3, 2017

@jdesrosiers

I sometimes use readOnly at the top level of a hyper-schema to indicate to a client that the entire resource is managed by the server.

This use case occurred to me, but I think in most cases it is better handled at the link level (including "self" links) with "targetHints" #383. For HTTP, that would be {"targetHints": {"allow": ["GET"]}} or similar.

Recommending a hyper-schema approach while we're moving readOnly to annotation/validation is a bit odd, so there is a gap still left that a readOnly array would not cover either.

I've also considered the unusual but possible case of making some but not all array elements read-only. This would mostly be fore the tuple form of items. Should the readOnly array allow integers when applied to an array instance? Obviously, to make the entire array read-only, one would apply readOnly to its parent property.

I still like this idea, but there are definitely some odd corner cases to smooth out.

@geemus
Copy link
Collaborator

geemus commented Sep 5, 2017

@handrews sounds reasonable, thanks for the heads up.

@handrews
Copy link
Contributor Author

See also PR #424 which delves more into the nature of annotation keywords. It might help us decide which way to go on this (although I'm still waffling back and forth myself for readOnly).

@handrews
Copy link
Contributor Author

handrews commented Oct 8, 2017

I don't think I have the energy left to consider this in draft-07, and assuming the concept of applicability in #424 is accepted, I don't think it's as much of a concern. Bumping it out to the "future" milestone and lowering the priority unless someone wants to champion this.

@handrews handrews modified the milestones: draft-07 (wright-*-02), draft-future Oct 8, 2017
@handrews handrews changed the title Should "readOnly" behave like "required"? Should "readOnly" (and "writeOnly") be an array like "required"? Nov 9, 2017
@handrews handrews modified the milestones: draft-future, draft-08 Dec 29, 2017
@handrews
Copy link
Contributor Author

Having spent a lot more time thinking about annotations and how they work over the last several months, I've decided against changing these, so I'm going to close this (feel free to comment if you have a compelling reason that I should reconsider and it can of course be re-opened).

Reasons to drop this idea:

  1. The logical behavior of a boolean annotation of this sort is that once applied, it cannot be lifted. Even though annotations are not constraints in the same way as assertions, this both matches intuition and is consistent with how assertion constraints work.
  2. The right way to handle perceived needs to lift the annotation is to move it out to the point of use. Don't declare re-usable things read-only unless the goal is to ensure that all cases are read-only. Otherwise, let this be set at point of use. Or make both a read-only and a read-write re-usable schemas.
  3. This would be a substantial breaking change. Even though readOnly was in Hyper-Schema, it has been broadly used outside of it, most notably in OpenAPI.
  4. The whole patternReadOnly/additionalReadOnly is a pretty clear design smell, and the analogy with required is significantly flawed. You would never make some but not all of a pattern read-only. If you want to do that, make another, more targeted pattern. patternRequired is more of a pattern-specific minRequired sort of thing, which is very different.
  5. JSON Schema in general allows nonsense to exist harmlessly in case an application finds a use for it. So it may not make sense to put "readOnly": true in certain locations, but it's definitely not harmful, and I'd argue better than getting into patternReadOnly/additionalReadOnly territory (with analogous writeOnly keywords).
  6. I'd had some concern about having to "find" all of the places where readOnly could appear. But this has become closer and closer to a core requirement the more we work with annotations, and the algorithm and usage has become more clear. I'm no longer concerned about it.

So I think it's best to drop this.

@brabeji
Copy link

brabeji commented Jan 15, 2018

I've stumbled upon a rather practical issue. We are trying to do:

{ $ref: '#/definitions/Foo', readOnly: true }

where readOnly will get stripped when the pointer is resolved.

... make both a read-only and a read-write re-usable schemas.

Do you mean this?

{ allOf: [ { $ref: '#/definitions/Foo' }, { readOnly: true } ] }

Thanks!

@dlax
Copy link
Member

dlax commented Jan 15, 2018 via email

@handrews
Copy link
Contributor Author

@brabeji this will likely be addressed in draft-08 via #523.

@jugaadi
Copy link

jugaadi commented Apr 27, 2022

What would be the recommended way to handle readOnly with allOf?

We would like to make status field in the below given data readOnly for POST requests. Right now, we're creating post specific schemas like given below. A readOnly array can handle it in a better way.

Example:

{
  "user": {
    "type": "object",
    "properties": {
      "name": {
        "type": "string"
      },
      "email": {
        "type": "string",
        "format": "email"
      },
      "status": {
        "type": "string",
        "enum": ["active", "inactive"]
      }
    }
  },
  "user-post-request": {
    "allOf": [{
        "$ref": "#/user"
      },
      {
        "status": false
      }
    ]
  }
}

@karenetheridge
Copy link
Member

@jugaadi I don't see readOnly anywhere in your example schema, so I'm not sure what you are wanting to happen?

readOnly generates an annotation, and recommended semantics for combining them from multiple schema locations is documented in the specification here: https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.9.4

@jugaadi
Copy link

jugaadi commented Apr 28, 2022

@karenetheridge Sorry for not being more clear. The example is how Im solving it now i.e by modeling request and response
as two separate schemas and skipping the fields(status: false) directly in the sub-schema. If I had readOnly as an array, I could have simply put the following:

"user-post": {
  "allOf": [{
      "$ref": "#/user"
    },
    {
      "type": "object",
      "readOnly": ["status"]
    }
  ]
}

@karenetheridge
Copy link
Member

karenetheridge commented Apr 28, 2022

Well technically, you could do this:

"user-post": {
  "$ref": "#/user",
  "type": "object",
  "readOnly": ["status"]
}

The problem with moving readOnly up to the object level rather than at the property level is that you are assuming the keyword will only be used for object properties. How will we indicate that a particular array item is to be readOnly, or an individual null, boolean, number or string? Having the keyword right at the position of the target data achieves that, for all data types. Using a property name as the keyword value does not.

@gregsdennis
Copy link
Member

How will we indicate that a particular array item is to be readOnly

Or... how would we differentiate between an array being read-only vs the items within it being read-only?

@awwright
Copy link
Member

(I feel like I commented on this elsewhere but I can't find it...) The array form of "required" was only necessary because it needs to be accessible to the validator when said property does not exist.

For "readOnly", the only time it's going to be accessed is when the property exists: If a user creates the property while preparing it for submission, they would immediately be able to see that it's read-only (from the generated annotation).

I would think this ought to be sufficient:

// ...
"user-post": {
  "$ref": "#/user",
  "type": "object",
  "properties": {
    "status": {"readOnly": true},
  }
}

But maybe this is impractical, or there's another use for "readOnly" where this logic isn't applicable. Is this the case?

@gregsdennis
Copy link
Member

For "readOnly", the only time it's going to be accessed is when the property exists

C# (and likely other languages) have read-only types built in.

https://devblogs.microsoft.com/premier-developer/the-in-modifier-and-the-readonly-structs-in-c/

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record

If you're using schema to model types, this can be important to have outside of properties.

@awwright
Copy link
Member

If you're using schema to model types, this can be important to have outside of properties.

What about keywords like "type"? Does the same logic apply?

If you're using this to model types, then what you're doing is a static analysis of the entire schema, so you can still see "X property is going to be a readonly string, max 20 characters".

@jugaadi
Copy link

jugaadi commented Apr 29, 2022

"user-post": {
  "$ref": "#/user",
  "type": "object",
  "properties": {
    "status": {"readOnly": true},
  }
}

@awwright In the above case, is it safe to assume that the rest of the constraints/vocabularies that are applicable for status field would be inherited from "$ref": "#/user" and other subschemas in allOf?

@awwright
Copy link
Member

@jugaadi Yes, all the keywords will be used to validate the property. I suggest, if in doubt, run a few experiments to get a feel for how it works.

@jugaadi
Copy link

jugaadi commented Apr 29, 2022

Thanks @awwright @gregsdennis @karenetheridge

Skipping the fields instead of depending on annotations seem to be a better solution for my problem since it does not depend on the owning authority's implementation choice to ignore or reject. Any choice would have a bearing on our API versioning strategy.

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

9 participants