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): declare x-expanded-relations - Store #3482

Merged
merged 10 commits into from
Mar 16, 2023

Conversation

patrick-medusajs
Copy link
Contributor

@patrick-medusajs patrick-medusajs commented Mar 15, 2023

What

Declare x-expanded-relations on store responses

Why

Allows our codegen to alter generated types to better represent the shape of models included in responses when the backend augments the models with relations and/or calculated totals.

How

  • Consider defaultRelations in route bootstrapping. Include as relations.
  • Consider relations passes to services in controllers. Move to a defaultRelations const. Include as relations.
  • Consider relations in service sub-processes. Include as implicit.
  • Consider eager relations in TypeORM models. Include as eager.
  • Consider decoratedTotals fields. Include as totals

Extra scope:

  • Fix responses that are not respecting OAS response contract.

Tests

  • Run yarn install
  • Run yarn build
  • Open generated types in packages/generated/client-types/src/lib/models
  • Expect types decorate with x-expanded-relations to be mutated using SetRelation.
  • Expect Merge for be used for deeply nested relations.

@patrick-medusajs patrick-medusajs self-assigned this Mar 15, 2023
@vercel
Copy link

vercel bot commented Mar 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
medusa-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 15, 2023 at 8:39PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Mar 15, 2023

🦋 Changeset detected

Latest commit: 1620375

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

This PR includes changesets to release 3 packages
Name Type
@medusajs/client-types Patch
@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: highlighting noteworthy code changes

@@ -91,7 +92,7 @@ export default async (req, res) => {

const customerService: CustomerService = req.scope.resolve("customerService")
const customer = await customerService.retrieve(result.customer?.id || "", {
relations: ["orders", "orders.items"],
relations: defaultRelations,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will also include shipping_addresses in order to harmonize the responses shaped with get-session.

@@ -22,11 +22,12 @@ export default (app) => {
return app
}

export const defaultStoreCollectionRelations = ["products"]
export const defaultStoreCollectionRelations = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultStoreCollectionRelations is not used and is not passed to services in controllers. To avoid confusion, product has been moves to allowedFields. This change should have no effect.

Comment on lines +16 to +23
* name: handle
* style: form
* explode: false
* description: Filter by the collection handle
* schema:
* type: array
* items:
* type: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing OAS for query param.

export class StoreGetCollectionsParams {
@IsOptional()
@IsArray()
handle?: string[]

Comment on lines +178 to +191
* implicit:
* - items
* - items.variant
* - items.variant.product
* - items.tax_lines
* - items.adjustments
* - gift_cards
* - discounts
* - discounts.rule
* - shipping_methods
* - shipping_methods.tax_lines
* - shipping_address
* - region
* - region.tax_rates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relations from decorateTotals subprocess.

private getTotalsRelations(config: FindConfig<Cart>): string[] {
const relationSet = new Set(config.relations)
relationSet.add("items")
relationSet.add("items.variant")
relationSet.add("items.variant.product")
relationSet.add("items.tax_lines")
relationSet.add("items.adjustments")
relationSet.add("gift_cards")
relationSet.add("discounts")
relationSet.add("discounts.rule")
relationSet.add("shipping_methods")
relationSet.add("shipping_methods.tax_lines")
relationSet.add("shipping_address")
relationSet.add("region")
relationSet.add("region.tax_rates")
return Array.from(relationSet.values())
}

@@ -124,9 +129,103 @@ export type StoreCustomersRes = {
customer: Omit<Customer, "password_hash">
}

/**
* @schema StoreCustomersResetPasswordRes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

customer model in reset password response will not include expanded-relations from StoreCustomersRes. Creating a separate response for reset password endpoint.

Comment on lines +28 to +31
* relations:
* - prices
* - options
* - product
Copy link
Contributor Author

Choose a reason for hiding this comment

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

export const defaultStoreVariantRelations = ["prices", "options", "product"]

Comment on lines +47 to +50
* relations:
* - prices
* - options
* - product
Copy link
Contributor Author

Choose a reason for hiding this comment

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

export const defaultStoreVariantRelations = ["prices", "options", "product"]

* required:
* - variant
* properties:
* variant:
* $ref: "#/components/schemas/PricedVariant"
*/
export type StoreVariantsRes = {
variant: ProductVariant
variant: PricedVariant
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Controller calls PricingService.setVariantPrices

@@ -46,7 +57,7 @@ export type StoreVariantsRes = {
* $ref: "#/components/schemas/PricedVariant"
*/
export type StoreVariantsListRes = {
variants: ProductVariant[]
variants: PricedVariant[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Controller calls PricingService.setVariantPrices

@@ -31,6 +31,35 @@ export type ShippingOptionPricing = {
tax_amount: number
}

/** @schema PricedShippingOption
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missing OAS

export type ShippingOptionPricing = {
price_incl_tax: number | null
tax_rates: TaxServiceRate[] | null
tax_amount: number
}

@patrick-medusajs patrick-medusajs marked this pull request as ready for review March 15, 2023 14:33
@patrick-medusajs patrick-medusajs requested a review from a team as a code owner March 15, 2023 14:33
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!

Not sure if you already considered this, so feel free to ignore if you did: maybe we should consider creating something like a schema in OAS for relations (for example, defaultRelations). The only reason I'm thinking about that is ensuring the solution is maintainable in the long run.

Comment on lines +39 to +61
* - type: object
* properties:
* price_incl_tax:
* type: number
* description: Price including taxes
* tax_rates:
* type: array
* description: An array of applied tax rates
* items:
* type: object
* properties:
* rate:
* type: number
* description: The tax rate value
* name:
* type: string
* description: The name of the tax rate
* code:
* type: string
* description: The code of the tax rate
* tax_amount:
* type: number
* description: The taxes applied.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: should we specify anything as required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so. I followed the same pattern as PriceVariant.

@patrick-medusajs
Copy link
Contributor Author

Not sure if you already considered this, so feel free to ignore if you did: maybe we should consider creating something like a schema in OAS for relations (for example, defaultRelations). The only reason I'm thinking about that is ensuring the solution is maintainable in the long run.

I hear you. Maintainability was top of mind when designing this solution. OAS not as a good as TypeScript when it comes to mutating definitions using composition. In the end, I settled on using the same definition pattern as the one used by the controllers, services, and repositories. I believe we will be able to improvement maintainability by fully deprecating eager loaded relations and also by making implicit relations more explicit. For example, decoratedTotals relations could be exposed as an array that can then be explicitly merged with const defaultRelations = ["foobar", ...totalsRelations].

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

Nice work @patrick-medusajs!

export const allowedFields = [
"id",
"title",
"handle",
"product",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"product",
"products",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 Fixed in 9945163

@@ -37,7 +37,7 @@ import { PaymentCollectionService } from "../../../../services"
* tags:
* - Payment Collections
* responses:
* 200:
* 207:
Copy link
Contributor

@olivermrbl olivermrbl Mar 15, 2023

Choose a reason for hiding this comment

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

todo: This must have slipped through at some point. We don't use any other success codes than 200 and 201. I'll create a follow-up ticket.

Unless you want to tackle it now? I believe its a minor change.

@@ -72,7 +72,7 @@ export default async (req, res) => {
req.request_context
)

res.status(207).json({ payment_collection })
res.status(200).json({ payment_collection })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olivermrbl Fixed

@olivermrbl olivermrbl merged commit 522e306 into develop Mar 16, 2023
@olivermrbl olivermrbl deleted the feat/x-expanded-relations-store branch March 16, 2023 08:08
@patrick-medusajs
Copy link
Contributor Author

We can see the result of the these changes on the generated types on this commit (0bee564) from a future PR.

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

Successfully merging this pull request may close these issues.

3 participants