Skip to content

Improve handling of empty response #125562

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Apr 7, 2025
Merged

Improve handling of empty response #125562

merged 15 commits into from
Apr 7, 2025

Conversation

DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Mar 25, 2025 •
edited
Loading

Today ActionResponse$Empty implements ToXContentObject, but yields
no bytes of content when serialized which creates an invalid JSON
response. This commit removes the bogus interface and adjusts the
affected REST APIs to send a text/plain response instead.

Closes #57639

@DaveCTurner
Improve handling of empty response
a1001e1 
Today `ActionResponse$Empty` implements `ToXContentObject`, but yields
no bytes of content when serialized which creates an invalid JSON
response. This commit removes the bogus interface and adjusts the
affected REST APIs to send a `text/plain` response instead.
@DaveCTurner DaveCTurner added >bug :Core/Infra/REST API REST infrastructure and utilities v9.1.0 labels Mar 25, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 25, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@DaveCTurner DaveCTurner requested a review from rjernst March 25, 2025 13:53
ldematte
ldematte reviewed Apr 4, 2025
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You probably want to wait for Ryan's review but this looks good to me. Nice use of capabilities too!

@ldematte
Copy link
Contributor

ldematte commented Apr 4, 2025

I have only a tiny doubt about BwC tests: with the changes you made to the rest spec YAML, are those going to be affected? E.g. for mixed clusters?

@DaveCTurner
Copy link
Contributor Author

I have only a tiny doubt about BwC tests: with the changes you made to the rest spec YAML, are those going to be affected? E.g. for mixed clusters?

I don't follow - I'm skipping these tests in a mixed cluster unless all the nodes have the plain_text_empty_response capability, because they won't pass the check added to ClientYamlTestResponse.java.

@DaveCTurner DaveCTurner requested a review from ldematte April 4, 2025 10:35
@DaveCTurner
Copy link
Contributor Author

@rjernst this somewhat relates to #126364 and #126374: after we've done this there's no real need for TransportResponse and ActionResponse to be different, we can replace them both with a single interface.

ldematte
ldematte approved these changes Apr 7, 2025
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

I don't follow

It was just a general concern; if you can skip those and not care it's fine.
LGTM

@DaveCTurner DaveCTurner merged commit 527d2a2 into elastic:main Apr 7, 2025
17 checks passed
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 7, 2025
@DaveCTurner
Improve handling of empty response
0d5dd1f 
Today `ActionResponse$Empty` implements `ToXContentObject`, but yields
no bytes of content when serialized which creates an invalid JSON
response. This commit removes the bogus interface and adjusts the
affected REST APIs to send a `text/plain` response instead.

Backport of elastic#125562 to `8.x`
@DaveCTurner DaveCTurner deleted the 2025/03/25/empty-response-not-xcontent branch April 7, 2025 11:23
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 7, 2025
@DaveCTurner
Improve handling of empty response
cbec62e 
Today `ActionResponse$Empty` implements `ToXContentObject`, but yields
no bytes of content when serialized which creates an invalid JSON
response. This commit removes the bogus interface and adjusts the
affected REST APIs to send a `text/plain` response instead.

Backport of elastic#125562 to `8.x`
@DaveCTurner DaveCTurner mentioned this pull request Apr 7, 2025
Merged
@DaveCTurner
Copy link
Contributor Author

Ah as this is a bugfix I meant to mark it for backport to 8.19 but forgot - backported now in #126393 but I've left that for review in case you have concerns about backporting it.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 7, 2025
@DaveCTurner
Forward port changes from backport of elastic#125562
c0bb260 
The backport to `8.x` needed some changes to pass through CI; this
commit forward-ports the relevant bits of those changes back into `main`
to keep the branches aligned.
DaveCTurner added a commit that referenced this pull request Apr 7, 2025
@DaveCTurner
Forward port changes from backport of #125562 (#126413)
f6c1965 
The backport to `8.x` needed some changes to pass through CI; this
commit forward-ports the relevant bits of those changes back into `main`
to keep the branches aligned.
mridula-s109 pushed a commit that referenced this pull request Apr 7, 2025
@DaveCTurner @mridula-s109
Forward port changes from backport of #125562 (#126413)
8245239 
The backport to `8.x` needed some changes to pass through CI; this
commit forward-ports the relevant bits of those changes back into `main`
to keep the branches aligned.
DaveCTurner added a commit that referenced this pull request Apr 8, 2025
@DaveCTurner
Improve handling of empty response (#126393)
0dbe2d7 
Today `ActionResponse$Empty` implements `ToXContentObject`, but yields
no bytes of content when serialized which creates an invalid JSON
response. This commit removes the bogus interface and adjusts the
affected REST APIs to send a `text/plain` response instead.

Backport of #125562 to `8.x`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldematte ldematte ldematte approved these changes

@rjernst rjernst Awaiting requested review from rjernst

Assignees
No one assigned
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team v8.19.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Voting config exclusions APIs return \n response
3 participants
@DaveCTurner @elasticsearchmachine @ldematte