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(repository): CRUD implementation and tests for hasManyThrough Repository #5674

Merged
merged 2 commits into from
Jun 17, 2020

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented Jun 5, 2020

continuation of #4438 and #2359

Add implementation and tests for CRUD operations.

Methods link and unlink need more discussions.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

constrainDataObject(dataObject, targetConstraint),
constrainWhere(where, targetConstraint as Where<TargetEntity>),
options,
);
}

async link(
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'd like to bring up discussion here for link and unlink before making any further changes.

IMO link and unlink are for creating/deleting the through instances i.e no target instances should be involved.
If so, the question is how can we make sure the target exists? Or is it users' responsibility?

Copy link
Contributor

@jannyHou jannyHou Jun 15, 2020

Choose a reason for hiding this comment

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

My take is we should check target instance exists, if not throw error properly.
Not necessarily in the first implementation but make sure we create a story for it.

Copy link
Member

Choose a reason for hiding this comment

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

Can we leave link and unlink out of scope of this pull request and open a follow-up issue to discuss the desired behavior and implement it?

@agnes512 agnes512 force-pushed the hmt-crud branch 2 times, most recently from f8da5c2 to a92a875 Compare June 5, 2020 16:02
@agnes512 agnes512 added the Relations Model relations (has many, etc.) label Jun 5, 2020
@agnes512 agnes512 self-assigned this Jun 5, 2020
@agnes512 agnes512 added this to the Jun 2020 milestone Jun 5, 2020
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Awesome! I left a few comments. Looks good in general.

@agnes512 agnes512 force-pushed the hmt-crud branch 2 times, most recently from 641e19a to d527f2f Compare June 8, 2020 15:01
const throughRepository = await this.getThroughRepository();
const throughConstraint = this.getThroughConstraint();
const throughInstances = await throughRepository.find(
constrainFilter(undefined, throughConstraint),
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, why the first param is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there is no original filter to apply to the through repo. Here's the API docs:

export function constrainFilter<T extends object>(
  originalFilter: Filter<T> | undefined,
  constraint: AnyObject,
): Filter<T> 
...

I think either {} or undefined works.

constrainDataObject(dataObject, targetConstraint),
constrainWhere(where, targetConstraint as Where<TargetEntity>),
options,
);
}

async link(
Copy link
Contributor

@jannyHou jannyHou Jun 15, 2020

Choose a reason for hiding this comment

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

My take is we should check target instance exists, if not throw error properly.
Not necessarily in the first implementation but make sure we create a story for it.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great work! I reviewed the changes at high level, they look pretty good!

I have few comments to discuss, PTAL below 👇

constrainDataObject(dataObject, targetConstraint),
constrainWhere(where, targetConstraint as Where<TargetEntity>),
options,
);
}

async link(
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave link and unlink out of scope of this pull request and open a follow-up issue to discuss the desired behavior and implement it?

@agnes512 agnes512 force-pushed the hmt-crud branch 2 times, most recently from ce40dcb to e6c57f5 Compare June 16, 2020 15:38
@hacksparrow
Copy link
Contributor

Let's use a repo name based on the model instead of hasManyThroughRepo, like how we do for the rest of the relations. Simply using hasManyThroughRepo is confusing and feels like it is a magical mind-reading repo.

@hacksparrow
Copy link
Contributor

LGTM now.

Please open an issue to re-consider the key names throughData and throughOptions:

  create(
    targetModelData: DataObject<Target>,
    options?: Options & {
      throughData?: DataObject<Through>;
      throughOptions?: Options;
    },
  ): Promise<Target>;

my suggestion is:

  create(
    targetModelData: DataObject<Target>,
    options?: Options & {
      through?: DataObject<Through>;
      options?: Options;
    },
  ): Promise<Target>;

We can do this after everything lands.

@agnes512 agnes512 merged commit a6cf16e into master Jun 17, 2020
@agnes512 agnes512 deleted the hmt-crud branch June 17, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Relations Model relations (has many, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants