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

feat(oas) - accurate model OAS representation - A to D #3203

Merged
merged 5 commits into from Feb 8, 2023

Conversation

patrick-medusajs
Copy link
Contributor

Scope

Models A to D

What

Refactor OAS for models to accurately represent their shape in API responses.

Why

About 33% of model fields are not accurately represented in the OAS. Most of the issues are:

  • fields that can not be omitted in the response are not declared as required
  • fields that could return null as their value are not declared as nullable: true

When using a code generator, these OAS issues would lead to inaccurate response shapes in the generated client.

How

nullable

Fields meeting at least one of the following condition will be represented as nullable: true in OAS:

  • The field is decorated with @Column({ nullable: true })
  • The field is decorated with @OneToOne, @ManyToOne
  • The field is decorated with @DeleteDateColumn

optional

Fields meeting at least one of the following conditions will never be listed as required in OAS and will be considered optional and could be omitted in the response:

  • The field is decorated with @OneToOne, @ManyToOne, @OneToMany, @ManyToMany
  • The field is decorated with @FeatureFlagColumn
  • The field is decorated with @Column({select: false})
  • The field is representing dynamic values not persisted in the database

Fields not meeting any of the conditions above will be declared as required and are expected to be present in the response.

Test

  • Ran OAS validator.
  • Ran docs build script.

Expect OAS changes to be reflected in the API documentation.

@patrick-medusajs patrick-medusajs self-assigned this Feb 8, 2023
@changeset-bot
Copy link

changeset-bot bot commented Feb 8, 2023

🦋 Changeset detected

Latest commit: a8fa726

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/medusa Patch
@medusajs/inventory Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor Author

@patrick-medusajs patrick-medusajs left a comment

Choose a reason for hiding this comment

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

self-review: I'd recommend using the Model TS refactor PR to facilitate the review process. I've also create a demo PR to showcase the generate types from the OAS.

@patrick-medusajs
Copy link
Contributor Author

Has a dependency on #3198 getting merged in order for Redocly to not crash when rendering the circular references in this PR.

@patrick-medusajs patrick-medusajs marked this pull request as ready for review February 8, 2023 02:40
@patrick-medusajs patrick-medusajs requested a review from a team as a code owner February 8, 2023 02:40
Copy link
Member

@shahednasser shahednasser left a comment

Choose a reason for hiding this comment

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

Added a couple of comments. I also want to test out how it looks like in the documentation after merging the circular reference PR, since it's crashing now.

packages/medusa/src/models/batch-job.ts Show resolved Hide resolved
packages/medusa/src/models/cart.ts Show resolved Hide resolved
Copy link
Member

@shahednasser shahednasser left a comment

Choose a reason for hiding this comment

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

LGTM! I also tested the documentation and it works well

@kodiakhq kodiakhq bot merged commit 4d3210b into develop Feb 8, 2023
@kodiakhq kodiakhq bot deleted the feat/model-oas-refactor-AtoD branch February 8, 2023 17:00
adrien2p pushed a commit that referenced this pull request Feb 13, 2023
### Scope

Models A to D

### What

Refactor OAS for models to accurately represent their shape in API responses.

### Why

About 33% of model fields are not accurately represented in the OAS. Most of the issues are:
- fields that can not be omitted in the response are not declared as `required`
- fields that could return `null` as their value are not declared as `nullable: true`

When using a code generator, these OAS issues would lead to inaccurate response shapes in the generated client.

### How

#### nullable
Fields meeting at least one of the following condition will be represented as `nullable: true` in OAS:
* The field is decorated with `@Column({ nullable: true })`
* The field is decorated with `@OneToOne`, `@ManyToOne`
* The field is decorated with `@DeleteDateColumn`

#### optional
Fields meeting at least one of the following conditions will never be listed as `required` in OAS and will be considered optional and could be omitted in the response:
* The field is decorated with `@OneToOne`, `@ManyToOne`, `@OneToMany`, `@ManyToMany`
* The field is decorated with `@FeatureFlagColumn`
* The field is decorated with `@Column({select: false})`
* The field is representing dynamic values not persisted in the database

Fields not meeting any of the conditions above will be declared as `required` and are expected to be present in the response.

### Test
* Ran OAS validator.
* Ran docs build script.

Expect OAS changes to be reflected in the API documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants