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

fix: fix wrong accumulator return type for initializeModel function #396

Merged

Conversation

zxl629
Copy link
Contributor

@zxl629 zxl629 commented Nov 15, 2024

Related issues:

Description of changes:

This change adds a defined-behavior integration test for initializeModel function. The test fails since the function has a wrong return type when it accumulates sort keys from data.

The change then fixes the return type and passes the test.

Example of why we need this:

If customers have two data models Order and Customer, where Order has reference fields that match Customer's identifier, and a belongsTo relationship field with Customer :

Order: a.model({
    id: a.id(),
    customerFirstName: a.string().required(),
    customerLastName: a.string().required(),
    customer: a.belongsTo('Customer', ['customerFirstName','customerLastName']),
  }).authorization(allow => [allow.publicApiKey()])

 Customer: a.model({
    customerFirstName: a.string().required(),
    customerLastName: a.string().required(),
    orders: a.hasMany('Order', ['customerFirstName','customerLastName']),
  }).identifier(['customerFirstName', 'customerLastName']).authorization(allow => [allow.publicApiKey()]),

when they try to lazy load the customer data from an order:

  const { data: order } = await client.models.Order.get({
    id: 'order-id',
  });
  const { data: customer } = await order!.customer();

they will get the following error: Uncaught (in promise) TypeError

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

changeset-bot bot commented Nov 15, 2024

🦋 Changeset detected

Latest commit: 156b6d0

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

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

@zxl629 zxl629 marked this pull request as ready for review November 20, 2024 00:43
@zxl629 zxl629 requested review from a team as code owners November 20, 2024 00:43
@zxl629 zxl629 force-pushed the lzhouq/fix/accumulator-wrong-return-type branch from eec25ec to 699aedc Compare November 20, 2024 18:10
Copy link
Member

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

Looks good overall. One nit

Comment on lines 57 to 58
describe('when related data model has a composite key', ()=>{
test('belongsTo: related data is queried with foreign keys', async() => {
Copy link
Member

Choose a reason for hiding this comment

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

describe blocks are helpful when grouping tests together, especially when we need to do some setup or teardown related to some shared grouping/testcase relationship.

In cases like this, I might just use a test without the wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I have removed the describe() wrapper.

@zxl629 zxl629 requested a review from stocaaro November 20, 2024 19:52

type CompositeKeySchema = ClientSchema<typeof compositeKeySchema>;

test('belongsTo: related data is queried with foreign keys', async() => {
Copy link
Member

Choose a reason for hiding this comment

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

Just want to confirm: This test fails in the absence of the change?

@zxl629 zxl629 merged commit 068f8af into aws-amplify:main Nov 20, 2024
7 checks passed
@zxl629 zxl629 deleted the lzhouq/fix/accumulator-wrong-return-type branch November 20, 2024 21:59
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