Gi60s / openapi-enforcer

Apache License 2.0
94 stars 23 forks source link

Rules question: disallowed properties and multiple composites #157

Closed thomasheartman closed 1 year ago

thomasheartman commented 1 year ago

I'm working on a slightly complex schema and am having some issues with making it pass the enforcer. I was hoping you could be able to provide some insight into where the rules come from and maybe how to get around it. This is for OpenAPI 3.0.3.

Specifically, the issue relates to a schema that has multiple composites: one oneOf and one anyOf. If I try to validate that schema as is, I'm told that:

Cannot have multiple composites: anyOf, oneOf

Where does this rule come from? I couldn't find any reference to that in the swagger spec?

For clarity, the specific schema in question (with some non-important properties (description, example, etc) stripped):

Multiple composites ```json { "type": "object", "properties": { "expiresAt": { "type": "string" } }, "oneOf": [ { "required": [ "type" ], "properties": { "type": { "type": "string", "pattern": "^[Aa][Dd][Mm][Ii][Nn]$" } } }, { "required": [ "type" ], "properties": { "type": { "type": "string", "pattern": "^([Cc][Ll][Ii][Ee][Nn][Tt]|[Ff][Rr][Oo][Nn][Tt][Ee][Nn][Dd])$" }, "environment": { "type": "string" }, "project": { "type": "string" }, "projects": { "type": "array", "items": { "type": "string" } } } } ], "anyOf": [ { "required": [ "tokenName" ], "properties": { "tokenName": { "type": "string" } } }, { "required": [ "username" ], "properties": { "username": { "type": "string" } } } ] } ```

The key details are that: you must specify the type to be either "admin" or one of "client" or "frontend". If it's one of the latter two, then you can also provide "project" and "environment". These two extra properties are not valid for the "admin" type.

However, we also require you to provide either a username (which is deprecated) or a tokenName (preferred). Because at least one of these is required, we used anyOf to indicate that.

Optional solution

To get around this issue, I tried to instead nest the anyOf selector within each of the oneOf objects. However, when I do that, it tells me that:

Properties not allowed: properties, required [EDEV001]

That looks a bit like this:

Nested anyOf ```json { "type": "object", "properties": { "expiresAt": { "type": "string" } }, "oneOf": [ { "required": [ "type" ], "properties": { "type": { "type": "string", "pattern": "^[Aa][Dd][Mm][Ii][Nn]$" } }, "anyOf": [ { "required": [ "tokenName" ], "properties": { "tokenName": { "type": "string" } } }, { "required": [ "username" ], "properties": { "username": { "type": "string" } } } ] }, { "required": [ "type" ], "properties": { "type": { "type": "string", "pattern": "^([Cc][Ll][Ii][Ee][Nn][Tt]|[Ff][Rr][Oo][Nn][Tt][Ee][Nn][Dd])$" }, "environment": { "type": "string" }, "project": { "type": "string" }, "projects": { "type": "array", "items": { "type": "string" } } }, "anyOf": [ { "required": [ "tokenName" ], "properties": { "tokenName": { "type": "string" } } }, { "required": [ "username" ], "properties": { "username": { "type": "string" } } } ] } ] } ```

I'm not sure exactly why the properties are not allowed or what part of the spec they're referencing. I found this other issue on here to allow disallowed properties on specific instances, though I wasn't able to make it work (but that might be due to our conversion from typescript, so I can look into that). However, I'd be most interested in understanding exactly what this error is and why it's disallowed.

Because we don't want to disallow additional properties on incoming requests, this isn't quite as tight as it could be, but it's been working well so far.

Appreciate any input and guidance you may have.

Thanks πŸ˜„

Gi60s commented 1 year ago

Hello @thomasheartman and thank you for the issue.

For the first part of your question, the rule that you cannot have multiple composites came from a decision that I made based on what I could find. I haven't ever seen official documentation that explains whether multiple composites can exist as siblings in a schema. It seems to me that they conflict. If you know of documentation that explains that this is OK and how the schema should be interpreted in these cases then please do share and I'll work on correcting it.

The second example has problems due to a misunderstanding I had when I wrote the code for validating a schema. I was mistakenly under the impression that you could have a composite identifier (allOf, anyOf, oneOf) only if you didn't have other properties as siblings. So the error you're seeing there is due to the properties required and properties being siblings of a composite type. I'm working on version 2 of this library which will resolve that. It's probably also possible that I could retrofit version 1 to fix that.

Below is a possible solution for now, although it's not as elegant. Let me know what you think.

{
    "openapi": "3.0.3",
    "info": { "title": "", "version": "" },
    "paths": {},
    "components": {
        "schemas": {
            "ComplexSchema": {
                "oneOf": [
                    {
                        "allOf": [
                            { "$ref": "#/components/schemas/Admin" },
                            { "$ref": "#/components/schemas/TokenCred" }
                        ]
                    },
                    {
                        "allOf": [
                            { "$ref": "#/components/schemas/Admin" },
                            { "$ref": "#/components/schemas/UserNameCred" }
                        ]
                    },
                    {
                        "allOf": [
                            { "$ref": "#/components/schemas/Client" },
                            { "$ref": "#/components/schemas/TokenCred" }
                        ]
                    },
                    {
                        "allOf": [
                            { "$ref": "#/components/schemas/Client" },
                            { "$ref": "#/components/schemas/UserNameCred" }
                        ]
                    }
                ]
            },
            "Admin": {
                "type": "object",
                "required": ["type"],
                "properties": {
                    "expiresAt": {
                        "type": "string"
                    },
                    "type": {
                        "type": "string",
                        "pattern": "^[Aa][Dd][Mm][Ii][Nn]$"
                    }
                }
            },
            "Client": {
                "type": "object",
                "required": ["type"],
                "properties": {
                    "expiresAt": {
                        "type": "string"
                    },
                    "type": {
                        "type": "string",
                        "pattern": "^([Cc][Ll][Ii][Ee][Nn][Tt]|[Ff][Rr][Oo][Nn][Tt][Ee][Nn][Dd])$"
                    },
                    "environment": {
                        "type": "string"
                    },
                    "project": {
                        "type": "string"
                    },
                    "projects": {
                        "type": "array",
                        "items": {
                            "type": "string"
                        }
                    }
                }
            },
            "TokenCred": {
                "required": [
                    "tokenName"
                ],
                "properties": {
                    "tokenName": {
                        "type": "string"
                    }
                }
            },
            "UserNameCred": {
                "required": [
                    "username"
                ],
                "properties": {
                    "username": {
                        "type": "string"
                    }
                }
            }
        }
    }
}
thomasheartman commented 1 year ago

Hey, @Gi60s and thanks for the quick response πŸ™πŸΌ

the rule that you cannot have multiple composites came from a decision that I made based on what I could find. I haven't ever seen official documentation that explains whether multiple composites can exist as siblings in a schema. It seems to me that they conflict. If you know of documentation that explains that this is OK and how the schema should be interpreted in these cases then please do share and I'll work on correcting it.

Thanks for the clarification! I've never seen any documents explicitly saying that it's fine, but I haven't seen anything that disallows it either, so I just figured it'd be okay: if you can't do something, then I'd expect the spec to mention it.

I also noticed that this error doesn't have a code that you can use to skip it, is there a reason for that?

The second example has problems due to a misunderstanding I had when I wrote the code for validating a schema. I was mistakenly under the impression that you could have a composite identifier (allOf, anyOf, oneOf) only if you didn't have other properties as siblings.

Ah, right! But it works fine on the top level, though. Why would that be? From what I can tell, they're the same, but maybe there's a subtle difference I'm not seeing?

Below is a possible solution for now, although it's not as elegant. Let me know what you think.

Thanks a bunch πŸ˜„ Yeah, that seems to work swimmingly πŸ’― It's a bit more code, but it appears to work as expected, so that's great!