optic capture failing due to discriminated union represented as oneOf with enums

[sennyeya]

Describe the bug
I'm trying to write a new spec for a Backstage plugin that uses discriminated union types rendered as oneOf. I'm getting an AJV validationg error where Optic is using AJV to validate its own patched schema, which I think means that the patched schema is invalid. All of the code that I outline below can be found in this PR, here. Basically, I'm trying to use oneOf and enums to differentiate between two separate types. From what I can tell with json-schema-to-ts, my spec is correct, but I'm getting a few weird errors from AJV surfaced through Optic,

1. enum contains multiple values that are the same
2. value that shouldn't be an array is not an array
3. value doesn't match schema from anyOf
   Full error messages below.

The types in question are,

type ConditionalPolicyDecision = {
  result: AuthorizeResult.CONDITIONAL;
  pluginId: string;
  resourceType: string;
  conditions: PermissionCriteria<PermissionCondition>;
};

type DefinitivePolicyDecision = {
  result: AuthorizeResult.ALLOW | AuthorizeResult.DENY;
}

export type PolicyDecision =
  | DefinitivePolicyDecision
  | ConditionalPolicyDecision;

So far, my JSON schema (YAML) looks like this,

  DefinitivePolicyDecision:
      type: object
      properties:
        result:
          type: string
          enum:
            - ALLOW
            - DENY
        id:
          type: string
      required:
        - result
        - id
      additionalProperties: false

    ConditionalPolicyDecision:
      type: object
      properties:
        result:
          type: string
          enum:
            - CONDITIONAL
        pluginId:
          type: string
        resourceType:
          type: string
        conditions:
          $ref: '#/components/schemas/PermissionCriteria'
        id:
          type: string
      required:
        - result
        - id
        - pluginId
        - resourceType
        - conditions
      additionalProperties: true

    PolicyDecision:
      oneOf:
        - $ref: '#/components/schemas/ConditionalPolicyDecision'
        - $ref: '#/components/schemas/DefinitivePolicyDecision'

This results in the following schema (added a console.dir here),

optic/projects/optic/src/commands/capture/patches/patchers/shapes/diff.ts

Line 301 in 8001d02

const schema: SchemaObject = JSON.parse(JSON.stringify(input));

{
  type: 'object',
  properties: {
    items: {
      type: 'array',
      items: {
        oneOf: [
          {
            type: 'object',
            properties: {
              result: {
                type: 'string',
                enum: [ 'CONDITIONAL', 'ALLOW', 'ALLOW', 'DENY', 'DENY' ]
              },
              pluginId: { type: 'string' },
              resourceType: { type: 'string' },
              conditions: {
                oneOf: [
                  {
                    type: 'object',
                    properties: {
                      allOf: {
                        type: 'array',
                        items: {
                          type: 'object',
                          properties: {
                            resourceType: { type: 'string' },
                            rule: { type: 'string' },
                            params: {
                              type: 'object',
                              additionalProperties: {
                                oneOf: [
                                  {
                                    oneOf: [
                                      { type: 'string' },
                                      { type: 'number' },
                                      {
                                        type: 'boolean',
                                        nullable: true
                                      }
                                    ]
                                  },
                                  {
                                    type: 'array',
                                    items: {
                                      oneOf: [
                                        { type: 'string' },
                                        { type: 'number' },
                                        {
                                          type: 'boolean',
                                          nullable: true
                                        }
                                      ]
                                    }
                                  }
                                ]
                              }
                            }
                          },
                          required: [ 'resourceType', 'rule' ],
                          additionalProperties: true
                        },
                        minItems: 1
                      }
                    },
                    additionalProperties: true
                  },
                  {
                    type: 'object',
                    properties: {
                      anyOf: {
                        type: 'array',
                        items: {
                          type: 'object',
                          properties: {
                            resourceType: { type: 'string' },
                            rule: { type: 'string' },
                            params: {
                              type: 'object',
                              additionalProperties: {
                                oneOf: [
                                  {
                                    oneOf: [
                                      { type: 'string' },
                                      { type: 'number' },
                                      {
                                        type: 'boolean',
                                        nullable: true
                                      }
                                    ]
                                  },
                                  {
                                    type: 'array',
                                    items: {
                                      oneOf: [
                                        { type: 'string' },
                                        { type: 'number' },
                                        {
                                          type: 'boolean',
                                          nullable: true
                                        }
                                      ]
                                    }
                                  }
                                ]
                              }
                            }
                          },
                          required: [ 'resourceType', 'rule' ],
                          additionalProperties: true
                        },
                        minItems: 1
                      }
                    },
                    additionalProperties: true
                  },
                  {
                    type: 'object',
                    properties: {
                      not: {
                        type: 'array',
                        items: {
                          type: 'object',
                          properties: {
                            resourceType: { type: 'string' },
                            rule: { type: 'string' },
                            params: {
                              type: 'object',
                              additionalProperties: {
                                oneOf: [
                                  {
                                    oneOf: [
                                      { type: 'string' },
                                      { type: 'number' },
                                      {
                                        type: 'boolean',
                                        nullable: true
                                      }
                                    ]
                                  },
                                  {
                                    type: 'array',
                                    items: {
                                      oneOf: [
                                        { type: 'string' },
                                        { type: 'number' },
                                        {
                                          type: 'boolean',
                                          nullable: true
                                        }
                                      ]
                                    }
                                  }
                                ]
                              }
                            }
                          },
                          required: [ 'resourceType', 'rule' ],
                          additionalProperties: true
                        },
                        minItems: 1
                      }
                    },
                    additionalProperties: true
                  },
                  {
                    type: 'object',
                    properties: {
                      resourceType: { type: 'string' },
                      rule: { type: 'string' },
                      params: {
                        type: 'object',
                        additionalProperties: {
                          oneOf: [
                            {
                              oneOf: [
                                { type: 'string' },
                                { type: 'number' },
                                { type: 'boolean', nullable: true }
                              ]
                            },
                            {
                              type: 'array',
                              items: {
                                oneOf: [
                                  { type: 'string' },
                                  { type: 'number' },
                                  { type: 'boolean', nullable: true }
                                ]
                              }
                            }
                          ]
                        }
                      }
                    },
                    required: [ 'resourceType', 'rule' ],
                    additionalProperties: true
                  }
                ]
              },
              id: { type: 'string' }
            },
            required: [ 'result', 'id' ],
            additionalProperties: true
          },
          {
            type: 'object',
            properties: {
              result: {
                type: 'string',
                enum: [ 'ALLOW', 'DENY', 'CONDITIONAL' ]
              },
              id: { type: 'string' },
              pluginId: { type: 'string' },
              resourceType: { type: 'string' },
              conditions: {
                type: 'object',
                properties: {
                  rule: { type: 'string' },
                  params: { type: 'array', items: { type: 'string' } }
                },
                required: [ 'rule', 'params' ]
              }
            },
            required: [ 'result', 'id' ],
            additionalProperties: false
          }
        ]
      }
    }
  },
  required: [ 'items' ],
  additionalProperties: false
}

which throws an error during AJV compilation,

Error: schema is invalid: data/properties/items/items/oneOf/0/properties/result/enum must NOT have duplicate items (items ## 3 and 4 are identical), data/properties/items/items must be array, data/properties/items/items must match a schema in anyOf
    at Ajv.validateSchema (/Users/sennyeya/.asdf/installs/nodejs/19.0.1/lib/node_modules/@useoptic/optic/node_modules/ajv/dist/core.js:266:23)
    at Ajv._addSchema (/Users/sennyeya/.asdf/installs/nodejs/19.0.1/lib/node_modules/@useoptic/optic/node_modules/ajv/dist/core.js:460:18)
    at Ajv.compile (/Users/sennyeya/.asdf/installs/nodejs/19.0.1/lib/node_modules/@useoptic/optic/node_modules/ajv/dist/core.js:158:26)
    at ShapeDiffTraverser.traverse
...

Taking a look at the logs, this request causes part of the enum error by adding the "ALLOW" and "DENY" values twice. The other 2 errors I'm not sure about.

{
  request: {
    host: 'localhost:8001',
    method: 'post',
    path: '/authorize',
    body: {
      contentType: 'application/json',
      body: '{"items":[{"id":"123","permission":{"type":"resource","name":"test.permission.1","resourceType":"test-resource-1","attributes":{}},"resourceRef":"resource:1"},{"id":"234","permission":{"type":"resource","name":"test.permission.2","resourceType":"test-resource-2","attributes":{}},"resourceRef":"resource:2"},{"id":"345","permission":{"type":"resource","name":"test.permission.3","resourceType":"test-resource-1","attributes":{}},"resourceRef":"resource:3"},{"id":"456","permission":{"type":"resource","name":"test.permission.4","resourceType":"test-resource-2","attributes":{}},"resourceRef":"resource:4"}]}',
      size: 607
    },
    headers: [],
    query: []
  },
  response: {
    statusCode: '200',
    body: {
      contentType: 'application/json; charset=utf-8',
      body: '{"items":[{"id":"123","result":"ALLOW"},{"id":"234","result":"ALLOW"},{"id":"345","result":"DENY"},{"id":"456","result":"DENY"}]}',
      size: 129
    },
    headers: []
  }
}

Internal optic schema diff after running the above request,

--- old.json	2024-01-11 16:39:58
+++ new.json	2024-01-11 16:01:40
@@ -8,7 +8,10 @@
           {
             "type": "object",
             "properties": {
-              "result": { "type": "string", "enum": ["CONDITIONAL"] },
+              "result": {
+                "type": "string",
+                "enum": ["CONDITIONAL", "ALLOW", "ALLOW", "DENY", "DENY"]
+              },
               "pluginId": { "type": "string" },
               "resourceType": { "type": "string" },
               "conditions": {
@@ -179,13 +182,7 @@
               },
               "id": { "type": "string" }
             },
-            "required": [
-              "result",
-              "id",
-              "pluginId",
-              "resourceType",
-              "conditions"
-            ],
+            "required": ["result", "id"],
             "additionalProperties": true
           },
           {
@@ -207,13 +204,7 @@
                 "required": ["rule", "params"]
               }
             },
-            "required": [
-              "result",
-              "id",
-              "pluginId",
-              "resourceType",
-              "conditions"
-            ],
+            "required": ["result", "id"],
             "additionalProperties": false
           }
         ]

To Reproduce
See my PR here. I'm running PORT=8001 optic capture src/schema/openapi.yaml --server-override http://localhost:8001 in plugins/permission-backend to get the output seen above.

Expected behavior
An error is not thrown and the schema validates as expected.

Details (please complete the following information):

• OS and version: [e.g. Mac OS 13.1] MacOS 13.0.1
• Optic version: [e.g. v0.37.1] 0.52 and 0.53.20
• NodeJS version: [e.g. 18.0.0] 19.0.1

[niclim]

I have a PR up to fix the duplicate enum items being generated by optic capture #2659 (releasing a pre-release 0.53.22-0).

I'm still trying to recreate the errors your getting and I'm not sure I understand the other 2 errors from AJV. Very weirdly I'm not getting an error with something like this (where I'd expect the duplicate enum error), both with this script and through my own Optic CLI (local AJV version is 8.12.0)

import Ajv from 'ajv/dist/2019';

const ajv = new Ajv({
  allErrors: true,
  validateFormats: false,
  strictSchema: false,
  strictTypes: false,
  useDefaults: true,
});

const validator = ajv.compile(prepareSchemaForDiff(schema));

schema:
  type: object
  properties:
    name:
      type: string
      enum: ['a','a','a']

I'm going to keep digging - but if you could try out the prerelease and see if that helps, and if you could check what AJV version the backstage repo is using that would be helpful!

[sennyeya]

@niclim Thanks for looking into this! Just tried it and no longer getting the error. I wonder if it was just related to the enum definition? Seems odd though.

Still getting a weird patch though,

     DefinitivePolicyDecision:
       type: object
       properties:
@@ -226,11 +225,31 @@ components:
           enum:
             - ALLOW
             - DENY
+            - CONDITIONAL
         id:
           type: string
+        pluginId:
+          type: string
+        resourceType:
+          type: string
+        conditions:
+          type: object
+          properties:
+            rule:
+              type: string
+            params:
+              type: array
+              items:
+                type: string
+          required:
+            - rule
+            - params
       required:
         - result
         - id
+        - pluginId
+        - resourceType
+        - conditions
       additionalProperties: false
 
     ConditionalPolicyDecision:
@@ -240,6 +259,8 @@ components:
           type: string
           enum:
             - CONDITIONAL
+            - ALLOW
+            - DENY
         pluginId:
           type: string
         resourceType:
@@ -251,9 +272,6 @@ components:
       required:
         - result
         - id
-        - pluginId
-        - resourceType
-        - conditions
       additionalProperties: true

This is definitely closer, but the enum is still being spread across both oneOfs and the required types are assigned to the first object not the second. ideally, no patch would be generated here.

We're using 8.12.0 as the AJV version in Backstage as well.

[sennyeya]

I just realized that I had additionalProperties: true on ConditionalPolicyDecision, see above last line of the object. Adjusting it back to false still gives me the same errors.

[niclim]

Ok this looks like an issue with how we patch oneOf. I tested out a minimal example ({items: [{id: '123', result: 'allow'}, {id: '234', result: 'conditional'...}]) if the interaction matches the schema, it produces no diffs, but when there is a diff between the interaction, it creates patches for every instance of the oneOf (i.e. it'll make every option conform to the interaction it just saw)

We'll need to fix this by looking at our patching code and choose the most relevant branch to then patch using some sort of heuristic... I think we'll need to fix something here and maybe we can use our closeness heuristic but modified for interactions rather than schemas

For the meantime, I think if you manually adjust the schema to match whatever the returned value of the interaction, it should still correctly validate that the request / responses match the oneOf blocks, it'll just patch every branch of the oneOf. Not sure what interaction is causing the patch you mentioned above though

[sennyeya]

Thanks! Sorry for the slow update here. I was able to get this working by using 'anyOf' instead of 'oneOf'. That also fixed some issues I was having with a separate AJV validator. I'll take a look and see if I can add the fix, I'd like to also add support for object access query parameters, (object[id]) and this would be a good chance to get familiar with the code base.