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: Fix Mongodb delete by ObjectId. Closes #6552 #6553

Merged
merged 3 commits into from
Oct 7, 2020

Conversation

danmana
Copy link
Contributor

@danmana danmana commented Aug 12, 2020

No description provided.

@imnotjames
Copy link
Contributor

imnotjames commented Oct 3, 2020

As I understand it - without this change only the second of the two test cases pass?

EDIT: Ah, not even that, ok.

@imnotjames
Copy link
Contributor

imnotjames commented Oct 3, 2020

Can you add the case of the previous required behavior? You'd mentioned it in the #6552

The workaround is to use repository.delete({_id: post._id}) which works correctly.

If you add that to detect any regressions I think this is more or less good to go.

@danmana
Copy link
Contributor Author

danmana commented Oct 5, 2020

@imnotjames I added the regression test as suggested.

You are correct, before this fix:

  • test 1: failed (deleted wrong entity)
  • test 2: failed (didn't delete anything)
  • test 3: passed

@imnotjames imnotjames self-requested a review October 5, 2020 08:50
@imnotjames imnotjames merged commit e37eb1e into typeorm:master Oct 7, 2020
@imnotjames
Copy link
Contributor

Thanks for the contribution!

@danmana danmana deleted the 6552-delete-by-object-id branch October 7, 2020 12:59
@imnotjames imnotjames added the hacktoberfest-accepted label hacktoberfest label Oct 9, 2020
zaro pushed a commit to zaro/typeorm that referenced this pull request Jan 12, 2021
* fix: Fix Mongodb delete by ObjectId. Closes typeorm#6552

* add regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted label hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants