COMPASS-541 Filter lost when inserting a new document #694
Conversation
pzrq
changed the title
|
Looks like something is toggling the indexes sort order when an index is created, completely unrelated to this change as it also broke master earlier. EDIT: COMPASS-564: #695 |
pzrq
force-pushed
the
COMPASS-541-insert-plus-filter
branch
from
c1132ec
to
e28c2c6
Compare
|
LGTM, though it seems like the functional tests don't pass (but this issue is also in master) and needs a rebase. |
|
There are these warnings though that were introduced (not from this PR) |
Start by adding functional tests to cover the filtered flows.
As there may be other clients interacting with the same mongod, so we can't just increment the count "this.state.count" as was done previously. Implemented by having the ResetDocumentListStore keep track of the lastKnownFilter. Note: Robomongo also does this.
So you are likely to see the document you just inserted. At least until mongodb decides to handle: https://en.wikipedia.org/wiki/Year_2038_problem
pzrq
force-pushed
the
COMPASS-541-insert-plus-filter
branch
from
e28c2c6
to
5298dda
Compare
I can’t see a way to easily delete it, and the index usage will definitely be different.
pzrq
force-pushed
the
COMPASS-541-insert-plus-filter
branch
from
5298dda
to
0ada848
Compare
There was a problem hiding this comment.
First of all: I can't reproduce the original issue from COMPASS-541 anymore on master, which was "filter lost when inserting a new document". So this may be a case of "gone away".
The following additional changes were introduced here:
- sort order is
{_id: -1}now, instead of{_id: 1} - after inserting a doc, the store resets and reloads a new set of documents.
1. was done so that a newly inserted document appears at the top and is visible to the user immediately. This only works a) until we introduce custom sort order, b) and only if the _id is larger than all previous _ids (which works for ObjectId but may not work for other types). It's also a deviation from shell behavior, which sorts in ascending order. I think the benefits are not strong enough here to justify the behavior change, especially when we let users sort on custom fields.
2. was done so that the count updates correctly in all cases and shows or doesn't show the new document correctly (as we cannot assume that the document matches the current query). However, it has the side-effect that it also resets the list of documents, and moves the scroll position back to the top. The user may have scrolled to a certain position for a reason and we should assume they wanted to remain there, since inserting documents is a modal action and should not change the screen state underneath the modal.
Determining on the client side if the new document matches the query or not is difficult however (although there are node modules that immitate the query matcher). So here I'm on the fence, but open to using this new behavior.
@samweaver, @durran any comments on this?
| sort: [[ '_id', 1 ]], | ||
| // Default to reverse chronological order so newly inserted | ||
| // documents are shown at the top near the insert button | ||
| sort: [[ '_id', -1 ]], |
There was a problem hiding this comment.
While this has the side-effect of showing new documents at the top, it's also an unexpected behavior change from prior versions and users coming from the shell may find this unintuitive (because there the documents return in ascending order). Also, soon the user can set an explicit sort, so the problem would reoccur.
I prefer staying consistent with the shell behavior here. @samweaver, @durran any thoughts?
There was a problem hiding this comment.
We should stay consistent with the shell and return ascending order. As you say, once we have sort order this doesn't matter anyway.
There was a problem hiding this comment.
- Is interesting. The way that mongochef does this is to insert the new document to the bottom of the view, then jump the scroll to the bottom of the view. They don't update the count, they simply do "X documents + Y added". Where X is the original number returned by the query.
There was a problem hiding this comment.
If we reset the store, can we call an action to jump to the bottom of the doc view? Once we have user defined sort, we detect -1 or 1 to decide whether to jump to bottom of view or not.
| sort: [[ '_id', 1 ]], | ||
| // Default to reverse chronological order so newly inserted | ||
| // documents are shown at the top near the insert button | ||
| sort: [[ '_id', -1 ]], |
|
I agree with @rueckstiess ' comments... I was under the impression we would introduce something like Sift and just check if the document matched the current filter or not before adding it to the list. |
|
Pending feedback from @samweaver or designers about how Compass should behave, which may ultimately end up blocked on user-configurable sort (+skip/limit/project which is currently the story in COMPASS-412). Going to close this for now as we have this PR and GitHub should retain the branch after deleting it, on the off-chance that this specific implementation ends up being how Compass should behave or if whoever picks up COMPASS-541 next decides to use it as inspiration to start on their fix. |
|
Left some comments. |
No description provided.