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

(PDB-5645) Call handler with response when :include-response set #251

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

rbrw
Copy link
Contributor

@rbrw rbrw commented May 31, 2023

If :include-response is true in an add-ring-handler call, then call the handler with the pending request Response as a second argument so that the handler can interrogate/manipulate the response directly if desired. PuppetDB intends to use this to detect client disconnects via the SocketChannel (via the Response getHttpChannel) in order to halt a potentially expensive query sooner.

@rbrw rbrw requested a review from a team as a code owner May 31, 2023 16:36
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rbrw
Copy link
Contributor Author

rbrw commented May 31, 2023

I added the "don't merge" since this will support a forthcoming PDB pr, and thought I'd like to get that one settled too before we proceed, though I don't currently anticipate any further changes to this side.

@rbrw rbrw force-pushed the pdb-5645-call-handler-with-response branch 4 times, most recently from 060d266 to 0a38c5f Compare May 31, 2023 19:15
@jonathannewman
Copy link
Contributor

jonathannewman commented Jun 6, 2023

@rbrw Note that we are working on the jetty 10 version of this service currently (we have everything but the web socket implementation working) with the intent of putting it into all the main branches soon. Assuming since this is a new feature it is only landing on the main branches, so you might want to delay until jetty 10 is done.

@jonathannewman
Copy link
Contributor

@rbrw rbrw force-pushed the pdb-5645-call-handler-with-response branch from 0a38c5f to 51849ae Compare June 8, 2023 22:10
@rbrw
Copy link
Contributor Author

rbrw commented Jun 8, 2023

OK, thanks. Appreciate the notice. I think we'd like to merge this if we can sometime in the next two weeks (though after the releases), and yep, it's currently only intended for pdb 8.

Also had to rework the approach a bit (now done).

@rbrw rbrw force-pushed the pdb-5645-call-handler-with-response branch from 51849ae to 3b9d020 Compare June 8, 2023 22:36
@rbrw rbrw force-pushed the pdb-5645-call-handler-with-response branch from 3b9d020 to 95aca0f Compare June 27, 2023 01:32
CHANGELOG.md Outdated
@@ -1,3 +1,11 @@
## pending
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to make an immediate release of this feature, I'd say we put the version here to save ourselves a PR.

"Currently only provides support for aliased keyword access for
clojure versions without :as-alias (before 1.11)."
(:require
[clojure.spec.alpha :as s])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why spec here? It looks like the rest of the project is still on prismatic

If :include-response is true in an add-ring-handler call, then include
the pending request Response instance in the request map as
:puppetlabs.trapperkeeper.services.webserver.jetty9/response so that
the handler can interrogate/manipulate the response directly if
desired.

PuppetDB intends to use this to detect client disconnects via the
SocketChannel (via the Response getHttpChannel) in order to halt a
potentially expensive query sooner.
@rbrw rbrw force-pushed the pdb-5645-call-handler-with-response branch from 95aca0f to 571d6e6 Compare July 10, 2023 22:12
@austb austb merged commit 3f5040c into puppetlabs:main Jul 10, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants