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

Added test that parentId is not undefined when using fetchAll with relations #1769

Merged
merged 1 commit into from Mar 13, 2018

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Feb 21, 2018

no issue

Copy link
Member

@ricardograca ricardograca left a comment

Choose a reason for hiding this comment

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

Small problem. I just ran the proposed test against master and it passed correctly. Can you verify?

@kirrg001
Copy link
Contributor Author

It wasn't for me 🤔But i was on 0.10.3 when writing these tests. Maybe it is already fixed. Let me double check.

@ricardograca
Copy link
Member

Probably fixed in #1716.

@kirrg001
Copy link
Contributor Author

  1. Integration Tests Dialect: mysql Relations Bookshelf Relations Eager Loading - Collections eager loads "belongsToMany" models correctly and parent is not undefined:
    TypeError: Cannot read property 'should' of undefined
    at . (test/integration/relations.js:229:68)
    From previous event:
    at Context. (test/integration/relations.js:228:14)

So yeah on 0.10.3 this test fails when i remove my code adaptions.
But it does not on latest master. Then it was probably already fixed. I just wonder where 🤔I need to check the history.

We could still merge test improvements to assert that relatedData has the correct values.

@ricardograca
Copy link
Member

ricardograca commented Feb 21, 2018

Sure, merging just the test sounds good to me.

@kirrg001
Copy link
Contributor Author

Probably fixed in #1716.

Yeah maybe.

return new EagerRelation(this.models, response, new this.model()).fetch(options);

this.models are already instantiated models (the collection). I don't understand why we have to instantiate new this.model() empty as third parameter to then call target.belongsToMany.

Furthermore:

https://github.com/bookshelf/bookshelf/blob/master/src/base/eager.js#L52

This calls e.g. tags: function() { return this.belongsToMany(...); }
And the result is always relation.relatedData.parentId=undefined. So i think the fix which was merged happens too late?

@ricardograca
Copy link
Member

So i think the fix which was merged happens too late?

Could be. I never delved too deep into the relations code. Are you going to investigate? It's quite possible that your fix is better than the already merged one, in which case it should be replaced. The current test suite has a test in place for #629.

@kirrg001
Copy link
Contributor Author

Are you going to investigate?

Yes will do. But not before next week 👍Will report back here.
I don't mind which fix we take as long as it's the correct one :)

@kirrg001
Copy link
Contributor Author

kirrg001 commented Mar 5, 2018

I have not investigated much, but my gut feeling tells me to keep the fix in master as is.
Because my fix instantiates for every model in the collection a new eager relation (performance decrease). I am not saying my fix is incorrect, i think the problem is that the relation code needs to be reconsidered as a whole if we want to improve the code.

I'll just update my PR to check that the relational data is not undefined.

@kirrg001 kirrg001 changed the title Fixed parentId is undefined when using fetchAll with relations Added test that parentId is not undefined when using fetchAll with relations Mar 5, 2018
@ricardograca ricardograca added this to To Do in Version 0.13.0 via automation Mar 7, 2018
@ricardograca ricardograca moved this from To Do to In Progress in Version 0.13.0 Mar 7, 2018
@kirrg001 kirrg001 merged commit 9fad708 into bookshelf:master Mar 13, 2018
Version 0.13.0 automation moved this from In Progress to Done Mar 13, 2018
kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Mar 27, 2018
- i discovered two bookshelf bugs on 0.10.3
- bookshelf/bookshelf#1769
- bookshelf/bookshelf#1768
- we are currently locked to 0.10.3, because we saw a connection problem when updating to the latest knex+bookshelf
- using a tarball can trouble when installing deps
  - we saw this in the past
  - e.g. you have bookshelf already installed
  - now you switch to a tarball
  - neither yarn, nor npm are able to replace the dep
- require bookshelf-0.10.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants