[SERVER-62322] Consistent validity treatment of empty objects (i.e., `{}`) Created: 30/Dec/21 Updated: 29/Oct/23 Resolved: 25/Jan/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 5.3.0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Mohammad Dashti (Inactive) | Assignee: | Nicholas Zolnierz |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | auto-reverted | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||
| Backport Requested: |
v5.2
|
||||||||||||||||||||
| Participants: | |||||||||||||||||||||
| Linked BF Score: | 173 | ||||||||||||||||||||
| Description |
|
Currently, there are two places in the code (i.e., here and here) that empty objects are strictly rejected in a user/intermediate document via assertions with this message: "an empty object is not a valid value". As empty objects are valid values, the reason for these assertions in the abovementioned code segments is unclear. Now, we have two options in front of us: 1- Keep these assertions: In this case, we need to make sure that valid user documents are cleaned before getting to these assertions (and make sure they don't have empty objects at any level). In addition, we need to make sure that no empty object is produced as an intermediate document (i.e., a result of previous stages). Here's an example for the latter:
Running this program on 5.2.0-rc2@4fab480 produces this output:
As you observe, an empty object is produced in the first result. Now, if we run this program instead (which adds a $setWindowFields at the end of pipeline):
Running the program produces the following error:
2- Remove these assertions and fix the rest of the code that depends on this assumption (i.e., there's no empty object in the input). IMO, with enough caution, taking the second option makes more sense. anton.korshunov as it seems that you have added these assertions, it'd be great if you could advise on the best way to deal with this issue. |
| Comments |
| Comment by Githook User [ 03/Feb/22 ] |
|
Author: {'name': 'HanaPearlman', 'email': 'hana.pearlman@mongodb.com', 'username': 'HanaPearlman'}Message: TIG-3800: Multiversion fuzzer should ignore differences in output after |
| Comment by Githook User [ 21/Jan/22 ] |
|
Author: {'name': 'Nicholas Zolnierz', 'email': 'nicholas.zolnierz@mongodb.com', 'username': 'nzolnierzmdb'}Message: |
| Comment by xgen-buildbaron-user [ 21/Jan/22 ] |
|
Ticket re-opened due to revert. aggregation began a consistent failure of jstests/aggregation/sources/fill/fill_and_densify.js,jstests/aggregation/sources/setWindowFields/locf.js |
| Comment by Githook User [ 21/Jan/22 ] |
|
Author: {'name': 'auto-revert-processor', 'email': 'dev-prod-dag@mongodb.com'}Message: Revert " This reverts commit 2dac3294841b0374a94a5823f3726d5d02e27261. |
| Comment by Nicholas Zolnierz [ 20/Jan/22 ] |
|
Note that the fix was isolated to $setWindowFields so the title/description may be misleading. |
| Comment by Githook User [ 20/Jan/22 ] |
|
Author: {'name': 'Nicholas Zolnierz', 'email': 'nicholas.zolnierz@mongodb.com', 'username': 'nzolnierzmdb'}Message: |
| Comment by Githook User [ 19/Jan/22 ] |
|
Author: {'name': 'Nicholas Zolnierz', 'email': 'nicholas.zolnierz@mongodb.com', 'username': 'nzolnierzmdb'}Message: |
| Comment by Mohammad Dashti (Inactive) [ 13/Jan/22 ] |
|
nicholas.zolnierz Thanks for looking into this ticket and giving some pointers to the related issues in the past. wrapping the projected field in a $literal sounds like a working solution. Although, my initial instinct was to fix the code re-use of projection machinery in $setWindowFields by adding some flag to the projection code to disable its syntax checks in this case (or any similar code- re-use that involves user data). |
| Comment by Nicholas Zolnierz [ 13/Jan/22 ] |
|
This has come up a few times in the past ( Also note that |
| Comment by Joe Kanaan [ 13/Jan/22 ] |
|
nicholas.zolnierz, can you take a look at this please? |
| Comment by Mohammad Dashti (Inactive) [ 06/Jan/22 ] |
|
david.percy I think the initial purpose was to assert the syntax and "disallow `{$project: {}}` because it's unclear whether it's an inclusion or exclusion projection.", as you've suggested. However, that code has been re-used in other places (e.g., in the implementation of setWindowFields)), where input user documents are treated as the syntax, e.g., similar to the examples in the ticket description. |
| Comment by David Percy [ 06/Jan/22 ] |
|
I thought those assertions are about syntax, not values. I think we disallow `{$project: {}}` because it's unclear whether it's an inclusion or exclusion projection. Maybe the bug is that we're generating that invalid syntax internally, during some optimization (or in parsing, desugaring, or serialization). |
| Comment by Joe Kanaan [ 06/Jan/22 ] |
|
anton.korshunov, can you please take a look at this? Thanks |