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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(repository): implement inclusion resolver helpers #3727

Merged
merged 1 commit into from Sep 17, 2019

Conversation

@agnes512
Copy link
Contributor

agnes512 commented Sep 13, 2019

connects to #3447, #3448, #3449

  • Add basic helpers for inclusion resolver

  • Add unit tests

  • Use self-defined objectId in repository folder to simulate MondoDB environment to test isBsonType, deduplicate, normalizeKey, buildLookupMap.

  • Unifies unit test files name

  • Refactor tests for findByForeignKeys:
    we were testing it with real repos, which it was problematic. For example, we had code

{inq: fkValues} 

instead of {inq: [...fkValues]} before. The wrong version passed in the old unit test that ran on real
repositories. But when we were implementing hasMany inclusion resolver, we realized that
findByForeignKeys wasn't correct.
Now with stub repositories, we are able to check the queries it sends out, which is difficult to reproduce with real repositories.

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 馃憟

@agnes512 agnes512 marked this pull request as ready for review Sep 13, 2019
@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Sep 13, 2019
Copy link
Member

bajtos left a comment

The patch looks much better now, let's improve it even more.

}
assert(Array.isArray(input), 'array argument is required');

const comparableA = input.map(item =>

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 13, 2019

Member

comparableA - is A an abbreviation for "array"? We try to avoid abbreviations in names, they make to code difficult to read.

There is no need to encode the type in the variable name, we have type system to capture that information. Hungarian notation is considered as anti-pattern.

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 13, 2019

Author Contributor

I convert the type back to T because I misunderstood the intention of this helper. I thought it would convert the type if it's ObjectId.

packages/repository/src/relations/relation.helpers.ts Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the resolver-unit-test branch from 004990d to 5e23aa6 Sep 13, 2019
packages/repository/src/relations/relation.helpers.ts Outdated Show resolved Hide resolved
* checkd of the valus is BsonType (mongodb)
* @param value
*/
export function isBsonType(value: unknown): value is object {

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 13, 2019

Contributor

Just a suggestion: If you want to use type bson, it could be imported from a module called bson, like https://github.com/strongloop/loopback-connector-mongodb/blob/c5200e26498ffcd2b560e3a3cf8ba2795bbf603f/lib/mongodb.js#L11

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 16, 2019

Author Contributor

@jannyHou I update the api documentation of this function. Does it look good to you?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 17, 2019

Contributor

Cool 馃憤 @agnes512

@agnes512 agnes512 force-pushed the resolver-unit-test branch 3 times, most recently from ecbd1fd to b438b49 Sep 13, 2019
@agnes512 agnes512 force-pushed the resolver-unit-test branch 3 times, most recently from 3951584 to d10477d Sep 14, 2019
Copy link
Contributor

nabdelgadir left a comment

LGTM 馃憤

@agnes512 agnes512 force-pushed the resolver-unit-test branch 4 times, most recently from 1ec8c67 to 4c5458b Sep 17, 2019
@bajtos
bajtos approved these changes Sep 17, 2019
Copy link
Member

bajtos left a comment

LGTM with a minor comment to consider.

assert(Array.isArray(input), 'array argument is required');

const comparableArray = input.map(item =>
isBsonType(item) ? item.toString() : item,

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 17, 2019

Member

Can we replace this lambda function with normalizeKey? IIUC, it is the same code.

@agnes512 agnes512 force-pushed the resolver-unit-test branch from 4c5458b to c0edf69 Sep 17, 2019
Copy link
Contributor

jannyHou left a comment

馃憦 LGTM!

@agnes512 agnes512 force-pushed the resolver-unit-test branch from c0edf69 to d94442e Sep 17, 2019
@agnes512 agnes512 force-pushed the resolver-unit-test branch from d94442e to 3165d54 Sep 17, 2019
@agnes512 agnes512 merged commit 7bf6f12 into master Sep 17, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 91.594%
Details
@agnes512 agnes512 deleted the resolver-unit-test branch Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.