Raise error when unrecognized content token type is found while indexing #19097
Conversation
Signed-off-by: Bruce Hong <[email protected]>
Signed-off-by: Bruce Hong <[email protected]>
Signed-off-by: Bruce Hong <[email protected]>
Signed-off-by: Bruce Hong <[email protected]>
|
❌ Gradle check result for ac73676: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Bruce Hong <[email protected]>
bruce-hong-glean
force-pushed
the
fix-false-allow-templates-edge-case
branch
from
ac73676 to
af8219f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #19097 +/- ##
============================================
+ Coverage 72.79% 72.83% +0.03%
- Complexity 69605 69683 +78
============================================
Files 5658 5658
Lines 320079 320078 -1
Branches 46345 46345
============================================
+ Hits 232996 233114 +118
+ Misses 68230 68085 -145
- Partials 18853 18879 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Bruce Hong <[email protected]>
Signed-off-by: Bruce Hong <[email protected]>
Signed-off-by: Bruce Hong <[email protected]>
bruce-hong-glean
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta and
Rishikesh1159
as code owners
bruce-hong-glean
requested review from
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah and
a team
as code owners
| @@ -886,7 +886,6 @@ private static Mapper.Builder<?> createBuilderFromDynamicValue( | |||
| if (builder != null) { | |||
| return builder; | |||
| } | |||
| return handleNoTemplateFound(dynamic, () -> null); | |||
There was a problem hiding this comment.
Could this be convenient to add some unit tests or yaml tests?
There was a problem hiding this comment.
I'm struggling to come up with a test for this case. This should only trigger if the field does not match and does not match any of the XContentParser.Token types. It falls back to using a string parser which has to fail to go down this codepath, and I haven't been able to trigger that with public functions. Maybe with reflection but it seems kind of gnarly. Any suggestions?
There was a problem hiding this comment.
@gaobinlong Looking at the diff of the previous change, it seems like this return statement should not be here. I also don't have any good suggestions for how to trigger this. What do you think?
Signed-off-by: Bruce Hong <[email protected]>
…com/bruce-hong-glean/OpenSearch into fix-false-allow-templates-edge-case
Signed-off-by: Bruce Hong <[email protected]>
|
❌ Gradle check result for f87c059: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Bruce Hong <[email protected]>
|
^ wanted to bump this. Wasn't really sure what a good solution to the remaining comment was. cc @andrross as well since you helped merge the original PR |
|
❕ Gradle check result for 57d5304: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Description
We merged this PR last week to introduce
false_allow_templatesas an indexing option. Taking another pass after it was merged, I realized it changed some behaviour in other indexing cases too (where there in an unrecognized content token). This should probably throw an error regardless of indexing rules and this returns to status quo before the PR above.Related Issues
Not sure how strict this is. Let me know if I should create an issue for this.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.