fix(joins): allow null optional nested references in object arrays on update#2852
fix(joins): allow null optional nested references in object arrays on update#2852tharropoulos wants to merge 2 commits intotypesense:v31from
Conversation
… update - skip null child reference values in object-array reference handling when the field is optional - add a regression test for patch-style updates on nested optional references
- allow internal nested reference helper arrays to validate both signed and unsigned json integers within the uint32 range - add tests for optional nested refs on update and async null/unresolved mixed object arrays
|
@happy-san Please review. |
| #include "validator.h" | ||
| #include "field.h" | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
@tharropoulos Is there a reason for having namespace here?
There was a problem hiding this comment.
Fair point. The unnamed namespace is only there to keep is_valid_uint32_json_integer file specific. Should I just change it to a static for consistency?
| ASSERT_EQ(0, doc["relatedassetsdata"][0].count("childobjectid")); | ||
| } | ||
|
|
||
| TEST_F(CollectionJoinTest, UpdateOptionalNestedReferenceInObjectArrayViaPatchPathOnCodeField) { |
There was a problem hiding this comment.
This test passes even without the changes in this PR. Could you check if we're testing the change correctly?
There was a problem hiding this comment.
I tried tightening it a few ways to mirror the failing cases more closely, like switching to the single-document UPDATE path, changing null -> valid code reference, and adding multiple object-array slots so the helper field had to record a non-zero object index, but it still passed without the fix.
At this point it seems this code-field variant is not targeting the broken path the PR fixes, while the two tests that do fail without the seem to be the real regressions.
So I agree the current test is not validating the change correctly, and I'd treat it as general coverage rather than evidence for the fix unless you had a specific pre-fix repro in mind for that variant.
EDIT: readability changes
There was a problem hiding this comment.
I think I found the reason, could you check if this test hits the else if (optional && object_array[i].at(keys[1]).is_null()) condition at line 278.
There was a problem hiding this comment.
Okay. Let's keep this test as is. The issue was only with the id field reference in an object array then.
|
@kishorenc apart from the |
Change Summary
helper arrays
PR Checklist