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

Required inheritance and generated objects when using allOf #531

Closed
florentchauveau opened this issue Feb 23, 2022 · 3 comments
Closed

Required inheritance and generated objects when using allOf #531

florentchauveau opened this issue Feb 23, 2022 · 3 comments

Comments

@florentchauveau
Copy link
Contributor

Considering the following schema:

    FooProperties:
      type: object
      properties:
        uuid:
          type: string
          format: uuid
          readOnly: true
        firstname:
          type: string
        lastname:
          type: string

    Foo:
      allOf:
        - $ref: "#/components/schemas/FooProperties"
        - type: object
          required:
            - uuid
            - firstname
            - lastname

    NewFoo:
      allOf:
        - $ref: "#/components/schemas/FooProperties"
        - type: object
          required:
            - firstname

According to the specs, also confirmed in the https://editor.swagger.io/, and swagger-api/swagger-editor#1212, NewFoo should have firstname as required, and Foo should have all properties as required.

The validation works as expected.

However, the resulting objects in Go are:

type Foo struct {
	// Embedded struct due to allOf(#/components/schemas/FooProperties)
	FooProperties `yaml:",inline"`
	// Embedded fields due to inline allOf schema
}

// FooProperties defines model for FooProperties.
type FooProperties struct {
	Firstname *string `json:"firstname,omitempty"`
	Lastname  *string `json:"lastname,omitempty"`
	Uuid      *string `json:"uuid,omitempty"`
}

// NewFoo defines model for NewFoo.
type NewFoo struct {
	// Embedded struct due to allOf(#/components/schemas/FooProperties)
	FooProperties `yaml:",inline"`
	// Embedded fields due to inline allOf schema
}

Because of the struct embedding, the properties are all pointers instead of values (like when using "required" directly in objects).

Now, because the validation works as expected, this might not be an issue at all. I wonder however if in this case, if the generated code should embed FooProperties as is, or if a new struct should be generated per "merge".

This has been discussed, and sometimes fixed, in several other projects:

@florentchauveau florentchauveau changed the title Required inheritance missing using allOf Required inheritance and generated objects when using allOf Feb 23, 2022
@alex-broad alex-broad mentioned this issue Mar 8, 2022
2 tasks
@deepmap-marcinr
Copy link
Contributor

This is a hard one, and not a part of the spec that I was aware of when I was initially writing this code.

I think the right way to do it is to process "allOf" on the schema level, not on the Go struct level - meaning, we don't embed FooProperties as a struct inside Foo or NewFoo, but rather, generate a new struct where all fields are promoted to the higher level. However, you can't easily assign FooProperties into Foo anymore.

I didn't realize that allOf could modify the required property alone.

@florentchauveau
Copy link
Contributor Author

Maybe we can add a "merge strategy" option? Either embed, or create a new struct with values (according to the required fields).

@Swiftwork
Copy link

Thought I would share my temporary and convoluted solution using JavaScript to "transpile" a schema by flattening allOf and dereferencing objects with a custom x-deref attribute. I don't suggest you use this method but it might be hint as to how you can have both embedded and dereferenced types.

const fs = require("fs").promises;
const path = require("path");
const mergeAllOf = require("json-schema-merge-allof");
const yaml = require("js-yaml");
const refParser = require("@apidevtools/json-schema-ref-parser");
const { Command } = require("commander");
const program = new Command();

/** Recursively mutates and dereferences all objects with the property x-deref set to true
 * @param {Object} schema - The schema to dereference
 * @param {Object} dereferenced - The dereferenced schema
 */
async function dereference(schema, dereferenced) {
  Object.entries(schema).forEach(([key, value]) => {
    if (typeof value === "object" && value !== null) {
      // Work deepest to shallowest
      dereference(value, dereferenced[key]);
      if (value["x-deref"] === true) schema[key] = dereferenced[key];
      else if (value["x-deref"] === false) dereferenced[key] = value;
    }
  });
}

async function processOpenAPISpec(schema) {
  let dereferenced = JSON.parse(JSON.stringify(schema));
  await refParser.dereference(dereferenced);
  dereference(schema, dereferenced);

  function mergeAllOfRecursively(candidate, parent, k) {
    if (typeof candidate !== "object" || candidate === null) return;

    if (candidate.allOf) {
      parent[k] = mergeAllOf(candidate, {
        ignoreAdditionalProperties: true,
        deep: true,
        resolvers: {
          "x-go-type": mergeAllOf.options.resolvers.default,
        },
      });
    } else {
      for (const [key, value] of Object.entries(candidate)) {
        mergeAllOfRecursively(value, candidate, key);
      }
    }
  }

  for (const [key, value] of Object.entries(schema)) {
    mergeAllOfRecursively(value, schema, key);
  }

  return schema;
}

const main = async (options) => {
  let specYaml = await fs.readFile(path.resolve(options.input), "utf8");
  const spec = yaml.load(specYaml);
  const merged = await processOpenAPISpec(spec);
  if (path.extname(options.output) === ".yaml") {
    specYaml = yaml.dump(merged);
    fs.writeFile(path.resolve(options.output), specYaml, "utf8");
  } else {
    fs.writeFile(
      path.resolve(options.output),
      JSON.stringify(merged, null, 2),
      "utf8"
    );
  }
};

program
  .name("OpenAPI Spec Transformer")
  .description("Transfrom OpenAPI Spec dereferencing and merging allOf.")
  .version("0.1.0");

program
  .requiredOption("-i, --input <path>", "Path to the OpenAPI spec file.")
  .requiredOption("-o, --output <path>", "Path to the output file.")
  .action(main);

program.parse();

Example:

    User:
      title: User
      allOf:
        - type: object
          properties:
            id:
              $ref: '#/components/schemas/id'
        - $ref: '#/components/schemas/UserProperties'
        - type: object
          required:
            - id
            - email
            - name
            - photo
            - verified
      x-deref: true

adrianpk pushed a commit to foorester/oapi-codegen that referenced this issue Jan 16, 2024
I believe this fixes oapi-codegen#531.

The original way of implementing AllOf was too simple, and what
we need to do is merge the openapi3 definitions into a composite
object before generating code from it.

The code generated is incompatible with that previously generated,
so we're added a config file option and command line flag to revert
to the old behavior, but the new behavior is defaulted.
adrianpk added a commit to foorester/oapi-codegen that referenced this issue May 31, 2024
I believe this fixes oapi-codegen#531.

The original way of implementing AllOf was too simple, and what
we need to do is merge the openapi3 definitions into a composite
object before generating code from it.

The code generated is incompatible with that previously generated,
so we're added a config file option and command line flag to revert
to the old behavior, but the new behavior is defaulted.
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

No branches or pull requests

3 participants