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

Add matching pattern to error in fields #76903

Merged
merged 17 commits into from
Sep 1, 2021
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,7 @@ Test nested field with sibling field resolving to DocValueFetcher:
- match: { hits.hits.0.fields.number.2 : 3 }
- match: { hits.hits.0.fields.number.3 : 5 }
- match: { hits.hits.0.fields.number.4 : 6 }

---
Test token_count inside nested field doesn't fail:
- skip:
Expand Down Expand Up @@ -892,3 +893,117 @@ Test token_count inside nested field doesn't fail:
body:
_source: false
fields: [ "*" ]

---
error includes field name:
- skip:
version: ' - 7.99.99'
reason: 'error changed in 8.0.0 to be backported to 7.15'

- do:
indices.create:
index: test
body:
settings:
index.number_of_shards: 1
mappings:
properties:
keyword:
type: keyword
date:
type: date

- do:
index:
index: test
id: 1
refresh: true
body:
keyword: "value"
date: "1990-12-29T22:30:00.000Z"

- do:
catch: '/error fetching \[keyword\]: Field \[keyword\] of type \[keyword\] doesn''t support formats./'
search:
index: test
body:
fields:
- field: keyword
format: "yyyy/MM/dd"

---
error includes glob pattern:
- skip:
version: ' - 7.99.99'
reason: 'error changed in 8.0.0 to be backported to 7.15'

- do:
indices.create:
index: test
body:
settings:
index.number_of_shards: 1
mappings:
properties:
dkeyword:
type: keyword
date:
type: date

- do:
index:
index: test
id: 1
refresh: true
body:
dkeyword: "value"
date: "1990-12-29T22:30:00.000Z"

- do:
catch: '/error fetching \[dkeyword\] which matched \[d\*\]: Field \[dkeyword\] of type \[keyword\] doesn''t support formats./'
search:
index: test
body:
fields:
- field: d*
format: "yyyy/MM/dd"


---
error for flattened includes whole path:
- skip:
version: ' - 7.99.99'
reason: 'error changed in 8.0.0 to be backported to 7.15'

- do:
indices.create:
index: test
body:
settings:
index.number_of_shards: 1
mappings:
properties:
flattened:
type: flattened

date:
type: date

- do:
index:
index: test
id: 1
refresh: true
body:
flattened:
foo: bar
date: "1990-12-29T22:30:00.000Z"

- do:
catch: '/error fetching \[flattened.bar\]: Field \[flattened.bar\] of type \[flattened\] doesn''t support formats./'
search:
index: test
body:
fields:
- field: flattened.bar
format: "yyyy/MM/dd"
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,17 @@ private static FieldFetcher create(SearchExecutionContext context,
}
// only add concrete fields if they are not beneath a known nested field
if (nestedParentPath == null) {
ValueFetcher valueFetcher = ft.valueFetcher(context, fieldAndFormat.format);
ValueFetcher valueFetcher;
try {
valueFetcher = ft.valueFetcher(context, fieldAndFormat.format);
} catch (IllegalArgumentException e) {
StringBuilder error = new StringBuilder("error fetching [").append(field).append(']');
if (isWildcardPattern) {
error.append(" which matched [").append(fieldAndFormat.field).append(']');
}
error.append(": ").append(e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of piecing exception messages together in this way (in particular because IllegalArgumentException is a generic exception).

Also, it's unclear to me how the change in this PR will affect other call sites of the valueFetcher method (those will now not return the field name anymore and might be less precise / harder to debug).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I were running this in production myself I wouldn't do it, no. But I think its useful because we drop the getMessage() from the top level cause into the reason field of the error by default. I really care that the reason describes exactly what failed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the other call sites - I believe there is only one non-test call that doesn't flow through here and it is in highlighting which calls the method in a way that won't throw. But the method is public and, who knows, highlighting may throw. I'll revert this and be happy with the more verbose message. It isn't worth that change.

throw new IllegalArgumentException(error.toString(), e);
}
fieldContexts.put(field, new FieldContext(field, valueFetcher));
}
}
Expand Down
3 changes: 3 additions & 0 deletions x-pack/qa/runtime-fields/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ subprojects {
'search.aggregation/20_terms/Global ordinals are loaded with the global_ordinals execution hint',
'search.aggregation/170_cardinality_metric/profiler string',
'search.aggregation/235_composite_sorted/*',
// The error messages are different
'search/330_fetch_fields/error includes field name',
'search/330_fetch_fields/error includes glob pattern',
/////// NOT SUPPORTED ///////
].join(',')
}
Expand Down