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

Add __parent reference to embedded models #1834

Merged
merged 1 commit into from Apr 17, 2020
Merged

Add __parent reference to embedded models #1834

merged 1 commit into from Apr 17, 2020

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Apr 17, 2020

This pull request supersedes #1787 by @mitsos1os. I cleaned up the commit history and also tweaked code formatting (white space) in few places.

Add a new hidden property __parent that's automatically set on all instances of embedded models.

For backwards compatibility, this feature is not enabled by default. You can turn it on by adding the following line to server/server.js file:

app.registry.modelBuilder.settings.parentRef = true;

Resolve #1787

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
  • Commit messages are following our guidelines

@bajtos bajtos added the community-contribution Patches contributed by community label Apr 17, 2020
@bajtos bajtos self-assigned this Apr 17, 2020
@bajtos
Copy link
Member Author

bajtos commented Apr 17, 2020

Here are the test failures I am observing in the PostgreSQL, Cloudant and MongoDB connectors:

1) juggler-v4
     basic-querying
       find
         check __parent relationship in embedded models
           should fill the parent in embedded model:
   TypeError: user.should.have.property(...).which.has.property(...).which.is.instanceof(...).and.equals is not a function
    at Context.it (node_modules/loopback-datasource-juggler/test/basic-querying.test.js:968:33)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)

2) juggler-v4
     basic-querying
       find
         check __parent relationship in embedded models
           should assign the container model as parent in list property:
   TypeError: user.should.have.property(...).which.has.property(...).which.is.instanceof(...).and.equals is not a function
    at Context.it (node_modules/loopback-datasource-juggler/test/basic-querying.test.js:973:33)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)

3) juggler-v4
     basic-querying
       find
         check __parent relationship in embedded models
           should have the complete chain of parents available in embedded list element:
   TypeError: userFriend.should.have.property(...).which.equals is not a function
    at user.friends.forEach (node_modules/loopback-datasource-juggler/test/basic-querying.test.js:978:61)
    at Array.forEach (<anonymous>)
    at Context.it (node_modules/loopback-datasource-juggler/test/basic-querying.test.js:977:22)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)

@bajtos
Copy link
Member Author

bajtos commented Apr 17, 2020

@slnode test please

@bajtos
Copy link
Member Author

bajtos commented Apr 17, 2020

Trying to drop support for Node.js 8.x in the PostgreSQL connector to see if it makes any difference: loopbackio/loopback-connector-postgresql#431

@bajtos
Copy link
Member Author

bajtos commented Apr 17, 2020

Hmm, by removing support for Node.js 8.x from the PostgreSQL connector I have effectively disabled dowstream testing :(

Let's wait for @rmg to update our downstream builders to use Node.js 12.

@bajtos
Copy link
Member Author

bajtos commented Apr 17, 2020

@slnode test please

@bajtos bajtos force-pushed the embedded-parent2 branch 3 times, most recently from 013c279 to 5dc158c Compare April 17, 2020 11:17
@@ -962,20 +962,24 @@ describe('basic-querying', function() {

describe('check __parent relationship in embedded models', () => {
createTestSetupForParentRef(() => User.modelBuilder);
// eslint-disable-next-line mocha/no-exclusive-tests
Copy link
Member Author

Choose a reason for hiding this comment

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

Forgotten comment, will remove.

@bajtos
Copy link
Member Author

bajtos commented Apr 17, 2020

Looks like 5dc158c fixed downstream tests, yay 馃暫 I am going to clean up the commit history to run the final CI build and prepare this PR for landing.

Add a new hidden property `__parent` that's automatically set on all
instances of embedded models.

For backwards compatibility, this feature is not enabled by default.
You can turn it on by adding the following line to `server/server.js`
file:

    app.registry.modelBuilder.settings.parentRef = true;
@mitsos1os
Copy link
Contributor

@bajtos kudos for finding the problem! Changes are good as far as I am concerned! Feel free to move on with the PR!

@bajtos bajtos merged commit e026b4c into master Apr 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the embedded-parent2 branch April 17, 2020 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Patches contributed by community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants