Skip to content

Fix/meta fields bad request #117229

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

Merged
merged 28 commits into from
Dec 3, 2024
Merged

Conversation

drempapis
Copy link
Contributor

In this pr, a 400 error is returned when _source / _seq_no / _feature / _nested_path / _field_names is requested, rather a 5xx

Solves #107136

@drempapis drempapis added >bug auto-backport Automatically create backport pull requests when merged Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations v9.0.0 v8.17.0 labels Nov 21, 2024
@drempapis drempapis self-assigned this Nov 21, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine
Copy link
Collaborator

Hi @drempapis, I've created a changelog YAML for you.

@@ -130,16 +130,71 @@ fetch _seq_no via stored_fields:
fetch _seq_no via fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of these tests require proper skip due to bwc and the fact that the error has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. I tried to skip or requires based on this documentation, but I was not able to make it work. Do you have any idea how to "skip" this test? Maybe by using capabilities?

Copy link
Contributor

@salvatore-campagna salvatore-campagna Nov 22, 2024

Choose a reason for hiding this comment

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

You can look for instance at MapperFeatures and how we use NodeFeature to mark introduction of changes (i.e. "mapper.logsdb_default_ignore_dynamic_beyond_limit"). Make sure you only use those for testing using them in getTestFeatures.
For instance this test rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/logsdb/10_settings.yml uses one of the features included in MapperFeature.

Basically that provides you a way to gate test execution based on when some feature/change has been introduced. There has been some discussion around using node features for such gates but I don't see a better way to do it. In your case it would be something like "Change fields api response from 500 to 400" or something.

@@ -128,18 +128,76 @@ fetch _seq_no via stored_fields:

---
fetch _seq_no via fields:
- requires:
cluster_features: ["error_code_changed"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a NodeFeature in MapperFeatures:: getTestFeatures to flag this method for execution. The naming is generic and can be used in other cases if needed.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

Left a small comment, LGTM otherwise

@@ -71,7 +73,8 @@ public Set<NodeFeature> getTestFeatures() {
IgnoredSourceFieldMapper.IGNORED_SOURCE_AS_TOP_LEVEL_METADATA_ARRAY_FIELD,
IgnoredSourceFieldMapper.ALWAYS_STORE_OBJECT_ARRAYS_IN_NESTED_OBJECTS,
MapperService.LOGSDB_DEFAULT_IGNORE_DYNAMIC_BEYOND_LIMIT,
CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX
CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,
ERROR_CODE_CHANGED
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should include some more details about the change in the constant name, like META_FETCH_FIELDS_ERROR_CODE_CHANGED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @javanna; the name you proposed looks better.

@drempapis drempapis added v8.18.0 and removed v8.17.0 labels Dec 3, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @drempapis, I've updated the changelog YAML for you.

@drempapis drempapis merged commit a514aad into elastic:main Dec 3, 2024
16 checks passed
drempapis added a commit to drempapis/elasticsearch that referenced this pull request Dec 3, 2024
 400 rather a 5xx error is returned when _source / _seq_no / _feature / _nested_path / _field_names is requested, via fields
elasticsearchmachine pushed a commit that referenced this pull request Dec 3, 2024
400 rather a 5xx error is returned when _source / _seq_no / _feature / _nested_path / _field_names is requested, via fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >bug :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants