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 does not work together with polymorphic oneOf. #400

Closed
hallsbyra opened this issue Oct 16, 2020 · 5 comments
Closed

readOnly does not work together with polymorphic oneOf. #400

hallsbyra opened this issue Oct 16, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@hallsbyra
Copy link

hallsbyra commented Oct 16, 2020

Describe the bug
readOnly does not work together with polymorphic oneOf.

To Reproduce

Use this spec:

openapi: 3.0.0
info:
  title: Dummy API
  description: Dummy API
  version: "0.1.0"
servers:
  - url: /v1
paths:
  '/orders':
    post:
      requestBody:
        content:
            application/json:
              schema:
                oneOf:
                  - $ref: '#/components/schemas/subA'
                  - $ref: '#/components/schemas/subB'
                discriminator:
                  propertyName: type
                  mapping:
                    A: '#/components/schemas/subA'
                    B: '#/components/schemas/subB'
      responses:
          200:
            description: successful operation

components:
  schemas:
    common:
      type: object
      properties:
        id:
          readOnly: true
          type: string
        type:
          type: string
          enum: [A, B]          
      required:
        - id
        - type  

    subA:
      allOf:
        - $ref: '#/components/schemas/common'

    subB:
      allOf:
        - $ref: '#/components/schemas/common'

POST this request to /v1/orders:

{
  "type": "A"
}

Actual behavior
Validation errors:

[
    {
      path: '.body.id',
      message: "should have required property 'id'",
      errorCode: 'required.openapi.validation'
    },
    {
      path: '.body.id',
      message: "should have required property 'id'",
      errorCode: 'required.openapi.validation'
    },
    {
      path: '.body',
      message: 'should match exactly one schema in oneOf',
      errorCode: 'oneOf.openapi.validation'
    }
]

Expected behavior
Request should be accepted, since id is readOnly.

@cdimascio cdimascio added the bug Something isn't working label Oct 25, 2020
@cdimascio
Copy link
Owner

The problem is in this code. As is, it does not handle anyOf, allOf, oneOf.
@hallsbyra Would you be up for taking a stab at it?

The following new test e.g. test/oneof.readonly.spec.ts
With the contents of your spec e.g. test/oneof.readonly.spec.yaml
can be used to test

import * as path from 'path';
import * as express from 'express';
import * as request from 'supertest';
import { createApp } from './common/app';

describe.only('one.of readonly', () => {
  let app = null;

  before(async () => {
    // Set up the express app
    const apiSpec = path.join(__dirname, 'oneof.readonly.yaml');
    app = await createApp({ apiSpec }, 3005, (app) =>
      app.use(
        express
          .Router()
          .post(`${app.basePath}/orders`, (req, res) =>
            res.status(200).json({ success: true }),
          ),
      ),
    );
  });

  after(() => {
    app.server.close();
  });

  it('post type (without readonly id) should pass', async () =>
    request(app)
      .post(`${app.basePath}/orders`)
      .send({ type: 'A' })
      .set('Content-Type', 'application/json')
      .expect(200))
});

cdimascio added a commit that referenced this issue Oct 26, 2020
)

* (fix) #400 readOnly does not work together with polymorphic oneOf.

* fix  typo

* (feat) enhanced readonly handling

* add oneOf test

* enable tests
@cdimascio
Copy link
Owner

@hallsbyra please give v4.3.1 a try. this should be fixed there

@hallsbyra
Copy link
Author

Thanks, seems to be almost solved. My original spec still doesn't work though, but that seems to be related to #104. And is easily fixed by getting rid of the discriminator and making each oneOf distinct from the other like so:

openapi: 3.0.0
info:
  title: Dummy API
  description: Dummy API
  version: "0.1.0"
servers:
  - url: /v1
paths:
  "/orders":
    post:
      requestBody:
        content:
          application/json:
            schema:
              oneOf:
                - $ref: "#/components/schemas/subA"
                - $ref: "#/components/schemas/subB"
      responses:
        200:
          description: successful operation

components:
  schemas:
    common:
      type: object
      properties:
        id:
          readOnly: true
          type: string
      required:
        - id

    subA:
      allOf:
        - $ref: "#/components/schemas/common"
      properties:
        type:
          type: string
          enum: [A]
      required:
        - type

    subB:
      allOf:
        - $ref: "#/components/schemas/common"
      properties:
        type:
          type: string
          enum: [B]
      required:
        - type

Problem is just that the above spec gives Error: schema is invalid: data.required should NOT have fewer than 1 items

It took a glance in the code yesterday and saw that you're removing required fields from the spec in case they are readOnly. I suspect, if ALL fields get removed, the whole required node must go too. Could that be it?

@cdimascio
Copy link
Owner

cdimascio commented Oct 26, 2020

yep, good catch. if there is only one required field and its readonly we need to remove the required field itself

@cdimascio cdimascio reopened this Oct 26, 2020
cdimascio added a commit that referenced this issue Oct 27, 2020
* remove required if no required props exist

* (fix) #400 readonly with single required prop fails

* remove 15
@cdimascio
Copy link
Owner

@hallsbyra this issue is fixed in v4.3.4

ex1st pushed a commit to ex1st/express-openapi-validator that referenced this issue Dec 9, 2020
* remove required if no required props exist

* (fix) cdimascio#400 readonly with single required prop fails

* remove 15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants