Support for polymorphic validation

[daveismith]

I've been trying to use swagger-tools to validate requests to my controllers. It seems that when I perform a request with the body parameter specified as a base class, validation happens only against the specification of the base class model, but not against the model for the extended class identified by the discriminator.

[whitlockjc]

Can you give a code snippet and some example? I ask because we have heavily tested validation so I'm not sure how to reproduce.

[daveismith]

Below a simplified version of my schema.

{
  "swagger": "2.0",
  "info": {
    "title": "My Cloud API",
    "version": "1.0"
  },
  "consumes": ["application/json"],
  "produces": ["application/json"],
  "host": "localhost:3000",
  "basePath": "/api",
  "paths": {
    "/commands": {
      "post": {
        "description": "Issues a command",
        "operationId": "executeCommand",
        "parameters": [
          {
            "name": "body",
            "in": "body",
            "description": "Command to execute",
            "required": true,
            "schema": {
              "$ref": "#/definitions/Command"
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Successful request.",
            "schema": {
              "$ref": "#/definitions/CommandResult"
            }
          },
          "default": {
            "description": "Invalid request.",
            "schema": {
              "$ref": "#/definitions/Error"
            }
          }
        }
      }
    }
  },
  "definitions": {
    "Error": {
      "properties": {
        "message": {
          "type": "string"
        }
      },
      "required": ["message"]
    },
    "CommandResult": {
      "properties": {
        "message": {
          "type": "string"
        }
      },
      "required": ["message"]
    },
    "Command": {
      "discriminator": "type",
      "properties": {
        "type": {
          "type": "string"
        },
        "propBase": {
          "type": "string"
        }
      },
      "required": ["type", "propBase"]
    },
    "Command1": {
      "description": "Command 1",
      "allOf": [
        {
          "$ref": "#/definitions/Command"
        },
        {
          "properties": {
            "prop1": {
              "type": "string"
            }
          },
          "required": ["prop1"]
        }
      ]
    }
  }
}

The issue is that commands that are sent to /api/commands are only validated against Command, regardless of the discriminator value. If I send the following it will still pass validation and call my controller function even though it's missing the required property "prop1" for the type "Command1".

{
  "type": "Command1",
  "propBase": "blah"
}

[whitlockjc]

I cannot reproduce this. Whenever I throw your definitions into my API and have my request/response use them, if I post only what you do, I get a request validation error (Parameter (command) failed schema validation]):

{
  [Error: Parameter (pet) failed schema validation]
  code: 'SCHEMA_VALIDATION_FAILED',
  failedValidation: true,
  results: {
    errors: [
      {
        "code": "OBJECT_MISSING_REQUIRED_PROPERTY",
        "message": "Missing required property: prop1",
        "path": []
      }
    ],
    warnings: []
  },
  path: [ 'paths', '/pets', 'post', 'parameters', '0' ],
  paramName: 'pet'
}

And when I post a full example, with the prop1 provided, the request is successful. Which version of swagger-tools are you using?

[daveismith]

My package.json has "swagger-tools": "^0.8.7". I am using it with MEAN.js, and have the following in my express.js configuration file. I'm using the express router vs the swagger-tools one, but it since that middleware comes after the validator, and it's validating some requests I'm hope I've got it correctly configured.

    var swaggerDoc = require('../app/api/swagger.json');

    swaggerTools.initializeMiddleware(swaggerDoc, function (middleware) {
        // Interpret Swagger resources and attach metadata to request - must be first in swagger-tools middleware chain
        app.use(middleware.swaggerMetadata());

        // Validate Swagger requests
        app.use(middleware.swaggerValidator());

        // Route validated requests to appropriate controller
        //app.use(middleware.swaggerRouter(options));

        // Serve the Swagger documents and Swagger UI
        app.use(middleware.swaggerUi());
    });

[whitlockjc]

Can you run with DEBUG=swagger-tools* and paste the output of the failed request? swagger-validator should be working right based on my tests.

[daveismith]

When I submit

{
  "type": "Command1",
  "propBase": "string"
}

the output of the request to the log is

NODE_ENV is not defined! Using default development environment
  swagger-tools:middleware Initializing middleware +0ms
  swagger-tools:middleware   Identified Swagger version: 2.0 +2ms
  swagger-tools:middleware   Validation: succeeded +26ms
  swagger-tools:middleware:metadata Initializing swagger-metadata middleware +162ms
  swagger-tools:middleware:metadata   Identified Swagger version: 2.0 +1ms
  swagger-tools:middleware:metadata   Found Path: /commands +1ms
  swagger-tools:middleware:validator Initializing swagger-validator middleware +1ms
  swagger-tools:middleware:validator   Response validation: disabled +0ms
  swagger-tools:middleware:ui Initializing swagger-ui middleware +35ms
  swagger-tools:middleware:ui   Using swagger-ui from: internal +1ms
  swagger-tools:middleware:ui   API Docs path: /api-docs +0ms
  swagger-tools:middleware:ui   swagger-ui path: /docs +0ms
MEAN.JS application started on port 3000
  swagger-tools:middleware:metadata POST /api/commands +2m
  swagger-tools:middleware:metadata   Is a Swagger path: true +1ms
  swagger-tools:middleware:metadata   Is a Swagger operation: true +0ms
  swagger-tools:middleware:metadata   Processing Parameters +0ms
  swagger-tools:middleware:metadata     body +1ms
  swagger-tools:middleware:metadata       Type: object +1ms
  swagger-tools:middleware:metadata       Value provided: true +0ms
  swagger-tools:middleware:metadata       Value: [object Object] +0ms
  swagger-tools:middleware:validator POST /api/commands +0ms
  swagger-tools:middleware:validator   Will process: yes +0ms
  swagger-tools:middleware:ui POST /api/commands +5ms
  swagger-tools:middleware:ui   Will process: no +0ms
POST /api/commands 200 159.372 ms - 4

[whitlockjc]

Can you show me the rest of your server file, replacing sensitive data with reasonable mocks so as not to change the structure of the file?

[whitlockjc]

I hate to keep bugging you but I cannot reproduce it. If you'd rather not give me your server file, could you give me a recipe via a patch/PR that will showcase this issue in test/2.0/test-middleware-swagger-validator.js. Without a way to reproduce this, I cannot fix it.

[whitlockjc]

I was able to reproduce it. It required me to get outside of the test suite. Let me take a look.

[daveismith]

Awesome. I was just about to start putting together a skeleton app for you. Sorry for the delay. I was out grabbing some lunch. Let me know if you need the skeleton app anymore.

[whitlockjc]

I've figured it out, swagger-tools is working right which is why my test worked but the server didn't. Your parameter references #/definitions/Command but you want it to be #/definitions/Command1. If I change the reference, it works as expected.

[daveismith]

Right, but the goal in reality is that I want to have Command1, Command2, Command3, etc. Is that not possible?

[whitlockjc]

Let me get @webron involved as I don't understand the discriminator field honestly, I thought it was only useful for swagger-codegen. I would expect that allowing a parameter, or a response, to be N different types would violate Swagger's design goals but let's see what Ron has to say.

[daveismith]

Cool. I'm pretty new to swagger, so I may be understanding it incorrectly. I'll wait to hear what the verdict is. Thanks for being so responsive.

[webron]

I'm guessing TL;DR won't work here? 😊

Let me have a look.

[webron]

Okay, I hope I got everything needed from the thread. If the intention is to pass Command as a (body) parameter, and Command has a discriminator, then indeed anything that adds Command to it (via allOf) becomes a valid input (and this is recursive). So in this case, both Command and Command1 would be valid as input for the operation.

Two side notes - I'm happy to see that you've used the discriminator properly. Describing it is a bit complicated, and you've hit both spots - the discriminator is required and the sub-Command overrides the discriminator with a specific value (seems natural, but it's very easy to miss).

The other, which is slightly unrelated, for every model you should include "type": "object". While missing it will still work, technically, it allows things you don't want to allow.

[whitlockjc]

Let me get this straight: If your model has an ancestor that uses the discriminator, that model is a valid value anywhere the ancestor is used as a type?

[daveismith]

That was my understanding. Think of it like a function in a language with strong typing like Java or C++. I can declare a function that receives a base class or interface, and any function that uses that base class can also accept any subclass too.

[webron]

@whitlockjc - that is correct and the explanation @daveismith provided is fairly accurate. The discriminator is used to declare inheritance between models (unlike the simple usage of allOf that's basically composition but offers not hierarchical relationship between the models). So whenever you use the parent definition, its descendants could be used as well. Hope that's clear.

[whitlockjc]

Wow...that changes all kinds of things.

[whitlockjc]

@webron - So to support this, I would need to see if a schema had a discriminator and if so, find all of the ancestors of the model. For each of these ancestors, validate the provided value against its schema and if any of those ancestor schemas validate successfully, validation succeeds. If all of them fail, validation fails. Is that correct?

[daveismith]

Wouldn't it be the reverse? If the specified schema had a discriminator you would need to look for any schemas that have an allOf that specifies a reference to the schema in question.

[whitlockjc]

Ah yes, that is correct.

[daveismith]

Actually, it's not even that complex. When you receive an object that is specified by a schema with a discriminator you would just look at the value of the discriminator field to determine the name of the schema that should be used (hopefully not too much more complex that what's already happening).

[whitlockjc]

It's not a complexity thing to understand, just a potentially complex thing to implement due to how we do validation right now. I could be wrong, it might be really simple.

[daveismith]

@webron mentioned recursion. I'm not 100% on that, I can think of two cases.

• subclass specifies another, different discriminator?
• a class specifies the subclass in an allOf, then the discriminator would inform you of the final class directly

[whitlockjc]

Can you join #swagger on irc.freenode.net? It might be easier to chat in real time there.

[daveismith]

I've got to head out now, but could tomorrow.

[whitlockjc]

Please do. I think I understand it but my understanding doesn't sound simple so I could be misunderstanding it. I'm jeremyw so highlight me and I'll respond.

[webron]

When I was talking about recursion, there are two cases:

• There's a model (ModelX) with allOf parent that doesn't specify the discriminator value (composition), and then there are models that allOf ModelX, and do specify the discriminator value. That's still inheritance.
• (and this one is less certain) - There's a model (ModelY) with allOf parent that does specify the discriminator value (inheritance), and then are models that allOf ModelY. That's a composition of an inherited model. I'm not sure you want to deal with this use case for now and the meaning of it would be uncertain.

[whitlockjc]

When you receive an object that is specified by a schema with a `discriminator`
you would just look at the value of the `discriminator` field to determine the name
of the schema that should be used (hopefully not too much more complex that
what's already happening).

This is the part I don't understand. I fully understand polymorphism, I've written more Java than I care to admit, but I don't see how the value specified in the discriminator field points to any schema name. In the example we have now, its value is type. This doesn't point to any schema by reference or name and it appears to just indicate that the type field is used to differentiate ancestors.

All of this being said, it seems that any schema that uses a discriminator makes it so that when it is referenced in an allOf, the descendant schema can be used in place of the ancestor for validation purposes. I'm working with @webron on this right now. If you get on IRC again @daveismith, I should be there.

[webron]

So while discussing it directly with @whitlockjc, I wanted to use your example @daveismith to show him the usage, only to realize that I've misread it and my previous comment is incorrect.

There is an issue with the example and it cannot be used properly. The problem is that when using the discriminator, you have to specify the value of the discriminator in the extending model so that proper mapping can be made.

I have to say that this is an issue I've been wanting to discuss for a while now because it is not properly defined in the specification right now, and something is missing. I do have a workaround for it to ease things up but we need to open an official discussion over on swagger-spec to provide better clarification.

The discriminator is not there for decoration but is intended to be used by each extending model to specify what value needs to be there in order for the mapping to work properly (otherwise, how would you know that's the right 'class' for the model?). However, this makes the definition cumbersome, and following your example above, it should be something like:

{
    "Command": {
      "discriminator": "type",
      "properties": {
        "type": {
          "type": "string"
        },
        "propBase": {
          "type": "string"
        }
      },
      "required": ["type", "propBase"]
    },
    "Command1": {
      "description": "Command 1",
      "allOf": [
        {
          "$ref": "#/definitions/Command"
        },
        {
          "properties": {
            "prop1": {
              "type": "string"
            },
            "type": {
              "type": "string",
              "enum": [ "command1" ]
            }
          },
          "required": ["prop1"]
        }
      ]
    }
  }

As you can see, I had to duplicate the type definition, and restrict it to one value, which is the value of the discriminator as it is for that model. That's ugly, but required. However, right now, if it is not declared, then the definition pretty much breaks as the discriminator is meaningless.

I do have a couple of suggestions on how to overcome this (one implicit which can be done within the scope of the current spec and one explicit which would break the current spec). However, further discussion should be taken officially to the swagger-spec repo instead of having it here, and then we can return here for the actual implementation. This is solvable, but for organization and documentation sake, let's do it in the right place.

[whitlockjc]

That clears up my misunderstanding but it definitely doesn't simplify the implementation. ;) However, that's not your problems and I'm sure I can figure it out.

[webron]

Completely get it. But we should clarify it before you start implementing it, because we may decide to go a different way than the one I stated above. This is not documented in the spec.

[whitlockjc]

So let me run my understanding by you. So whenever a schema defines a discriminator property, any child schema that wants to be considered polymorphic would need to define a property definition for the name in the discriminator property on the parent. This contract is clear, if that is indeed the contract.

How do we do multiple levels of polymorphism?

[webron]

That's definitely an issue with it, but I'd rather we take the discussion to swagger-spec (for the reasons stated above). We can't resolve it here.

[whitlockjc]

Well, for the solution I need multiple levels of polymorphism shouldn't matter. I mean, a parameter or response can only be one base so as long as I find all descendants, it doesn't matter if any of the descendants are single or multiple polymorphic.

[webron]

Right, but we still need to decide on the implementation, so that's just the theory.

[whitlockjc]

For now, I have what I need. cc me on the swagger-spec stuff because I think we need an opinion on the matter that benefits more tools than just swagger-codegen.

[isadovskiy]

So what's the story with this topic?

I have an issue validating my json object with validateModel method. JSON object's type field contains valid name of the schema, which should be used to validate it. type field is also specified as discriminator for my base schema, which I'm using to validate this object.

As a result validateModel validates only fields, specifies in my base schema. But it ignores fiest, specified in the schema, which name if the value of the type field (the discriminator one). so owndering if it's an expected behavior or i'm doing somethin wrong?

[isadovskiy]

Any update on?

[dan-kez]

Also looking for an update on this. Was this ever implemented?

[foxel]

Is there any update on this issue?

[whitlockjc]

No update right now. I apologize but this isn't something likely to happen due to this project effectively being deprecated, see #335.