Skip to content

Conversation

@giovanniberti
Copy link

@giovanniberti giovanniberti commented Aug 9, 2023

Description

  • added checking for oneOf when defining object properties

Motivation and Context

This PR implements defining object properties with oneOf.
Property definition with oneOf is permitted by the OpenAPI spec but currently fails with an error during generation (note that this is a different case from a separate schema defined with oneOf).

Note that this PR:

  • does not add support for discriminator (which has more requirements, for example it cannot be used with inline schemas). Code generation doesn't fail with discriminator but no schema checking is made and no special type information about the discriminator is propagated to the generated code.
  • does not add support for defining object properties with allOf or anyOf (should we add it in this PR or in another one? I don't like very much supporting oneOf for properties but not the others)

How Has This Been Tested?

Manual inspection of generated snapshots used for testing

Screenshots (if appropriate):

Types of changes

  • Chore (improvement with no change in the behaviour)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@giovanniberti giovanniberti added the enhancement New feature or request label Aug 9, 2023
@giovanniberti giovanniberti self-assigned this Aug 9, 2023
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Jira Pull Request Link

This Pull Request refers to the following Jira issue CHK-1807

@giovanniberti giovanniberti marked this pull request as ready for review August 9, 2023 13:22
@giovanniberti giovanniberti requested a review from a team as a code owner August 9, 2023 13:22
@giovanniberti giovanniberti force-pushed the oneof-on-properties branch 3 times, most recently from 467e78e to 215ccde Compare August 9, 2023 13:38
Copy link

@Fernando-Granato Fernando-Granato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀 🚀

Copy link
Contributor

@gquadrati gquadrati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small comments

export const {{ definitionName }} =
t.union([
{% for schema in prop -%}
{% if schema.type == "object" or schema.type == "array" %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by the snapshot (specifically the generated constants named OneOfPropertyTestFieldsN like here)

Would you suggest a separate test case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I see from generated code is:

export const OneOfPropertyTestFields4 = t.readonlyArray(
  t.object,
  "array of object"
);

Inline array object definitions are not supported, unfortunately. :(

Comment on lines +353 to +369
it("should generate an object with a union property when that property has oneOf", async () => {
const definitonName = "OneOfPropertyTest";
const definition = getDefinitionOrFail(spec, definitonName);

const code = await renderDefinitionCode(
definitonName,
getParser(spec).parseDefinition(
// @ts-ignore
definition
),
false
);

expect(code).toContain("t.union");
expect(code).toMatchSnapshot("oneof-property-test");
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests in 4c70dee

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks! In addition to that, could you please include some scenarios where we expect things to go wrong? For example:

  const variant5 = {
    fields: [{ name: 42 }]
  };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants