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): add findByForeignKeys function #3473

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

nabdelgadir
Copy link
Contributor

@nabdelgadir nabdelgadir commented Jul 30, 2019

Implemented initial version of the new helper function findByForeignKeys.

Resolves #3443.

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 👈

@nabdelgadir nabdelgadir force-pushed the find-by-foreign-keys branch 6 times, most recently from 642d5dc to 0d77fb9 Compare July 31, 2019 19:14
@nabdelgadir nabdelgadir changed the title Find by foreign keys feat(repository): add findByForeignKeys function Jul 31, 2019
@nabdelgadir nabdelgadir force-pushed the find-by-foreign-keys branch from 0d77fb9 to d37714a Compare July 31, 2019 19:16
@nabdelgadir nabdelgadir marked this pull request as ready for review July 31, 2019 19:35
>(
targetRepository: EntityCrudRepository<Target, TargetID, TargetRelations>,
fkName: StringKeyOf<Target>,
fkValues: ForeignKey[],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we accept a single value too? For example:

fkValues: ForeignKey[] | ForeignKey,

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have a type param such as FK extends StringKeyOf<Target> and use it to constrain fk values, such as Target[FK]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me reword it to make sure that I understand the type constrains in tsc correctly:

if we do foreignKey extends StringKeyOf<Target>, it would constrain the passed in foreignKey to be a string-like <Target> instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm proposing the following:

export async function findByForeignKeys<
  Target extends Entity,
  FK extends StringKeyOf<Target>,
  TargetRelations extends object,
>(
  targetRepository: EntityCrudRepository<Target, unknown, TargetRelations>,
  fkName: FK,
  fkValues: Target[FK][],
  scope?: Filter<Target>,
  options?: Options,
): Promise<(Target & TargetRelations)[]> {
}

@nabdelgadir nabdelgadir force-pushed the find-by-foreign-keys branch 3 times, most recently from f8645cc to af293c3 Compare August 2, 2019 12:19
@nabdelgadir nabdelgadir added feature Repository Issues related to @loopback/repository package labels Aug 2, 2019
}

const value = Array.isArray(fkValues)
? fkValues.length === 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a shortcut for empty array to return [] immediately. Include a test to cover this corner case too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we pass in an empty array, it complains that Type 'never[]' is not assignable to type 'number'. (or whatever the type of the foreign key value is)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can do the following:

const fkIds: number[] = []; 

return targetRepository.find(targetFilter, options);
}

export type StringKeyOf<T> = Extract<keyof T, string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not necessary to export this type?

@nabdelgadir nabdelgadir force-pushed the find-by-foreign-keys branch from af293c3 to d096fbd Compare August 2, 2019 17:24
Implemented initial version of the new helper function findByForeignKeys that finds model instances that contain any of the provided foreign key values.

Co-authored-by: Agnes Lin <[email protected]>
Co-authored-by: Miroslav Bajtoš <[email protected]>
@agnes512 agnes512 force-pushed the find-by-foreign-keys branch from a3a7796 to 149f1a3 Compare August 6, 2019 18:48
@agnes512 agnes512 merged commit f5eaf1d into master Aug 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the find-by-foreign-keys branch August 6, 2019 19:11
@agnes512 agnes512 mentioned this pull request Aug 7, 2019
7 tasks
@bajtos bajtos mentioned this pull request Sep 2, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Repository Issues related to @loopback/repository package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add findByForeignKeys helper (initial version)
3 participants