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

issue with modifying unselected subdocs #9427

Closed
vkarpov15 opened this issue Sep 17, 2020 · 2 comments
Closed

issue with modifying unselected subdocs #9427

vkarpov15 opened this issue Sep 17, 2020 · 2 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@vkarpov15
Copy link
Collaborator

vkarpov15 commented Sep 17, 2020

Copied from #4651 (comment) from @dietergeerts

I just encounter this bug in version 5.9.16. Does this mean that this got reverted, and when doing changes in a sub-document, it has to be selected in the first place?

I wrote a plugin that does some extra changes based on other changes. The plugin is sure that the sub-documents exist, and it wants to update 1 specific field. Though using this.set completely overwrites the whole sub-document, which is not expected. Even though it was not specifically selected, and it conforms to the schema, I would expect only that path to be updated, not the whole sub-document. I must say that this is in a sub-document array.

A workaround I've found that makes my unit tests green is:

  // This document array is not selected, and has existing data.
  // It is not selected, because this change will not always happen.
  // This plugin only wants to update 1 specific field, and leave all others as-is.
  mapping.screens.forEach((screen, index) => {
    this.set(`mainStage.screens.${index}.activeView`, screen.views[0]);
  });
  this.unmarkModified('mainStage.screens');

Though I don't know if using unmarkModified will have some side-effects that we don't want?

Imho, it makes sense to set certain paths that aren't selected, but are defined in the schema. If any error happens because the data doesn't exist, then it's our own fault off course, and then the unhappy business logic path wasn't taken into account.

@vkarpov15
Copy link
Collaborator Author

@dietergeerts can you please add code samples that demonstrates the issue you're seeing? Also, why is the subdocument deselected? Perhaps you could work around this issue by projecting the subdocument in for the code in question?

The issue is that, if the field is deselected, Mongoose doesn't have a previous value to diff against, so it doesn't know what changed. That's why Mongoose is overwriting the entire subdocument.

@vkarpov15 vkarpov15 added the needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity label Sep 18, 2020
@vkarpov15 vkarpov15 removed this from the 5.10.6 milestone Sep 18, 2020
@dietergeerts
Copy link

@vkarpov15 , as the code about is in a middleware, it doesn't know what will be selected. It could add its own selects when a query is done, but then these selects will always be done, even if those fields doesn't need to change. The changes done on the data by the plugin aren't always done, it depends on other data being changed, and as we tested the performance of mongoose and selects, we noticed that it performs so much quicker the less selected paths there are. That's why at first, we didn't want to select these paths if they aren't always changed.

A possible solution would be to have an option with .set, where one could tell that it is OK for that path to not have been selected.

@vkarpov15 vkarpov15 added this to the 5.10.10 milestone Oct 21, 2020
@vkarpov15 vkarpov15 added needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue and removed needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity labels Oct 21, 2020
@vkarpov15 vkarpov15 modified the milestones: 5.10.10, 5.10.11 Oct 23, 2020
vkarpov15 added a commit that referenced this issue Oct 23, 2020
@vkarpov15 vkarpov15 added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

2 participants