Skip to content

Fix/meta fields bad request#117229

Merged
drempapis merged 28 commits intoelastic:mainfrom
drempapis:fix/meta-fields-bad-request
Dec 3, 2024
Merged

Fix/meta fields bad request#117229
drempapis merged 28 commits intoelastic:mainfrom
drempapis:fix/meta-fields-bad-request

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.

---
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
Contributor

@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

MapperService.LOGSDB_DEFAULT_IGNORE_DYNAMIC_BEYOND_LIMIT,
CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX
CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,
ERROR_CODE_CHANGED
Copy link
Contributor

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
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

4 participants