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: readOnly objects in arrays are handled correctly #2513

Merged
merged 17 commits into from
Apr 26, 2024

Conversation

ilanashapiro
Copy link
Contributor

@ilanashapiro ilanashapiro commented Apr 18, 2024

Addresses #2336

Summary

Currently, given an object with a readOnly required field, Prism does not handle it properly as soon as the object is embedded into an array. Specifically, the user will get an error saying they must manually specify the readOnly property. This PR fixes this issue for all possibilities of arrays: single-typed arrays specified by a single item schema, and tuple-typed arrays specified by a list of item schemas, both with and without the additionalItems schema for tuple-typed arrays.

See: https://json-schema.org/understanding-json-schema/reference/array
And also see: https://tools.ietf.org/html/draft-zyp-json-schema-03 for items type signature (sections 5.5, 6.4, and 6.9) and additionalItems type signature (sections 5.6, 6.4, and 6.10) in the code

Checklist

  • The basics
    • I tested these changes manually in my local or dev environment
  • Tests
    • Added or updated
    • N/A
  • Event Tracking
    • I added event tracking and followed the event tracking guidelines
    • N/A
  • Error Reporting
    • I reported errors and followed the error reporting guidelines
    • N/A

Screenshots
--------------------------------------------------READONLY TESTS--------------------------------------------------
Given the following schema (which includes a non-tuple-typed array):

openapi: 3.1.0

paths:
  /pets:
    post:
      requestBody:
        content:
          application/json:
            schema:
                $ref: '#/components/schemas/Pets'
      responses:
        '200':
          description: A pet id
          "content":
            application/json:
              schema:
                type: object
                properties:
                  id:     
                    type: integer
                    format: int64
                required: 
                  - "id"
                additionalProperties: false

  
components:
  schemas:
    Pets:
      type: object
      properties:
        objectsArray:
          type: array
          items:
            type: object
            required:
              - id
              - name
            properties:
              id:
                readOnly: true
                type: string
              name:
                type: string
        title:
          type: string
          readOnly: true
        address:
          type: integer
      required:
        - title
        - address

I send the following POST request successfully:
image
image

With Prism before this fix, we get an error:
image
image

If I change the schema to include a tuple-typed array, with additionalItems specified for demonstration like so:

openapi: 3.1.0

paths:
  /pets:
    post:
      requestBody:
        content:
          application/json:
            schema:
                $ref: '#/components/schemas/Pets'
      responses:
        '200':
          description: A pet id
          "content":
            application/json:
              schema:
                type: object
                properties:
                  id:     
                    type: integer
                    format: int64
                required: 
                  - "id"
                additionalProperties: false

  
components:
  schemas:
    Pets:
      type: object
      properties:
        objectsArrayWithAdditionalItems:
          type: array
          items:
            - type: object
              required:
                - id
                - name
              properties:
                id:
                  readOnly: true
                  type: string
                name:
                  type: string
            - type: object
              required:
                - address
                - title
              properties:
                address:
                  readOnly: true
                  type: string
                title:
                  type: string
          additionalItems:
            type: object
            required:
              - status
              - ticket
            properties:
              status:
                readOnly: true
                type: string
              ticket:
                type: string

we see that the fix works for readonly properties in both the tuple-typed items list, and the additionalItems schema object.
image
image

With Prism before this fix, we get an error:
image
image

--------------------------------------------------WRITEONLY TESTS--------------------------------------------------
Given the following schema (which includes a non-tuple-typed array):

openapi: 3.1.0

paths:
  /pets:
    get:
      requestBody: {}
      responses:
        '200':
          description: Detailed pet information
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pets'

components:
  schemas:
    Pets:
      type: object
      properties:
        objectsArray:
          type: array
          items:
            type: object
            required:
              - id
              - name
            properties:
              id:
                writeOnly: true
                type: string
              name:
                type: string
              additionalItems: false
        title:
          type: string
          writeOnly: true
        address:
          type: integer
        additionalItems: false
      required:
        - title
        - address

I send the following POST request and get a response with the writeOnly properties (including the "id" property in the array) excluded as they're supposed to be:
image
image

With Prism before this fix, we get the writeOnly property "id" from the array inappropriately included in the response:
image
image

If I change the schema to include a tuple-typed array:

openapi: 3.1.0

paths:
  /pets:
    get:
      requestBody: {}
      responses:
        '200':
          description: Detailed pet information
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pets'

components:
  schemas:
    Pets:
      type: object
      properties:
        objectsArrayWithAdditionalItems:
          type: array
          items:
            - type: object
              required:
                - id
                - name
              properties:
                id:
                  writeOnly: true
                  type: string
                name:
                  type: string
                additionalProperties: false
            - type: object
              required:
                - address
                - title
              properties:
                address:
                  writeOnly: true
                  type: string
                title:
                  type: string
                additionalProperties: false
        additionalProperties: false

we see that the fix works for writeOnly properties in the tuple-typed array -- they're appropriately excluded from the response:
image
image

With Prism before this fix, however, the writeOnly properties in the array are inappropriately returned in the response:
image
image

@ilanashapiro ilanashapiro changed the title Fix/readonly array fix: readOnly objects in arrays are handles correctly Apr 18, 2024
@ilanashapiro ilanashapiro changed the title fix: readOnly objects in arrays are handles correctly fix: readOnly objects in arrays are handled correctly Apr 18, 2024
@ilanashapiro
Copy link
Contributor Author

@rattrayalex

@ilanashapiro ilanashapiro marked this pull request as ready for review April 18, 2024 21:33
@ilanashapiro ilanashapiro requested a review from a team as a code owner April 18, 2024 21:33
@ilanashapiro ilanashapiro requested review from EdVinyard and removed request for a team April 18, 2024 21:33
@brendarearden
Copy link
Contributor

Thank you for opening this PR! Everything here looks great. After reviewing, we are wondering if these array changes need to be added for writeOnly as well. Is that something you would be willing to add as part of this PR?

@ilanashapiro
Copy link
Contributor Author

Hi Brenda, thanks for the feedback! I thought about this for a moment and realized that the existing PR actually does handle arrays with writeOnly as well (it's independent of the predicate used). I am working on adding some test cases for this and will update shortly!

@ilanashapiro ilanashapiro marked this pull request as draft April 19, 2024 23:18
@ilanashapiro
Copy link
Contributor Author

I've added tests (unit and end-end) showing this works for writeonly. However, I didn't include additionalItems in the tuple typed array for the end-end tests. additionalItems are indeed being filtered correctly for both readOnly and writeOnly based on the unit tests, but there is apparently an unrelated bug in Prism where additionalItems are never returned in the response. See the following repro using the master branch of Prism:

openapi: 3.1.0

paths:
  /pets:
    get:
      requestBody: {}
      responses:
        '200':
          description: Detailed pet information
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pets'

components:
  schemas:
    Pets:
      type: object
      properties:
        objectsArrayWithAdditionalItems:
          type: array
          items:
            - type: object
              required:
                - id
                - name
              properties:
                id:
                  type: string
                name:
                  type: string
                additionalProperties: false
            - type: object
              required:
                - address
                - title
              properties:
                address:
                  type: string
                title:
                  type: string
                additionalProperties: false
          additionalItems: 
            type: object
            required:
              - status
              - ticket
            properties:
              status:
                type: string
              ticket:
                type: string
              additionalProperties: false
        additionalProperties: false
image Note how status and ticket aren't included in a third array item.

@ilanashapiro ilanashapiro marked this pull request as ready for review April 21, 2024 05:34
@ilanashapiro
Copy link
Contributor Author

@brendarearden Hi! Would it be possible to get a follow up review? Thank you!

Copy link
Contributor

@brendarearden brendarearden left a comment

Choose a reason for hiding this comment

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

looks good!

@brendarearden brendarearden merged commit 7670236 into stoplightio:master Apr 26, 2024
9 checks passed
@brendarearden
Copy link
Contributor

#2519 I opened an issue to track the additionalitems bug you discovered as well!

This was referenced Apr 29, 2024
This was referenced Jul 2, 2024
This was referenced Aug 8, 2024
ilanashapiro added a commit to ilanashapiro/prism that referenced this pull request Aug 22, 2024
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.

2 participants