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

fix(core): fix nested query with fk as pk #2650

Merged
merged 3 commits into from
Jan 20, 2022

Conversation

ml1nk
Copy link
Contributor

@ml1nk ml1nk commented Jan 19, 2022

Some tests are broken but at least the cases of #2649 are working as expected. Both changes break tests separately and i only have a rough idea why.

@B4nan I try to improve this fix tomorrow, but i'm missing insight.

@ml1nk ml1nk force-pushed the issue2648-fix branch 2 times, most recently from dfa866f to 82ca875 Compare January 20, 2022 15:30
packages/core/src/utils/QueryHelper.ts Outdated Show resolved Hide resolved
packages/core/src/utils/QueryHelper.ts Outdated Show resolved Hide resolved
packages/knex/src/query/ObjectCriteriaNode.ts Outdated Show resolved Hide resolved
@B4nan
Copy link
Member

B4nan commented Jan 20, 2022

let's not amend/force push after you get a review. this makes reviewing the changes almost impossible to me, as I only see the whole commit and not the changes you did after i reviewed it

@ml1nk
Copy link
Contributor Author

ml1nk commented Jan 20, 2022

let's not amend/force push after you get a review. this makes reviewing the changes almost impossible to me, as I only see the whole commit and not the changes you did after i reviewed it

I will stop amending, sorry for creating more work for you.

Any idea how to decrease the complexity to solve the codeclimate issue?

@B4nan
Copy link
Member

B4nan commented Jan 20, 2022

Any idea how to decrease the complexity to solve the codeclimate issue?

You could move some of the logic to new method to distribute the complexity.

But dont be too concerned with codeclimate, as long as we will have 100% branch coverage I will be happy to merge. (note that we dont have it, one line is not covered by tests currently)

@B4nan B4nan changed the title fix(GH-2648): deep smart query with fk as pk fix(core): fix nested query with fk as pk Jan 20, 2022
@B4nan B4nan merged commit cc54ff9 into mikro-orm:master Jan 20, 2022
@B4nan
Copy link
Member

B4nan commented Jan 20, 2022

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants