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

fix(rulesets): required readOnly and writeOnly properties should not … #2573

Merged
merged 2 commits into from May 3, 2024

Conversation

pplr
Copy link
Contributor

@pplr pplr commented Jan 10, 2024

…be considered required for respectively request and response bodies

Fixes #1274.

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@pplr pplr requested a review from a team as a code owner January 10, 2024 16:32
@pplr pplr force-pushed the fix/read-write-only-examples branch 7 times, most recently from 8ddecd1 to 7fd98c3 Compare January 11, 2024 09:22
… properties

Required readOnly and writeOnly properties should not be considered required
for respectively request and response bodies.
@pplr pplr force-pushed the fix/read-write-only-examples branch from 7fd98c3 to 2352e39 Compare January 16, 2024 12:51
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I left one remark. LMK if you have questions.

parent,
propertyName,
) => {
if ((fragment.readOnly === true && readOnlyProperties) || (fragment.writeOnly === true && writeOnlyProperties)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we also change the schema to false or not: true?
That's to ensure we catch if the property is defined.
To illustrate it with example:

{
  "type": "object",
  "properties": {
    "foo": {
      "type": "string",
      "readOnly": true
    }
  },
  "required": [
    "foo"
  ]
}

The above schema would be transformed to

{
  "type": "object",
  "properties": {
    "foo": false
  }
}

This way we wouldn't let property "foo" be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @P0lip, do you request to make readOnly / writeOnly properties forbidden on request / response? Not sure that this is the exact definition of the read and writeOnly flags.

The 3.0 OAS spec says "SHOULD NOT".

For OAS 3.1 / json schema, the readOnly / writeOnly definition is more open.

If readOnly has a value of boolean true, it indicates that the value of the
instance is managed exclusively by the owning authority, and attempts by an
application to modify the value of this property are expected to be ignored or
rejected by that owning authority.

Your proposal could be optional and enabled with a flag like 'strict-read-write-only' ?

@P0lip P0lip force-pushed the develop branch 3 times, most recently from dc90b7a to c22f408 Compare April 4, 2024 13:29
@mnaumanali94 mnaumanali94 self-requested a review May 3, 2024 14:48
@mnaumanali94 mnaumanali94 enabled auto-merge (squash) May 3, 2024 14:48
@mnaumanali94 mnaumanali94 merged commit ae1fea5 into stoplightio:develop May 3, 2024
9 checks passed
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.

oas3-valid-oas-content-example rule needs to obey readOnly/writeOnly property
3 participants